| Message ID | 1441658846-5786-1-git-send-email-cedric.marie@openmailbox.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On 07-09-15 22:47, Cédric Marie wrote: > KBUILD_VERBOSE and quiet variables are set and exported, but they are > not used. We can safely remove them. > > These variables are inherited from the Makefile of the Linux kernel, > and are not used in Buildroot. > > In support/scripts/mkmakefile, quiet value is checked, but the test is > always true (quiet is never set to silent_), so the test can be removed > as well. > > Signed-off-by: Cédric Marie <cedric.marie@openmailbox.org> Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Tested-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> [A few build tests with and without V= ] Don't forget to version your patches and keep a patch changelog... I would already have introduced the VERBOSE construct in this patch, but I guess this one is less controversial, so OK. Regards, Arnout [snip]
Hi, Le 2015-09-07 23:14, Arnout Vandecappelle a écrit : > Don't forget to version your patches and keep a patch changelog... I had this remark in mind - you told me some time ago - but I intended to do it with the patch that will handle verbosity in infrastructures. I kind of considered this one as a new one. Since the patch has been split into several patches, should I increment the version for every new patch? > I would already have introduced the VERBOSE construct in this patch, > but I > guess this one is less controversial, so OK. I have tried to follow your guidelines: > 1. Remove KBUILD_VERBOSE and quiet (this one will be uncontroversial I > think) > 2. Define VERBOSE based on V= passed on command line > 3. Use VERBOSE in autotools and cmake > 4. Update documentation. Do you confirm I still should send two separate patches for steps 2 and 3? Regards,
On 08-09-15 14:14, Cédric Marie wrote: > Hi, > > Le 2015-09-07 23:14, Arnout Vandecappelle a écrit : >> Don't forget to version your patches and keep a patch changelog... > > I had this remark in mind - you told me some time ago - but I intended to do it > with the patch that will handle verbosity in infrastructures. I kind of > considered this one as a new one. > Since the patch has been split into several patches, should I increment the > version for every new patch? Yeah, that's more convenient. The goal of the version number is to make it clear that something similar has come by before and that that previous thing is superseded by the new thing. > > >> I would already have introduced the VERBOSE construct in this patch, but I >> guess this one is less controversial, so OK. > > I have tried to follow your guidelines: > >> 1. Remove KBUILD_VERBOSE and quiet (this one will be uncontroversial I think) >> 2. Define VERBOSE based on V= passed on command line >> 3. Use VERBOSE in autotools and cmake >> 4. Update documentation. Oops, you're right, I proposed that myself :-) But I didn't realise that the VERBOSE variable existed already. > > > Do you confirm I still should send two separate patches for steps 2 and 3? No, since VERBOSE already exists, my proposed step 2 is useless. Regards, Arnout > > Regards, >
Le 2015-09-08 14:30, Arnout Vandecappelle a écrit : >>> 1. Remove KBUILD_VERBOSE and quiet (this one will be uncontroversial >>> I think) >>> 2. Define VERBOSE based on V= passed on command line >>> 3. Use VERBOSE in autotools and cmake >>> 4. Update documentation. > No, since VERBOSE already exists, my proposed step 2 is useless. OK. Please note that I will change it a little bit: - remove export VERBOSE - VERBOSE=1 if V=1, even if VERBOSE is already defined (i.e. remove "ifndef VERBOSE" test) - VERBOSE=0 if V=0 - VERBOSE=(empty) otherwise to prevent this variable from being inherited from the shell. Regards,
On 08-09-15 15:01, Cédric Marie wrote: > Le 2015-09-08 14:30, Arnout Vandecappelle a écrit : >>>> 1. Remove KBUILD_VERBOSE and quiet (this one will be uncontroversial I think) >>>> 2. Define VERBOSE based on V= passed on command line >>>> 3. Use VERBOSE in autotools and cmake >>>> 4. Update documentation. > >> No, since VERBOSE already exists, my proposed step 2 is useless. > > OK. Please note that I will change it a little bit: > - remove export VERBOSE > - VERBOSE=1 if V=1, even if VERBOSE is already defined (i.e. remove "ifndef > VERBOSE" test) > - VERBOSE=0 if V=0 > - VERBOSE=(empty) otherwise to prevent this variable from being inherited from > the shell. Good! Note in the commit log why it has to be that way (for cmake, I guess). Regards, Arnout
Hi, I've just realized that this patch is breaking linux and busybox verbose mode. Exporting KBUILD_VERBOSE makes it available for linux and busybox makefile. This makefile sets KBUILD_VERBOSE if V is set in the command line, but it also uses KBUILD_VERBOSE if exported by the shell. In fact it seems that the original idea was to export all variables that could be understood by different build systems: - KBUILD_VERBOSE for linux and busybox - VERBOSE for CMake packages. So, if we still want to handle verbosity in infrastructures (and some packages that use generic infrastructure) - and I think we should: I think the right way to forward verbose mode to linux and busybox is to add V=1 in linux.mk and busybox.mk. As a consequence, I believe it is not possible to split my patch into different steps. The patch must: - remove quiet - remove KBUILD_VERBOSE - use VERBOSE but in a different way (not exported), as already described - handle VERBOSE in pkg-cmake.mk, pkg-autotools.mk, linux.mk, busybox.mk, and qt.mk (this one is possibly already correct). If you agree, I will provide a single patch that will cancel the two previous ones (I will add patch version and log this time). Regards,
On 11-09-15 09:52, Cédric Marie wrote: > Hi, > > I've just realized that this patch is breaking linux and busybox verbose mode. > > Exporting KBUILD_VERBOSE makes it available for linux and busybox makefile. This > makefile sets KBUILD_VERBOSE if V is set in the command line, but it also uses > KBUILD_VERBOSE if exported by the shell. > > In fact it seems that the original idea was to export all variables that could > be understood by different build systems: > - KBUILD_VERBOSE for linux and busybox > - VERBOSE for CMake packages. > > So, if we still want to handle verbosity in infrastructures (and some packages > that use generic infrastructure) - and I think we should: > I think the right way to forward verbose mode to linux and busybox is to add V=1 > in linux.mk and busybox.mk. I'm not so sure of that. The KBUILD_VERBOSE approach worked well, there wasn't really a problem with it. The problem was with VERBOSE, which is interpreted differently by cmake. Removing KBUILD_VERBOSE and quiet was just because (quoting your commit message): "KBUILD_VERBOSE and quiet variables are set and exported, but they are not used. We can safely remove them." The current way (exporting KBUILD_VERBOSE) is in fact quite elegant: it's just a couple of lines of code, and it works for any package based on Kbuild. Like uClibc, barebox, who knows, maybe they also use KBUILD_VERBOSE? This way is much easier than handling it for every package separately. So I think the approach should be: - remove quiet - add an explanation about why KBUILD_VERBOSE is useful - use VERBOSE but in a different way. and in fact, since we already have KBUILD_VERBOSE, maybe VERBOSE can just be dropped? (internally, it's still used for cmake.) > > As a consequence, I believe it is not possible to split my patch into different > steps. > The patch must: > - remove quiet > - remove KBUILD_VERBOSE > - use VERBOSE but in a different way (not exported), as already described > - handle VERBOSE in pkg-cmake.mk, pkg-autotools.mk, linux.mk, busybox.mk, and > qt.mk (this one is possibly already correct). > > If you agree, I will provide a single patch that will cancel the two previous > ones (I will add patch version and log this time). The reason for splitting was that the first one would be uncontroversial - but it clearly isn't, so I'm OK with keeping them merged. Thanks for keeping this up! Regards, Arnout > > Regards, >
Cédric, On Mon, 7 Sep 2015 22:47:26 +0200, Cédric Marie wrote: > KBUILD_VERBOSE and quiet variables are set and exported, but they are > not used. We can safely remove them. > > These variables are inherited from the Makefile of the Linux kernel, > and are not used in Buildroot. > > In support/scripts/mkmakefile, quiet value is checked, but the test is > always true (quiet is never set to silent_), so the test can be removed > as well. > > Signed-off-by: Cédric Marie <cedric.marie@openmailbox.org> > --- > Makefile | 20 ++++++-------------- > support/scripts/mkmakefile | 4 +--- > 2 files changed, 7 insertions(+), 17 deletions(-) Following the discussion you had with Arnout, and your own report that this patch was breaking things in the Busybox and Linux kernel build, I marked this patch as "Changes Requested" in patchwork. Will you work on a new iteration of this patch? Thanks! Thomas
Hi Thomas, Arnout, and all, Le 20/09/2015 15:11, Thomas Petazzoni a écrit : > Following the discussion you had with Arnout, and your own report that > this patch was breaking things in the Busybox and Linux kernel build, I > marked this patch as "Changes Requested" in patchwork. > > Will you work on a new iteration of this patch? Yes, I've started to work on a new iteration. It is working fine with CMake, autotools, and Kbuild packages (I keep on exporting KBUILD_VERBOSE as suggested by Arnout). I still have to: - modify qt.mk because it currently enables verbose mode if VERBOSE is defined and not empty, but my modification implies that VERBOSE may be equal to 0, and it should not enable verbose mode in that case!... - check that no other package .mk file is using VERBOSE variable. - write clear comments and commit message. Sorry for the delay, but I've got only very occasional free time to spend on it.
Hello, On Sun, 20 Sep 2015 22:43:54 +0200, Cédric Marie wrote: > Le 20/09/2015 15:11, Thomas Petazzoni a écrit : > > Following the discussion you had with Arnout, and your own report that > > this patch was breaking things in the Busybox and Linux kernel build, I > > marked this patch as "Changes Requested" in patchwork. > > > > Will you work on a new iteration of this patch? > > Yes, I've started to work on a new iteration. It is working fine with > CMake, autotools, and Kbuild packages (I keep on exporting > KBUILD_VERBOSE as suggested by Arnout). > > I still have to: > > - modify qt.mk because it currently enables verbose mode if VERBOSE is > defined and not empty, but my modification implies that VERBOSE may be > equal to 0, and it should not enable verbose mode in that case!... > - check that no other package .mk file is using VERBOSE variable. > - write clear comments and commit message. Excellent, thanks! > Sorry for the delay, but I've got only very occasional free time to > spend on it. No problem at all. I just e-mailed you to let you know that your patch has been marked "Changes Requested": it means that it is no longer visible in the list of pending patches to be considered by the maintainers. So when this is done, it is up to the submitter to come back at some point with a new iteration of the patch, otherwise the topic will simply be forgotten by the maintainers. This is why I generally send a notification e-mail when changing a patch to "Changes Requested". There is obviously no problem if the new iteration only comes in several weeks or even months. Thanks again, Thomas
On 20-09-15 22:43, Cédric Marie wrote: > Hi Thomas, Arnout, and all, > > Le 20/09/2015 15:11, Thomas Petazzoni a écrit : >> Following the discussion you had with Arnout, and your own report that >> this patch was breaking things in the Busybox and Linux kernel build, I >> marked this patch as "Changes Requested" in patchwork. >> >> Will you work on a new iteration of this patch? > > Yes, I've started to work on a new iteration. It is working fine with CMake, > autotools, and Kbuild packages (I keep on exporting KBUILD_VERBOSE as suggested > by Arnout). > > I still have to: > > - modify qt.mk because it currently enables verbose mode if VERBOSE is defined > and not empty, but my modification implies that VERBOSE may be equal to 0, and > it should not enable verbose mode in that case!... Well, just unset VERBOSE when V=0. So VERBOSE is either empty or 1. Similar with how Config.in works. > - check that no other package .mk file is using VERBOSE variable. git grep -w VERBOSE -> only in qt.mk > - write clear comments and commit message. Yep, very important! Regards, Arnout > > Sorry for the delay, but I've got only very occasional free time to spend on it. > >
Le 2015-09-20 23:26, Arnout Vandecappelle a écrit : >> - modify qt.mk because it currently enables verbose mode if VERBOSE is >> defined >> and not empty, but my modification implies that VERBOSE may be equal >> to 0, and >> it should not enable verbose mode in that case!... > > Well, just unset VERBOSE when V=0. So VERBOSE is either empty or 1. > Similar > with how Config.in works. Well, the idea was to set VERBOSE in Makefile and use its value in infrastructures. In autotools I need to make the difference between VERBOSE=1 (force verbose), VERBOSE=0 (force non-verbose), and VERBOSE not set (keep package default verbose level). Unless we decide that finally there is no real interest in forcing non-verbose and adding a new possibility with 'make V=0'. I'm not so sure it can be very usefull, since it will only be taken into account by a few autotools package. If you want a quiet output, you'd rather use 'make -s' NB: I found it in Makefile, but it doesn't seem to be documented...
On 21-09-15 12:23, Cédric Marie wrote: > Le 2015-09-20 23:26, Arnout Vandecappelle a écrit : >>> - modify qt.mk because it currently enables verbose mode if VERBOSE is defined >>> and not empty, but my modification implies that VERBOSE may be equal to 0, and >>> it should not enable verbose mode in that case!... >> >> Well, just unset VERBOSE when V=0. So VERBOSE is either empty or 1. Similar >> with how Config.in works. > > Well, the idea was to set VERBOSE in Makefile and use its value in infrastructures. > In autotools I need to make the difference between VERBOSE=1 (force verbose), > VERBOSE=0 (force non-verbose), and VERBOSE not set (keep package default verbose > level). Right, sorry, I forgot about that. > > Unless we decide that finally there is no real interest in forcing non-verbose > and adding a new possibility with 'make V=0'. > I'm not so sure it can be very usefull, since it will only be taken into account > by a few autotools package. A few? Isn't it going to be a very large subset of the 1041 autotools packages? > If you want a quiet output, you'd rather use 'make -s' > NB: I found it in Makefile, but it doesn't seem to be documented... True. 'make -s' is documented in the make documentation of course, but it would make sense to also mention it in the buildroot documentation close to the V=1. Regards, Arnout
Hi, The more I try to fix it, the more I believe there is nothing to fix... :) Arnout has pointed out that KBUILD_VERBOSE should be kept, because it was a nice way to set verbose level in Kbuild packages, without having to modify any .mk files. I have also recently realized that autotools infrastructure didn't need anything more to have support for V=0 or 1. When V is in the command line, it is exported to all sub-makes. As a consequence, an autotools package already sees the value of V. It doesn't check whether it comes from the command line or not. I thought there was a problem because I first checked with autotools packages that did not support verbose level, and couldn't see any difference. With libpthsem, I have realized that V=0/1 was already supported. The fact that V is exported is not enough for Kbuild packages, because they also check that it comes from the command line. That's why they need KBUILD_VERBOSE to be exported. Autotools packages don't check that V comes from the command line. So, in the end, I think it would be stupid to convert V=0/1 into VERBOSE=0/1, and then, in pkg-autotools.mk, convert VERBOSE=0/1 into V=0/1 - which is already set and exported - again!... So, to sum up: - we keep KBUILD_VERBOSE for Kbuild packages - we don't need to change anything for autotools packages - If CMake is the only one that needs a conversion (from V to VERBOSE), wouldn't it be more simple to keep on exporting VERBOSE as it is done currently? In the end, the only thing I could do, is just to remove "quiet" :) I can still provide a patch for that, if it makes sense. Sorry for the time we have spent on this subject, while there is - almost - nothing to change in the end... There are many subtleties in makefiles that I was not aware of, in the first place.
On 02-10-15 16:52, Cédric Marie wrote: > Hi, > > The more I try to fix it, the more I believe there is nothing to fix... :) > > Arnout has pointed out that KBUILD_VERBOSE should be kept, because it was a nice > way to set verbose level in Kbuild packages, without having to modify any .mk > files. > > I have also recently realized that autotools infrastructure didn't need anything > more to have support for V=0 or 1. > When V is in the command line, it is exported to all sub-makes. As a > consequence, an autotools package already sees the value of V. It doesn't check > whether it comes from the command line or not. > > I thought there was a problem because I first checked with autotools packages > that did not support verbose level, and couldn't see any difference. With > libpthsem, I have realized that V=0/1 was already supported. > > The fact that V is exported is not enough for Kbuild packages, because they also > check that it comes from the command line. That's why they need KBUILD_VERBOSE > to be exported. Autotools packages don't check that V comes from the command line. > > So, in the end, I think it would be stupid to convert V=0/1 into VERBOSE=0/1, > and then, in pkg-autotools.mk, convert VERBOSE=0/1 into V=0/1 - which is already > set and exported - again!... > > So, to sum up: > - we keep KBUILD_VERBOSE for Kbuild packages > - we don't need to change anything for autotools packages > - If CMake is the only one that needs a conversion (from V to VERBOSE), wouldn't > it be more simple to keep on exporting VERBOSE as it is done currently? > > In the end, the only thing I could do, is just to remove "quiet" :) > I can still provide a patch for that, if it makes sense. Yep, sounds like a good idea! > > Sorry for the time we have spent on this subject, while there is - almost - > nothing to change in the end... > There are many subtleties in makefiles that I was not aware of, in the first place. It's great that you investigated all this, I wouldn't call this time wasted. Now I think we really understand most of the subtleties of the different verbose options. One more fix you could do is to document all this in comments in the top-level Makefile. Something like: # autotools packages use the V=0/1 option, which is passed implicitly by make. # Kbuild packages use KBUILD_VERBOSE # CMake packages use VERBOSE # Others don't have a supported verbosity switch export VERBOSE KBUILD_VERBOSE I wonder if exporting the Q is useful at all? Regards, Arnout
diff --git a/Makefile b/Makefile index 23e2ee6..05dd2db 100644 --- a/Makefile +++ b/Makefile @@ -218,23 +218,15 @@ endif # To put more focus on warnings, be less verbose as default # Use 'make V=1' to see the full commands +Q = @ ifeq ("$(origin V)", "command line") - KBUILD_VERBOSE = $(V) -endif -ifndef KBUILD_VERBOSE - KBUILD_VERBOSE = 0 -endif - -ifeq ($(KBUILD_VERBOSE),1) - quiet = - Q = +ifeq ($(V),1) +Q = ifndef VERBOSE - VERBOSE = 1 +VERBOSE = 1 endif export VERBOSE -else - quiet = quiet_ - Q = @ +endif endif # we want bash as shell @@ -245,7 +237,7 @@ SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \ # kconfig uses CONFIG_SHELL CONFIG_SHELL := $(SHELL) -export SHELL CONFIG_SHELL quiet Q KBUILD_VERBOSE +export SHELL CONFIG_SHELL Q ifndef HOSTAR HOSTAR := ar diff --git a/support/scripts/mkmakefile b/support/scripts/mkmakefile index 833be6a..37162a3 100755 --- a/support/scripts/mkmakefile +++ b/support/scripts/mkmakefile @@ -15,9 +15,7 @@ if test -e $2/Makefile && ! grep -q Automatically $2/Makefile then exit 0 fi -if [ "${quiet}" != "silent_" ]; then - echo " GEN $2/Makefile" -fi +echo " GEN $2/Makefile" cat << EOF > $2/Makefile # Automatically generated by $0: don't edit
KBUILD_VERBOSE and quiet variables are set and exported, but they are not used. We can safely remove them. These variables are inherited from the Makefile of the Linux kernel, and are not used in Buildroot. In support/scripts/mkmakefile, quiet value is checked, but the test is always true (quiet is never set to silent_), so the test can be removed as well. Signed-off-by: Cédric Marie <cedric.marie@openmailbox.org> --- Makefile | 20 ++++++-------------- support/scripts/mkmakefile | 4 +--- 2 files changed, 7 insertions(+), 17 deletions(-)