# [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion 7 messages from 2016-05-31 to 2016-06-01. Participants: Matthieu Moy, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/42493 ## Matthieu Moy, 2016-05-31 13:24 Subject: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion Message-ID: <20160531132443.5033-1-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/20160531132443.5033-1-Matthieu.Moy%40imag.fr ``` The DEVELOPER knob was introduced in 658df95 (add DEVELOPER makefile knob to check for acknowledged warnings, 2016-02-25), and works well when used as "make DEVELOPER=1", and when the configure script was not used. However, the advice given in CodingGuidelines to add DEVELOPER=1 to config.mak does not: config.mak is included after testing for DEVELOPER in the Makefile, and at least GNU Make's manual specifies "Conditional directives are parsed immediately", hence the config.mak declaration is not visible at the time the conditional is evaluated. Also, when using the configure script to generate a config.mak.autogen, the later file contained a "CFLAGS = " initialization, which overrode the "CFLAGS += -W..." triggered by DEVELOPER. This patch fixes both issues. Signed-off-by: Matthieu Moy --- I'm surprised that no one noticed the issue. Probably because the Makefile is silent by default. Did I miss anything obvious? Makefile | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/Makefile b/Makefile index 15fcd57..2226319 100644 --- a/Makefile +++ b/Makefile @@ -380,18 +380,6 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) STRIP ?= strip -ifdef DEVELOPER -CFLAGS += -Werror \ - -Wdeclaration-after-statement \ - -Wno-format-zero-length \ - -Wold-style-definition \ - -Woverflow \ - -Wpointer-arith \ - -Wstrict-prototypes \ - -Wunused \ - -Wvla -endif - # Create as necessary, replace existing, make ranlib unneeded. ARFLAGS = rcs @@ -952,6 +940,18 @@ include config.mak.uname -include config.mak.autogen -include config.mak +ifdef DEVELOPER +CFLAGS += -Werror \ + -Wdeclaration-after-statement \ + -Wno-format-zero-length \ + -Wold-style-definition \ + -Woverflow \ + -Wpointer-arith \ + -Wstrict-prototypes \ + -Wunused \ + -Wvla +endif + ifndef sysconfdir ifeq ($(prefix),/usr) sysconfdir = /etc -- 2.8.2.397.gbe91ebf.dirty ``` ## Junio C Hamano, 2016-05-31 17:00 Subject: Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion Message-ID: URL: https://gitlist.dev/e/xmqqpos25fiq.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160531132443.5033-1-Matthieu.Moy@imag.fr> ``` Matthieu Moy writes: > The DEVELOPER knob was introduced in 658df95 (add DEVELOPER makefile > knob to check for acknowledged warnings, 2016-02-25), and works well > when used as "make DEVELOPER=1", and when the configure script was not > used. > > However, the advice given in CodingGuidelines to add DEVELOPER=1 to > config.mak does not: config.mak is included after testing for > DEVELOPER in the Makefile, and at least GNU Make's manual specifies > "Conditional directives are parsed immediately", hence the config.mak > declaration is not visible at the time the conditional is evaluated. > > Also, when using the configure script to generate a > config.mak.autogen, the later file contained a "CFLAGS = " > initialization, which overrode the "CFLAGS += -W..." triggered by > DEVELOPER. > > This patch fixes both issues. > > Signed-off-by: Matthieu Moy > --- > I'm surprised that no one noticed the issue. Probably because the > Makefile is silent by default. Did I miss anything obvious? Probably because the overlap of the population that use DEVELOPER=1 and config.mak is miniscule? When you work in multiple "git worktrees" (or its equivalent), it is far more convenient to have a single "make" wrapper that you use everywhere than to make sure that you copy (or symlink) a config.mak into each and every one of them. In any case, this change is a right thing to do. Thanks for noticing. > Makefile | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/Makefile b/Makefile > index 15fcd57..2226319 100644 > --- a/Makefile > +++ b/Makefile > @@ -380,18 +380,6 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) > ALL_LDFLAGS = $(LDFLAGS) > STRIP ?= strip > > -ifdef DEVELOPER > -CFLAGS += -Werror \ > - -Wdeclaration-after-statement \ > - -Wno-format-zero-length \ > - -Wold-style-definition \ > - -Woverflow \ > - -Wpointer-arith \ > - -Wstrict-prototypes \ > - -Wunused \ > - -Wvla > -endif > - > # Create as necessary, replace existing, make ranlib unneeded. > ARFLAGS = rcs > > @@ -952,6 +940,18 @@ include config.mak.uname > -include config.mak.autogen > -include config.mak > > +ifdef DEVELOPER > +CFLAGS += -Werror \ > + -Wdeclaration-after-statement \ > + -Wno-format-zero-length \ > + -Wold-style-definition \ > + -Woverflow \ > + -Wpointer-arith \ > + -Wstrict-prototypes \ > + -Wunused \ > + -Wvla > +endif > + > ifndef sysconfdir > ifeq ($(prefix),/usr) > sysconfdir = /etc ``` ## Jeff King, 2016-06-01 07:30 Subject: Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion Message-ID: <20160601073037.GA14096@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160601073037.GA14096%40sigill.intra.peff.net In-Reply-To: <20160531132443.5033-1-Matthieu.Moy@imag.fr> ``` On Tue, May 31, 2016 at 03:24:43PM +0200, Matthieu Moy wrote: > The DEVELOPER knob was introduced in 658df95 (add DEVELOPER makefile > knob to check for acknowledged warnings, 2016-02-25), and works well > when used as "make DEVELOPER=1", and when the configure script was not > used. > > However, the advice given in CodingGuidelines to add DEVELOPER=1 to > config.mak does not: config.mak is included after testing for > DEVELOPER in the Makefile, and at least GNU Make's manual specifies > "Conditional directives are parsed immediately", hence the config.mak > declaration is not visible at the time the conditional is evaluated. > > Also, when using the configure script to generate a > config.mak.autogen, the later file contained a "CFLAGS = " > initialization, which overrode the "CFLAGS += -W..." triggered by > DEVELOPER. > > This patch fixes both issues. Hmm. So I think this does fix some issues, but it also means that one's config.mak cannot use DEVELOPER as a base and then override particular flags. I dunno if people want to do that or not. I do not use DEVELOPER myself because I have my own detailed config.mak that is a superset. -Peff ``` ## Matthieu Moy, 2016-06-01 07:57 Subject: Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion Message-ID: URL: https://gitlist.dev/e/vpqpos11gv3.fsf%40anie.imag.fr In-Reply-To: <20160601073037.GA14096@sigill.intra.peff.net> ``` Jeff King writes: > Hmm. So I think this does fix some issues, but it also means that one's > config.mak cannot use DEVELOPER as a base and then override particular > flags. You mean, using "make DEVELOPER=1" and then tweak CFLAGS in config.mak? Well, you still can do "CFLAGS += ..." (the extra CFLAGS will come before the ones added by DEVELOPER instead of after), which should cover 99% use-cases. You can't do "CFLAGS = $(filter-out ..., $(CFLAGS))" anymore indeed. But if you are at that level of customization, I'd say DEVELOPER isn't for you and you should just set CFLAGS directly. Not really serious, but we can fix that easily. Patch follows. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Matthieu Moy, 2016-06-01 08:00 Subject: [PATCH] Makefile: add $(DEVELOPER_CFLAGS) variable Message-ID: <20160601080008.7348-1-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/20160601080008.7348-1-Matthieu.Moy%40imag.fr In-Reply-To: ``` This does not change the behavior, but allows the user to tweak DEVELOPER_CFLAGS on the command-line or in a config.mak* file if needed. This also makes the code somewhat cleaner as it follows the pattern by specifying which flags to activate in the first part, and actually activating them in the last one. Signed-off-by: Matthieu Moy --- Junio, you can add this to mm/makefile-developer-can-be-in-config-mak (or squash it in the commit, but having two separate commit messages make sense IMO). Makefile | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 2226319..9753abe 100644 --- a/Makefile +++ b/Makefile @@ -375,6 +375,15 @@ GIT-VERSION-FILE: FORCE # CFLAGS and LDFLAGS are for the users to override from the command line. CFLAGS = -g -O2 -Wall +DEVELOPER_CFLAGS = -Werror \ + -Wdeclaration-after-statement \ + -Wno-format-zero-length \ + -Wold-style-definition \ + -Woverflow \ + -Wpointer-arith \ + -Wstrict-prototypes \ + -Wunused \ + -Wvla LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) @@ -941,15 +950,7 @@ include config.mak.uname -include config.mak ifdef DEVELOPER -CFLAGS += -Werror \ - -Wdeclaration-after-statement \ - -Wno-format-zero-length \ - -Wold-style-definition \ - -Woverflow \ - -Wpointer-arith \ - -Wstrict-prototypes \ - -Wunused \ - -Wvla +CFLAGS += $(DEVELOPER_CFLAGS) endif ifndef sysconfdir -- 2.8.2.397.gbe91ebf.dirty ``` ## Jeff King, 2016-06-01 08:03 Subject: Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion Message-ID: <20160601080348.GA22528@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160601080348.GA22528%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Jun 01, 2016 at 09:57:20AM +0200, Matthieu Moy wrote: > Jeff King writes: > > > Hmm. So I think this does fix some issues, but it also means that one's > > config.mak cannot use DEVELOPER as a base and then override particular > > flags. > > You mean, using "make DEVELOPER=1" and then tweak CFLAGS in config.mak? > > Well, you still can do "CFLAGS += ..." (the extra CFLAGS will come > before the ones added by DEVELOPER instead of after), which should cover > 99% use-cases. I specifically meant this, that your flags will now come before the DEVELOPER ones. So they will not override for any options which are parsed in command-line order (e.g., -Wno-error=something-specific). > You can't do "CFLAGS = $(filter-out ..., $(CFLAGS))" anymore indeed. But > if you are at that level of customization, I'd say DEVELOPER isn't for > you and you should just set CFLAGS directly. Yes, though it would be nice if the developer cflags were in a separate variable to make that easier to play with. Perhaps: DEVELOPER_CFLAGS += -Wfoo DEVELOPER_CFLAGS += -Wbar ... -include config.mak ... ifdef DEVELOPER CFLAGS += $(DEVELOPER_CFLAGS) endif would be more flexible. I don't currently use filter-out, but I do have compiler-specific flags (which I accomplish by just not adding them in the first place for certain compilers). For example, you may notice that: make DEVELOPER=1 CC=clang is broken (clang doesn't know -Wold-style-declaration). -Peff ``` ## Jeff King, 2016-06-01 08:05 Subject: Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion Message-ID: <20160601080504.GB22528@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160601080504.GB22528%40sigill.intra.peff.net In-Reply-To: <20160601080348.GA22528@sigill.intra.peff.net> ``` On Wed, Jun 01, 2016 at 04:03:48AM -0400, Jeff King wrote: > Perhaps: > > DEVELOPER_CFLAGS += -Wfoo > DEVELOPER_CFLAGS += -Wbar > ... > -include config.mak > ... > ifdef DEVELOPER > CFLAGS += $(DEVELOPER_CFLAGS) > endif > > would be more flexible. Heh, our mails crossed, but I see you had the same idea. I think it is a strict improvement. -Peff ```