[PATCH] Makefile: remove an out-of-date comment

[PATCH] Makefile: remove an out-of-date comment

[Ævar Arnfjörð Bjarmason]

Remove a comment referring to a caveat that hasn't been applicable
since 18b0fc1ce1 ("Git.pm: Kill Git.xs for now", 2006-09-23).

At the time of 8d7f586f13 ("Git.pm: Support for perl/ being built by a
different compiler", 2006-06-25) some of the code in perl would be
built by a C compiler, but support for that went away a few months
later in 18b0fc1ce1 discussed above.

Since my 20d2a30f8f ("Makefile: replace perl/Makefile.PL with simple
make rules", 2017-12-10) the perl/ directory doesn't even have its own
build process.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Makefile b/Makefile
index c5240942f2..97e922cc41 100644
--- a/Makefile
+++ b/Makefile
@@ -591,9 +591,6 @@ SPATCH_FLAGS = --all-includes --patch .
 
 ### --- END CONFIGURATION SECTION ---
 
-# Those must not be GNU-specific; they are shared with perl/ which may
-# be built by a different compiler. (Note that this is an artifact now
-# but it still might be nice to keep that distinction.)
 BASIC_CFLAGS = -I.
 BASIC_LDFLAGS =
 
-- 
2.21.0.rc2.261.ga7da99ff1b

[PATCH v2 0/6] Makefile: allow for combining DEVELOPER= and CFLAGS="..."

[Ævar Arnfjörð Bjarmason]

This fixes a long-standing annoyance of mine of not being able to set
my own CFLAGS combined with DEVELOPER="...".

There *should* be no other functional changes, but I'm moving around
some core Makefile logic, so this needs to be carefully reviewed.

I've tried installing with this, setting flags involved etc. It works
as expected, but we have no tests for the Makefile itself, so I might
have missed something.

I sent a stand-alone "Makefile: remove an out-of-date comment"[1] a
bit too soon as it turns out. I thought it could be stand-alone, but
as it turns out I needed to move some code adjacent to it. It would
conflict with this, so it's now bundled up with it as a v2.

1. https://public-inbox.org/git/20190222105658.26831-1-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (6):
  Makefile: remove an out-of-date comment
  Makefile: move "strip" assignment down from flags
  Makefile: add/remove comments at top and tweak whitespace
  Makefile: Move *_LIBS assignment into its own section
  Makefile: move the setting of *FLAGS closer to "include"
  Makefile: allow for combining DEVELOPER=1 and CFLAGS="..."

 Makefile       | 63 ++++++++++++++++++++++++++------------------------
 config.mak.dev | 44 +++++++++++++++++------------------
 2 files changed, 55 insertions(+), 52 deletions(-)

-- 
2.21.0.rc2.1.g2d5e20a900.dirty

Re: [PATCH v2 0/6] Makefile: allow for combining DEVELOPER= and CFLAGS="..."

[Jeff King]

On Fri, Feb 22, 2019 at 03:41:21PM +0100, Ævar Arnfjörð Bjarmason wrote:

> This fixes a long-standing annoyance of mine of not being able to set
> my own CFLAGS combined with DEVELOPER="...".

I already do this, via my config.mak. Are you passing these on the
command-line (and thus your CFLAGS= prevents DEVELOPER= from adding
anything to it)? That does seem worth fixing.

My personal complaint is that you cannot override DEVELOPER flags via
config.mak, because of the ordering. But it looks like your patch 6
specifically addresses that (yay!).

-Peff

Re: [PATCH v2 0/6] Makefile: allow for combining DEVELOPER= and CFLAGS="..."

[Ævar Arnfjörð Bjarmason]

On Fri, Feb 22 2019, Jeff King wrote:

> On Fri, Feb 22, 2019 at 03:41:21PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> This fixes a long-standing annoyance of mine of not being able to set
>> my own CFLAGS combined with DEVELOPER="...".
>
> I already do this, via my config.mak. Are you passing these on the
> command-line (and thus your CFLAGS= prevents DEVELOPER= from adding
> anything to it)? That does seem worth fixing.

Yes. I'll make this clearer in v2. I never use config.mak, so I didn't
even know that worked. Having refreshed on the docs[1], it's because
command-line arguments are "override variables".

> My personal complaint is that you cannot override DEVELOPER flags via
> config.mak, because of the ordering. But it looks like your patch 6
> specifically addresses that (yay!).

Do you mean DEVOPTS? That works for me.

    $ cat config.mak
    CFLAGS = -blah
    DEVELOPER = 1
    DEVOPTS = pedantic
    $ make V=1
    [...] cc [...]  -blah -Werror -pedantic [...]

But then of course:

    $ rm config.mak
    $ CFLAGS=-cmd DEVELOPER=1 DEVOPTS=pedantic V=1

Only includes the flags, and ignores DEVELOPER & DEVOPTS.

1. https://www.gnu.org/software/make/manual/html_node/Overriding.html

Re: [PATCH v2 0/6] Makefile: allow for combining DEVELOPER= and CFLAGS="..."

[Jeff King]

On Fri, Feb 22, 2019 at 10:59:20PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > My personal complaint is that you cannot override DEVELOPER flags via
> > config.mak, because of the ordering. But it looks like your patch 6
> > specifically addresses that (yay!).
> 
> Do you mean DEVOPTS? That works for me.

No, I meant overriding a developer CFLAG with a later one on the command
line. I.e., putting this in your config.mak:

  DEVELOPER = 1
  CFLAGS += -Wno-foo

doesn't work, because the user CFLAGS comes first. Your patch 6 does the
right thing.

-Peff

[PATCH v2 1/6] Makefile: remove an out-of-date comment

[Ævar Arnfjörð Bjarmason]

Remove a comment referring to a caveat that hasn't been applicable
since 18b0fc1ce1 ("Git.pm: Kill Git.xs for now", 2006-09-23).

At the time of 8d7f586f13 ("Git.pm: Support for perl/ being built by a
different compiler", 2006-06-25) some of the code in perl would be
built by a C compiler, but support for that went away a few months
later in 18b0fc1ce1 discussed above.

Since my 20d2a30f8f ("Makefile: replace perl/Makefile.PL with simple
make rules", 2017-12-10) the perl/ directory doesn't even have its own
build process.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Makefile b/Makefile
index c5240942f2..97e922cc41 100644
--- a/Makefile
+++ b/Makefile
@@ -591,9 +591,6 @@ SPATCH_FLAGS = --all-includes --patch .
 
 ### --- END CONFIGURATION SECTION ---
 
-# Those must not be GNU-specific; they are shared with perl/ which may
-# be built by a different compiler. (Note that this is an artifact now
-# but it still might be nice to keep that distinction.)
 BASIC_CFLAGS = -I.
 BASIC_LDFLAGS =
 
-- 
2.21.0.rc2.1.g2d5e20a900.dirty

[PATCH v2 2/6] Makefile: move "strip" assignment down from flags

[Ævar Arnfjörð Bjarmason]

Move the assignment of the "STRIP" variable down to where we're
setting variables with the names of other programs.

For consistency with those use "=" for the assignment instead of
"?=". I can't imagine why this would need to be different than the
rest, and 4dc00021f7 ("Makefile: add 'strip' target", 2006-01-12)
which added it doesn't provide an explanation.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 97e922cc41..c53727e44b 100644
--- a/Makefile
+++ b/Makefile
@@ -512,7 +512,6 @@ CFLAGS = -g -O2 -Wall
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
-STRIP ?= strip
 
 # Create as necessary, replace existing, make ranlib unneeded.
 ARFLAGS = rcs
@@ -576,6 +575,7 @@ CURL_CONFIG = curl-config
 PTHREAD_LIBS = -lpthread
 PTHREAD_CFLAGS =
 GCOV = gcov
+STRIP = strip
 SPATCH = spatch
 
 export TCL_PATH TCLTK_PATH
-- 
2.21.0.rc2.1.g2d5e20a900.dirty

Re: [PATCH v2 2/6] Makefile: move "strip" assignment down from flags

[Jeff King]

On Fri, Feb 22, 2019 at 03:41:23PM +0100, Ævar Arnfjörð Bjarmason wrote:

> Move the assignment of the "STRIP" variable down to where we're
> setting variables with the names of other programs.
> 
> For consistency with those use "=" for the assignment instead of
> "?=". I can't imagine why this would need to be different than the
> rest, and 4dc00021f7 ("Makefile: add 'strip' target", 2006-01-12)
> which added it doesn't provide an explanation.

This might annoy somebody expecting $STRIP in the environment to have
precedence. But I agree that consistency is probably our best strategy
here, and I don't see any reason the same argument would not apply to
$SPATCH, or $CC for that matter.

(So I could see an argument for moving them all to "?=", but that can
create its own confusion as environment variables accidentally start
taking effect).

-Peff

Re: [PATCH v2 2/6] Makefile: move "strip" assignment down from flags

[Ramsay Jones]

On 22/02/2019 15:18, Jeff King wrote:
> On Fri, Feb 22, 2019 at 03:41:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
>> Move the assignment of the "STRIP" variable down to where we're
>> setting variables with the names of other programs.
>>
>> For consistency with those use "=" for the assignment instead of
>> "?=". I can't imagine why this would need to be different than the
>> rest, and 4dc00021f7 ("Makefile: add 'strip' target", 2006-01-12)
>> which added it doesn't provide an explanation.
> 
> This might annoy somebody expecting $STRIP in the environment to have
> precedence. But I agree that consistency is probably our best strategy
> here, and I don't see any reason the same argument would not apply to
> $SPATCH, or $CC for that matter.
> 
> (So I could see an argument for moving them all to "?=", but that can
> create its own confusion as environment variables accidentally start
> taking effect).

$STRIP and $SPATCH will work OK, but you would be disappointed
with $CC (or any other variable from make's built-in database). ;-)

Try this:

  $ cat -n Makefile 
       1	
       2	CC ?= gcc
       3	
       4	all:
       5		@echo "CC is $(CC), origin " $(origin CC)
       6	
  $ 

The command-line works OK:

  $ make CC=cmd-line
  CC is cmd-line, origin  command line
  $ 

So does the environment:

  $ CC=env make
  CC is env, origin  environment
  $ 

But that conditional assignment:

  $ make
  CC is cc, origin  default
  $ 
  
... not so much! :-D

ATB,
Ramsay Jones

Re: [PATCH v2 2/6] Makefile: move "strip" assignment down from flags

[Jeff King]

On Fri, Feb 22, 2019 at 09:58:14PM +0000, Ramsay Jones wrote:

> > This might annoy somebody expecting $STRIP in the environment to have
> > precedence. But I agree that consistency is probably our best strategy
> > here, and I don't see any reason the same argument would not apply to
> > $SPATCH, or $CC for that matter.
> > 
> > (So I could see an argument for moving them all to "?=", but that can
> > create its own confusion as environment variables accidentally start
> > taking effect).
> 
> $STRIP and $SPATCH will work OK, but you would be disappointed
> with $CC (or any other variable from make's built-in database). ;-)

Oof. That's confusing. All the more reason not to go in the direction of
"?=". Thanks for an interesting tidbit.

-Peff

[PATCH v2 3/6] Makefile: add/remove comments at top and tweak whitespace

[Ævar Arnfjörð Bjarmason]

The top of the Makfile is mostly separated into logical steps like set
default configuration, set programs etc., but there's some deviation
from that.

Let's add mostly comments where they're missing, remove those that
don't add anything. The whitespace tweaking makes subsequent patches
smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index c53727e44b..0870fd4651 100644
--- a/Makefile
+++ b/Makefile
@@ -507,15 +507,14 @@ GIT-VERSION-FILE: FORCE
 -include GIT-VERSION-FILE
 
 # CFLAGS and LDFLAGS are for the users to override from the command line.
-
 CFLAGS = -g -O2 -Wall
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
-
-# Create as necessary, replace existing, make ranlib unneeded.
 ARFLAGS = rcs
 
+# Set our default configuration.
+#
 # Among the variables below, these:
 #   gitexecdir
 #   template_dir
@@ -560,6 +559,7 @@ perllibdir_relative = $(patsubst $(prefix)/%,%,$(perllibdir))
 
 export prefix bindir sharedir sysconfdir gitwebdir perllibdir localedir
 
+# Set our default programs
 CC = cc
 AR = ar
 RM = rm -f
@@ -587,10 +587,6 @@ SP_EXTRA_FLAGS =
 
 SPATCH_FLAGS = --all-includes --patch .
 
-
-
-### --- END CONFIGURATION SECTION ---
-
 BASIC_CFLAGS = -I.
 BASIC_LDFLAGS =
 
-- 
2.21.0.rc2.1.g2d5e20a900.dirty

[PATCH v2 4/6] Makefile: Move *_LIBS assignment into its own section

[Ævar Arnfjörð Bjarmason]

Now the only other non-program assignment in the previous list is
PTHREAD_CFLAGS, which'll be moved elsewhere in a follow-up chang.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 0870fd4651..59674ce9d7 100644
--- a/Makefile
+++ b/Makefile
@@ -572,7 +572,6 @@ TCLTK_PATH = wish
 XGETTEXT = xgettext
 MSGFMT = msgfmt
 CURL_CONFIG = curl-config
-PTHREAD_LIBS = -lpthread
 PTHREAD_CFLAGS =
 GCOV = gcov
 STRIP = strip
@@ -580,6 +579,9 @@ SPATCH = spatch
 
 export TCL_PATH TCLTK_PATH
 
+# Set our default LIBS variables
+PTHREAD_LIBS = -lpthread
+
 # user customisation variable for 'sparse' target
 SPARSE_FLAGS ?=
 # internal/platform customisation variable for 'sparse'
-- 
2.21.0.rc2.1.g2d5e20a900.dirty

Re: [PATCH v2 4/6] Makefile: Move *_LIBS assignment into its own section

[Jeff King]

On Fri, Feb 22, 2019 at 03:41:25PM +0100, Ævar Arnfjörð Bjarmason wrote:

> Now the only other non-program assignment in the previous list is
> PTHREAD_CFLAGS, which'll be moved elsewhere in a follow-up chang.

Hmm. I get why this organization might make sense. But it also might be
convenient to see all of the pthread-related flags/libs together.

I dunno. I guess we are not expecting people to touch these manually
anyway, so it may not matter.

-Peff

Re: [PATCH v2 4/6] Makefile: Move *_LIBS assignment into its own section

[Eric Sunshine]

On Fri, Feb 22, 2019 at 9:41 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Now the only other non-program assignment in the previous list is
> PTHREAD_CFLAGS, which'll be moved elsewhere in a follow-up chang.

s/chang/change/

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

[PATCH v2 5/6] Makefile: move the setting of *FLAGS closer to "include"

[Ævar Arnfjörð Bjarmason]

Move the setting of variables like CFLAGS down past settings like
"prefix" and default programs like "TAR" to just before we do the
include from "config.mak.*".

There's no functional changes here yet, but move note that
"ALL_CFLAGS" and "ALL_LDFLAGS" are moved below the include. A
follow-up change will tweak those depending on a variable set in
config.mak.dev.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/Makefile b/Makefile
index 59674ce9d7..82cfd6c2e4 100644
--- a/Makefile
+++ b/Makefile
@@ -506,13 +506,6 @@ GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
 
-# CFLAGS and LDFLAGS are for the users to override from the command line.
-CFLAGS = -g -O2 -Wall
-LDFLAGS =
-ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
-ALL_LDFLAGS = $(LDFLAGS)
-ARFLAGS = rcs
-
 # Set our default configuration.
 #
 # Among the variables below, these:
@@ -572,7 +565,6 @@ TCLTK_PATH = wish
 XGETTEXT = xgettext
 MSGFMT = msgfmt
 CURL_CONFIG = curl-config
-PTHREAD_CFLAGS =
 GCOV = gcov
 STRIP = strip
 SPATCH = spatch
@@ -582,16 +574,6 @@ export TCL_PATH TCLTK_PATH
 # Set our default LIBS variables
 PTHREAD_LIBS = -lpthread
 
-# user customisation variable for 'sparse' target
-SPARSE_FLAGS ?=
-# internal/platform customisation variable for 'sparse'
-SP_EXTRA_FLAGS =
-
-SPATCH_FLAGS = --all-includes --patch .
-
-BASIC_CFLAGS = -I.
-BASIC_LDFLAGS =
-
 # Guard against environment variables
 BUILTIN_OBJS =
 BUILT_INS =
@@ -1160,6 +1142,25 @@ ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/s
 DC_SHA1_SUBMODULE = auto
 endif
 
+# Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be
+# tweaked by config.* below as well as the command-line, both of
+# which'll override these defaults.
+CFLAGS = -g -O2 -Wall
+LDFLAGS =
+BASIC_CFLAGS = -I.
+BASIC_LDFLAGS =
+
+# library flags
+ARFLAGS = rcs
+PTHREAD_CFLAGS =
+
+# For the 'sparse' target
+SPARSE_FLAGS ?=
+SP_EXTRA_FLAGS =
+
+# For the 'coccicheck' target
+SPATCH_FLAGS = --all-includes --patch .
+
 include config.mak.uname
 -include config.mak.autogen
 -include config.mak
@@ -1168,6 +1169,9 @@ ifdef DEVELOPER
 include config.mak.dev
 endif
 
+ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
+ALL_LDFLAGS = $(LDFLAGS)
+
 comma := ,
 empty :=
 space := $(empty) $(empty)
-- 
2.21.0.rc2.1.g2d5e20a900.dirty

Re: [PATCH v2 5/6] Makefile: move the setting of *FLAGS closer to "include"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Move the setting of variables like CFLAGS down past settings like
> "prefix" and default programs like "TAR" to just before we do the
> include from "config.mak.*".
>
> There's no functional changes here yet, but move note that
> "ALL_CFLAGS" and "ALL_LDFLAGS" are moved below the include. A
> follow-up change will tweak those depending on a variable set in
> config.mak.dev.

Sorry, but cannot parse the sentence around "move note".

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Makefile | 40 ++++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 59674ce9d7..82cfd6c2e4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -506,13 +506,6 @@ GIT-VERSION-FILE: FORCE
>  	@$(SHELL_PATH) ./GIT-VERSION-GEN
>  -include GIT-VERSION-FILE
>  
> -# CFLAGS and LDFLAGS are for the users to override from the command line.
> -CFLAGS = -g -O2 -Wall
> -LDFLAGS =
> -ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
> -ALL_LDFLAGS = $(LDFLAGS)
> -ARFLAGS = rcs
> -
>  # Set our default configuration.
>  #
>  # Among the variables below, these:
> @@ -572,7 +565,6 @@ TCLTK_PATH = wish
>  XGETTEXT = xgettext
>  MSGFMT = msgfmt
>  CURL_CONFIG = curl-config
> -PTHREAD_CFLAGS =
>  GCOV = gcov
>  STRIP = strip
>  SPATCH = spatch
> @@ -582,16 +574,6 @@ export TCL_PATH TCLTK_PATH
>  # Set our default LIBS variables
>  PTHREAD_LIBS = -lpthread
>  
> -# user customisation variable for 'sparse' target
> -SPARSE_FLAGS ?=
> -# internal/platform customisation variable for 'sparse'
> -SP_EXTRA_FLAGS =
> -
> -SPATCH_FLAGS = --all-includes --patch .
> -
> -BASIC_CFLAGS = -I.
> -BASIC_LDFLAGS =
> -
>  # Guard against environment variables
>  BUILTIN_OBJS =
>  BUILT_INS =
> @@ -1160,6 +1142,25 @@ ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/s
>  DC_SHA1_SUBMODULE = auto
>  endif
>  
> +# Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be
> +# tweaked by config.* below as well as the command-line, both of
> +# which'll override these defaults.
> +CFLAGS = -g -O2 -Wall
> +LDFLAGS =
> +BASIC_CFLAGS = -I.
> +BASIC_LDFLAGS =
> +
> +# library flags
> +ARFLAGS = rcs
> +PTHREAD_CFLAGS =
> +
> +# For the 'sparse' target
> +SPARSE_FLAGS ?=

Makes us wonder why only this one is different and uses ?= ;-)

> +SP_EXTRA_FLAGS =
> +
> +# For the 'coccicheck' target
> +SPATCH_FLAGS = --all-includes --patch .
> +
>  include config.mak.uname
>  -include config.mak.autogen
>  -include config.mak
> @@ -1168,6 +1169,9 @@ ifdef DEVELOPER
>  include config.mak.dev
>  endif
>  
> +ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
> +ALL_LDFLAGS = $(LDFLAGS)
> +
>  comma := ,
>  empty :=
>  space := $(empty) $(empty)

[PATCH v2 6/6] Makefile: allow for combining DEVELOPER=1 and CFLAGS="..."

[Ævar Arnfjörð Bjarmason]

Ever since the DEVELOPER=1 facility introduced there's been no way to
have custom CFLAGS (e.g. CFLAGS="-O0 -g -ggdb3") while still
benefiting from the set of warnings and assertions DEVELOPER=1
enables.

This is because the semantics of variables in the Makefile are such
that the user setting CFLAGS overrides anything we set, including what
we're doing in config.mak.dev[1].

So let's introduce a "DEVELOPER_CFLAGS" variable in config.mak.dev and
add it to ALL_CFLAGS. Before this the ALL_CFLAGS variable
would (basically, there's some nuance we won't go into) be set to:

    $(CPPFLAGS) [$(CFLAGS) *or* $(CFLAGS) in config.mak.dev] $(BASIC_CFLAGS) $(EXTRA_CPPFLAGS)

But will now be:

    $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(BASIC_CFLAGS) $(EXTRA_CPPFLAGS)

The reason for putting DEVELOPER_CFLAGS first is to allow for
selectively overriding something DEVELOPER=1 brings in. On both GCC
and Clang later settings override earlier ones. E.g. "-Wextra
-Wno-extra" will enable no "extra" warnings, but not if those two
arguments are reversed.

Examples of things that weren't possible before, but are now:

    # Use -O0 instead of -O2 for less painful debuggng
    DEVELOPER=1 CFLAGS="-O0 -g"
    # DEVELOPER=1 plus -Wextra, but disable some of the warnings
    DEVELOPER=1 DEVOPTS="no-error extra-all" CFLAGS="-O0 -g -Wno-unused-parameter"

The reason for the patches leading up to this one re-arranged the
various *FLAGS assignments and includes is just for readability. The
Makefile supports assignments out of order, e.g.:

    $ cat Makefile
    X = $(A) $(B) $(C)
    A = A
    B = B
    include c.mak
    all:
            @echo $(X)
    $ cat c.mak
    C=C
    $ make
    A B C

So we could have gotten away with the much smaller change of changing
"CFLAGS" in config.mak.dev to "DEVELOPER_CFLAGS" and adding that to
ALL_CFLAGS earlier in the Makefile "before" the config.mak.*
includes. But I think it's more readable to use variables "after"
they're included.

1. https://www.gnu.org/software/make/manual/html_node/Overriding.html

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile       |  8 ++++++--
 config.mak.dev | 44 ++++++++++++++++++++++----------------------
 2 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/Makefile b/Makefile
index 82cfd6c2e4..710a3475a9 100644
--- a/Makefile
+++ b/Makefile
@@ -479,7 +479,11 @@ all::
 #
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and family are auto detected, but could be overridden by defining
-# COMPILER_FEATURES (see config.mak.dev)
+# COMPILER_FEATURES (see config.mak.dev). You can still set
+# CFLAGS="..." in combination with DEVELOPER enables, whether that's
+# for tweaking something unrelated (e.g. optimization level), or for
+# selectively overriding something DEVELOPER or one of the DEVOPTS
+# (see just below) brings in.
 #
 # When DEVELOPER is set, DEVOPTS can be used to control compiler
 # options.  This variable contains keywords separated by
@@ -1169,7 +1173,7 @@ ifdef DEVELOPER
 include config.mak.dev
 endif
 
-ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
 
 comma := ,
diff --git a/config.mak.dev b/config.mak.dev
index 7354fe15b3..bf1f3fcdee 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,41 +1,41 @@
 ifeq ($(filter no-error,$(DEVOPTS)),)
-CFLAGS += -Werror
+DEVELOPER_CFLAGS += -Werror
 endif
 ifneq ($(filter pedantic,$(DEVOPTS)),)
-CFLAGS += -pedantic
+DEVELOPER_CFLAGS += -pedantic
 # don't warn for each N_ use
-CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0
-endif
-CFLAGS += -Wall
-CFLAGS += -Wdeclaration-after-statement
-CFLAGS += -Wformat-security
-CFLAGS += -Wno-format-zero-length
-CFLAGS += -Wold-style-definition
-CFLAGS += -Woverflow
-CFLAGS += -Wpointer-arith
-CFLAGS += -Wstrict-prototypes
-CFLAGS += -Wunused
-CFLAGS += -Wvla
+DEVELOPER_CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0
+endif
+DEVELOPER_CFLAGS += -Wall
+DEVELOPER_CFLAGS += -Wdeclaration-after-statement
+DEVELOPER_CFLAGS += -Wformat-security
+DEVELOPER_CFLAGS += -Wno-format-zero-length
+DEVELOPER_CFLAGS += -Wold-style-definition
+DEVELOPER_CFLAGS += -Woverflow
+DEVELOPER_CFLAGS += -Wpointer-arith
+DEVELOPER_CFLAGS += -Wstrict-prototypes
+DEVELOPER_CFLAGS += -Wunused
+DEVELOPER_CFLAGS += -Wvla
 
 ifndef COMPILER_FEATURES
 COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
 endif
 
 ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
-CFLAGS += -Wtautological-constant-out-of-range-compare
+DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare
 endif
 
 ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
-CFLAGS += -Wextra
+DEVELOPER_CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
-CFLAGS += -Wmissing-prototypes
+DEVELOPER_CFLAGS += -Wmissing-prototypes
 ifeq ($(filter extra-all,$(DEVOPTS)),)
 # These are disabled because we have these all over the place.
-CFLAGS += -Wno-empty-body
-CFLAGS += -Wno-missing-field-initializers
-CFLAGS += -Wno-sign-compare
-CFLAGS += -Wno-unused-parameter
+DEVELOPER_CFLAGS += -Wno-empty-body
+DEVELOPER_CFLAGS += -Wno-missing-field-initializers
+DEVELOPER_CFLAGS += -Wno-sign-compare
+DEVELOPER_CFLAGS += -Wno-unused-parameter
 endif
 endif
 
@@ -43,6 +43,6 @@ endif
 # not worth fixing since newer compilers correctly stop complaining
 ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
 ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
-CFLAGS += -Wno-uninitialized
+DEVELOPER_CFLAGS += -Wno-uninitialized
 endif
 endif
-- 
2.21.0.rc2.1.g2d5e20a900.dirty

Re: [PATCH v2 6/6] Makefile: allow for combining DEVELOPER=1 and CFLAGS="..."

[Jeff King]

On Fri, Feb 22, 2019 at 03:41:27PM +0100, Ævar Arnfjörð Bjarmason wrote:

> So let's introduce a "DEVELOPER_CFLAGS" variable in config.mak.dev and
> add it to ALL_CFLAGS. Before this the ALL_CFLAGS variable
> would (basically, there's some nuance we won't go into) be set to:

Yeah, I definitely think this is the right direction, and the patch
looks good to me.

> Examples of things that weren't possible before, but are now:
> 
>     # Use -O0 instead of -O2 for less painful debuggng
>     DEVELOPER=1 CFLAGS="-O0 -g"

Sort an aside, but in my config.mak I have:

  O=0
  CFLAGS = -g -O$(O)

which lets me do "make O=2", which is nice and short. I wonder if other
people would want that, too.

(I default to -O0 because 99% of my compiles are for development and
debugging, so the compile being fast is more important than the build
result being fast; for perf testing or daily use, I use O=2).

> So we could have gotten away with the much smaller change of changing
> "CFLAGS" in config.mak.dev to "DEVELOPER_CFLAGS" and adding that to
> ALL_CFLAGS earlier in the Makefile "before" the config.mak.*
> includes. But I think it's more readable to use variables "after"
> they're included.

I think the reordering does make things more readable overall. And it
may be more sensible if config.mak uses any ":=" assignments.

-Peff

Re: [PATCH v2 6/6] Makefile: allow for combining DEVELOPER=1 and CFLAGS="..."

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/config.mak.dev b/config.mak.dev
> index 7354fe15b3..bf1f3fcdee 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -1,41 +1,41 @@
>  ifeq ($(filter no-error,$(DEVOPTS)),)
> -CFLAGS += -Werror
> +DEVELOPER_CFLAGS += -Werror

This is the first mention of DEVELOPER_CFLAGS; what is -Werror
appended to?  Any random thing in the environment or command line?

>  endif

Re: [PATCH] Makefile: remove an out-of-date comment

[Jeff King]

On Fri, Feb 22, 2019 at 11:56:58AM +0100, Ævar Arnfjörð Bjarmason wrote:

> Remove a comment referring to a caveat that hasn't been applicable
> since 18b0fc1ce1 ("Git.pm: Kill Git.xs for now", 2006-09-23).
> 
> At the time of 8d7f586f13 ("Git.pm: Support for perl/ being built by a
> different compiler", 2006-06-25) some of the code in perl would be
> built by a C compiler, but support for that went away a few months
> later in 18b0fc1ce1 discussed above.
> 
> Since my 20d2a30f8f ("Makefile: replace perl/Makefile.PL with simple
> make rules", 2017-12-10) the perl/ directory doesn't even have its own
> build process.

I liked that commit before, but seeing that it makes this hairy corner
case go away, I like it even more. ;)

The patch itself seems obviously correct.

-Peff