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

From: Junio C Hamano <gitster@pobox.com>
Date: May 31, 2016, 17:00 UTC
Message-ID: <xmqqpos25fiq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160531132443.5033-1-Matthieu.Moy@imag.fr>

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

Show 22 quoted lines

> 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.

Show 45 quoted lines

>  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