{"thread":{"id":"42493","subject":"[RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion","startedAt":"2016-05-31T13:24:43Z","lastAt":"2016-06-01T08:05:04Z","messageCount":7,"participants":["Matthieu Moy","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"287897","messageId":"20160531132443.5033-1-Matthieu.Moy@imag.fr","threadId":"42493","inReplyTo":null,"subject":"[RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2016-05-31T13:24:43Z","receivedAt":"2016-05-31T13:24:43Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"The DEVELOPER knob was introduced in 658df95 (add DEVELOPER makefile\nknob to check for acknowledged warnings, 2016-02-25), and works well\nwhen used as \"make DEVELOPER=1\", and when the configure script was not\nused.\n\nHowever, the advice given in CodingGuidelines to add DEVELOPER=1 to\nconfig.mak does not: config.mak is included after testing for\nDEVELOPER in the Makefile, and at least GNU Make's manual specifies\n\"Conditional directives are parsed immediately\", hence the config.mak\ndeclaration is not visible at the time the conditional is evaluated.\n\nAlso, when using the configure script to generate a\nconfig.mak.autogen, the later file contained a \"CFLAGS = <flags>\"\ninitialization, which overrode the \"CFLAGS += -W...\" triggered by\nDEVELOPER.\n\nThis patch fixes both issues.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\nI'm surprised that no one noticed the issue. Probably because the\nMakefile is silent by default. Did I miss anything obvious?\n\n Makefile | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 15fcd57..2226319 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -380,18 +380,6 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n STRIP ?= strip\n \n-ifdef DEVELOPER\n-CFLAGS += -Werror \\\n-\t-Wdeclaration-after-statement \\\n-\t-Wno-format-zero-length \\\n-\t-Wold-style-definition \\\n-\t-Woverflow \\\n-\t-Wpointer-arith \\\n-\t-Wstrict-prototypes \\\n-\t-Wunused \\\n-\t-Wvla\n-endif\n-\n # Create as necessary, replace existing, make ranlib unneeded.\n ARFLAGS = rcs\n \n@@ -952,6 +940,18 @@ include config.mak.uname\n -include config.mak.autogen\n -include config.mak\n \n+ifdef DEVELOPER\n+CFLAGS += -Werror \\\n+\t-Wdeclaration-after-statement \\\n+\t-Wno-format-zero-length \\\n+\t-Wold-style-definition \\\n+\t-Woverflow \\\n+\t-Wpointer-arith \\\n+\t-Wstrict-prototypes \\\n+\t-Wunused \\\n+\t-Wvla\n+endif\n+\n ifndef sysconfdir\n ifeq ($(prefix),/usr)\n sysconfdir = /etc\n-- \n2.8.2.397.gbe91ebf.dirty\n"},{"id":"287917","messageId":"xmqqpos25fiq.fsf@gitster.mtv.corp.google.com","threadId":"42493","inReplyTo":"20160531132443.5033-1-Matthieu.Moy@imag.fr","subject":"Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-31T17:00:29Z","receivedAt":"2016-05-31T17:00:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n\n> The DEVELOPER knob was introduced in 658df95 (add DEVELOPER makefile\n> knob to check for acknowledged warnings, 2016-02-25), and works well\n> when used as \"make DEVELOPER=1\", and when the configure script was not\n> used.\n>\n> However, the advice given in CodingGuidelines to add DEVELOPER=1 to\n> config.mak does not: config.mak is included after testing for\n> DEVELOPER in the Makefile, and at least GNU Make's manual specifies\n> \"Conditional directives are parsed immediately\", hence the config.mak\n> declaration is not visible at the time the conditional is evaluated.\n>\n> Also, when using the configure script to generate a\n> config.mak.autogen, the later file contained a \"CFLAGS = <flags>\"\n> initialization, which overrode the \"CFLAGS += -W...\" triggered by\n> DEVELOPER.\n>\n> This patch fixes both issues.\n>\n> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n> ---\n> I'm surprised that no one noticed the issue. Probably because the\n> Makefile is silent by default. Did I miss anything obvious?\n\nProbably because the overlap of the population that use DEVELOPER=1\nand config.mak is miniscule?  \n\nWhen you work in multiple \"git worktrees\" (or its equivalent), it is\nfar more convenient to have a single \"make\" wrapper that you use\neverywhere than to make sure that you copy (or symlink) a config.mak\ninto each and every one of them.\n\nIn any case, this change is a right thing to do.  Thanks for\nnoticing.\n\n>  Makefile | 24 ++++++++++++------------\n>  1 file changed, 12 insertions(+), 12 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 15fcd57..2226319 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -380,18 +380,6 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n>  ALL_LDFLAGS = $(LDFLAGS)\n>  STRIP ?= strip\n>  \n> -ifdef DEVELOPER\n> -CFLAGS += -Werror \\\n> -\t-Wdeclaration-after-statement \\\n> -\t-Wno-format-zero-length \\\n> -\t-Wold-style-definition \\\n> -\t-Woverflow \\\n> -\t-Wpointer-arith \\\n> -\t-Wstrict-prototypes \\\n> -\t-Wunused \\\n> -\t-Wvla\n> -endif\n> -\n>  # Create as necessary, replace existing, make ranlib unneeded.\n>  ARFLAGS = rcs\n>  \n> @@ -952,6 +940,18 @@ include config.mak.uname\n>  -include config.mak.autogen\n>  -include config.mak\n>  \n> +ifdef DEVELOPER\n> +CFLAGS += -Werror \\\n> +\t-Wdeclaration-after-statement \\\n> +\t-Wno-format-zero-length \\\n> +\t-Wold-style-definition \\\n> +\t-Woverflow \\\n> +\t-Wpointer-arith \\\n> +\t-Wstrict-prototypes \\\n> +\t-Wunused \\\n> +\t-Wvla\n> +endif\n> +\n>  ifndef sysconfdir\n>  ifeq ($(prefix),/usr)\n>  sysconfdir = /etc\n"},{"id":"287983","messageId":"20160601073037.GA14096@sigill.intra.peff.net","threadId":"42493","inReplyTo":"20160531132443.5033-1-Matthieu.Moy@imag.fr","subject":"Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-01T07:30:37Z","receivedAt":"2016-06-01T07:30:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 31, 2016 at 03:24:43PM +0200, Matthieu Moy wrote:\n\n> The DEVELOPER knob was introduced in 658df95 (add DEVELOPER makefile\n> knob to check for acknowledged warnings, 2016-02-25), and works well\n> when used as \"make DEVELOPER=1\", and when the configure script was not\n> used.\n> \n> However, the advice given in CodingGuidelines to add DEVELOPER=1 to\n> config.mak does not: config.mak is included after testing for\n> DEVELOPER in the Makefile, and at least GNU Make's manual specifies\n> \"Conditional directives are parsed immediately\", hence the config.mak\n> declaration is not visible at the time the conditional is evaluated.\n> \n> Also, when using the configure script to generate a\n> config.mak.autogen, the later file contained a \"CFLAGS = <flags>\"\n> initialization, which overrode the \"CFLAGS += -W...\" triggered by\n> DEVELOPER.\n> \n> This patch fixes both issues.\n\nHmm. So I think this does fix some issues, but it also means that one's\nconfig.mak cannot use DEVELOPER as a base and then override particular\nflags.\n\nI dunno if people want to do that or not. I do not use DEVELOPER myself\nbecause I have my own detailed config.mak that is a superset.\n\n-Peff\n"},{"id":"287985","messageId":"vpqpos11gv3.fsf@anie.imag.fr","threadId":"42493","inReplyTo":"20160601073037.GA14096@sigill.intra.peff.net","subject":"Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-06-01T07:57:20Z","receivedAt":"2016-06-01T07:57:20Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King <peff@peff.net> writes:\n\n> Hmm. So I think this does fix some issues, but it also means that one's\n> config.mak cannot use DEVELOPER as a base and then override particular\n> flags.\n\nYou mean, using \"make DEVELOPER=1\" and then tweak CFLAGS in config.mak?\n\nWell, you still can do \"CFLAGS += ...\" (the extra CFLAGS will come\nbefore the ones added by DEVELOPER instead of after), which should cover\n99% use-cases.\n\nYou can't do \"CFLAGS = $(filter-out ..., $(CFLAGS))\" anymore indeed. But\nif you are at that level of customization, I'd say DEVELOPER isn't for\nyou and you should just set CFLAGS directly.\n\nNot really serious, but we can fix that easily. Patch follows.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287986","messageId":"20160601080008.7348-1-Matthieu.Moy@imag.fr","threadId":"42493","inReplyTo":"vpqpos11gv3.fsf@anie.imag.fr","subject":"[PATCH] Makefile: add $(DEVELOPER_CFLAGS) variable","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2016-06-01T08:00:08Z","receivedAt":"2016-06-01T08:00:08Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"This does not change the behavior, but allows the user to tweak\nDEVELOPER_CFLAGS on the command-line or in a config.mak* file if\nneeded.\n\nThis also makes the code somewhat cleaner as it follows the pattern\n\n<initialisation of variables>\n<include statements>\n<actual build logic>\n\nby specifying which flags to activate in the first part, and actually\nactivating them in the last one.\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\nJunio, you can add this to mm/makefile-developer-can-be-in-config-mak\n(or squash it in the commit, but having two separate commit messages\nmake sense IMO).\n\n Makefile | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 2226319..9753abe 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -375,6 +375,15 @@ GIT-VERSION-FILE: FORCE\n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n CFLAGS = -g -O2 -Wall\n+DEVELOPER_CFLAGS = -Werror \\\n+\t-Wdeclaration-after-statement \\\n+\t-Wno-format-zero-length \\\n+\t-Wold-style-definition \\\n+\t-Woverflow \\\n+\t-Wpointer-arith \\\n+\t-Wstrict-prototypes \\\n+\t-Wunused \\\n+\t-Wvla\n LDFLAGS =\n ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n@@ -941,15 +950,7 @@ include config.mak.uname\n -include config.mak\n \n ifdef DEVELOPER\n-CFLAGS += -Werror \\\n-\t-Wdeclaration-after-statement \\\n-\t-Wno-format-zero-length \\\n-\t-Wold-style-definition \\\n-\t-Woverflow \\\n-\t-Wpointer-arith \\\n-\t-Wstrict-prototypes \\\n-\t-Wunused \\\n-\t-Wvla\n+CFLAGS += $(DEVELOPER_CFLAGS)\n endif\n \n ifndef sysconfdir\n-- \n2.8.2.397.gbe91ebf.dirty\n"},{"id":"287987","messageId":"20160601080348.GA22528@sigill.intra.peff.net","threadId":"42493","inReplyTo":"vpqpos11gv3.fsf@anie.imag.fr","subject":"Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-01T08:03:48Z","receivedAt":"2016-06-01T08:03:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2016 at 09:57:20AM +0200, Matthieu Moy wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Hmm. So I think this does fix some issues, but it also means that one's\n> > config.mak cannot use DEVELOPER as a base and then override particular\n> > flags.\n> \n> You mean, using \"make DEVELOPER=1\" and then tweak CFLAGS in config.mak?\n> \n> Well, you still can do \"CFLAGS += ...\" (the extra CFLAGS will come\n> before the ones added by DEVELOPER instead of after), which should cover\n> 99% use-cases.\n\nI specifically meant this, that your flags will now come before the\nDEVELOPER ones. So they will not override for any options which are\nparsed in command-line order (e.g., -Wno-error=something-specific).\n\n> You can't do \"CFLAGS = $(filter-out ..., $(CFLAGS))\" anymore indeed. But\n> if you are at that level of customization, I'd say DEVELOPER isn't for\n> you and you should just set CFLAGS directly.\n\nYes, though it would be nice if the developer cflags were in a separate\nvariable to make that easier to play with.\n\nPerhaps:\n\n  DEVELOPER_CFLAGS += -Wfoo\n  DEVELOPER_CFLAGS += -Wbar\n  ...\n  -include config.mak\n  ...\n  ifdef DEVELOPER\n  CFLAGS += $(DEVELOPER_CFLAGS)\n  endif\n\nwould be more flexible.\n\nI don't currently use filter-out, but I do have compiler-specific flags\n(which I accomplish by just not adding them in the first place for\ncertain compilers). For example, you may notice that:\n\n  make DEVELOPER=1 CC=clang\n\nis broken (clang doesn't know -Wold-style-declaration).\n\n-Peff\n"},{"id":"287988","messageId":"20160601080504.GB22528@sigill.intra.peff.net","threadId":"42493","inReplyTo":"20160601080348.GA22528@sigill.intra.peff.net","subject":"Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-01T08:05:04Z","receivedAt":"2016-06-01T08:05:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2016 at 04:03:48AM -0400, Jeff King wrote:\n\n> Perhaps:\n> \n>   DEVELOPER_CFLAGS += -Wfoo\n>   DEVELOPER_CFLAGS += -Wbar\n>   ...\n>   -include config.mak\n>   ...\n>   ifdef DEVELOPER\n>   CFLAGS += $(DEVELOPER_CFLAGS)\n>   endif\n> \n> would be more flexible.\n\nHeh, our mails crossed, but I see you had the same idea. I think it is a\nstrict improvement.\n\n-Peff\n"}]}