[RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion

[RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion

[Matthieu Moy]

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 = <flags>"
initialization, which overrode the "CFLAGS += -W..." triggered by
DEVELOPER.

This patch fixes both issues.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
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

Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> 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 = <flags>"
> initialization, which overrode the "CFLAGS += -W..." triggered by
> DEVELOPER.
>
> This patch fixes both issues.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> 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

Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion

[Jeff King]

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 = <flags>"
> 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

Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion

[Matthieu Moy]

Jeff King <peff@peff.net> 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/

[PATCH] Makefile: add $(DEVELOPER_CFLAGS) variable

[Matthieu Moy]

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

<initialisation of variables>
<include statements>
<actual build logic>

by specifying which flags to activate in the first part, and actually
activating them in the last one.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
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

Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion

[Jeff King]

On Wed, Jun 01, 2016 at 09:57:20AM +0200, Matthieu Moy wrote:

> Jeff King <peff@peff.net> 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

Re: [RCF/PATCH] Makefile: move 'ifdef DEVELOPER' after config.mak* inclusion

[Jeff King]

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