[PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Ævar Arnfjörð Bjarmason]

The "COMPUTE_HEADER_DEPENDENCIES" feature added in [1] was extended to
use auto-detection in [2]. Then when -Wpedantic support was added to
DEVOPTS in [3] we started passing -Wpedantic in combination with
-Werror to the compiler here.

This broke the auto-detection, but since we'd quieted it in [4] we
didn't find out. It was emitting all of this on STDERR under GCC:

    /dev/null:1: error: ISO C forbids an empty translation unit
    [-Werror=pedantic]
    cc1: note: unrecognized command-line option
    ‘-Wno-pedantic-ms-format’ may have been intended to silence
    earlier diagnostics
    cc1: all warnings being treated as errors

Let's fix that bug by maintaining a NON_DEVELOPER_CFLAGS, it's like
ALL_CFLAGS but without anything we add in config.mak.dev, and
furthermore stop redirecting STDERR to /dev/null, this means that
someone whose compiler doesn't support this will see this output, but
also this new message:

    Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect

It's also possible that some compilers will emit warnings but still
give a zero exit code, anyone using a compiler like that will
potentially get more verbose output from the Makefile until they set
COMPUTE_HEADER_DEPENDENCIES=no. E.g. on AIX's xlc we'll now emit:

    /opt/IBM/xlc/13.1.3/bin/.orig/xlc: 1501-208 (S) command option D is missing a subargument
    Non-zero 40 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect

And on Solaris with SunCC:

    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise
    cc: refused to overwrite input file by output file: /dev/null
    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise
    cc: refused to overwrite input file by output file: /dev/null
    Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect

Both are quieted by setting COMPUTE_HEADER_DEPENDENCIES=no as
suggested.

I considered piping the output and the exit code to a variable
instead, but e.g. under GCC that would lose the coloring in the error
messages.

1. f2fabbf76e4 (Teach Makefile to check header dependencies,
   2010-01-26)
2. 111ee18c31f (Makefile: Use computed header dependencies if the
   compiler supports it, 2011-08-18)
3. 729b3925ed9 (Makefile: add a DEVOPTS flag to get pedantic
   compilation, 2018-07-24)
4. 6a8cbc41bac (developer: enable pedantic by default, 2021-09-03)

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

diff --git a/Makefile b/Makefile
index 9df565f27bb..c30f3b8b204 100644
--- a/Makefile
+++ b/Makefile
@@ -1242,7 +1242,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
 ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)
 endif
 
-ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)
+NON_DEVELOPER_CFLAGS = $(CPPFLAGS) $(CFLAGS)
+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(NON_DEVELOPER_CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
 
 comma := ,
@@ -1277,13 +1278,14 @@ COMPUTE_HEADER_DEPENDENCIES = auto
 endif
 
 ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)
-dep_check = $(shell $(CC) $(ALL_CFLAGS) \
+dep_check = $(shell $(CC) $(NON_DEVELOPER_CFLAGS) \
 	-c -MF /dev/null -MQ /dev/null -MMD -MP \
-	-x c /dev/null -o /dev/null 2>&1; \
+	-x c /dev/null -o /dev/null; \
 	echo $$?)
 ifeq ($(dep_check),0)
 override COMPUTE_HEADER_DEPENDENCIES = yes
 else
+$(info Non-zero $(dep_check) exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect)
 override COMPUTE_HEADER_DEPENDENCIES = no
 endif
 endif
-- 
2.33.0.1225.g17f21f53d74

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Carlo Arenas]

On Wed, Sep 22, 2021 at 3:38 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> The "COMPUTE_HEADER_DEPENDENCIES" feature added in [1] was extended to
> use auto-detection in [2]. Then when -Wpedantic support was added to
> DEVOPTS in [3] we started passing -Wpedantic in combination with
> -Werror to the compiler here.

It is my impression though that header dependencies computation is
something that might only work in gcc and clang (because of its gcc
compatibility), so shouldn't this be restricted to those compilers
instead of forcing all others to error?

Also, why is this process run with DEVELOPER=1 to begin with, if we
obviously don't need/want any compilations warnings?

Carlo

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 22 2021, Carlo Arenas wrote:

> On Wed, Sep 22, 2021 at 3:38 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> The "COMPUTE_HEADER_DEPENDENCIES" feature added in [1] was extended to
>> use auto-detection in [2]. Then when -Wpedantic support was added to
>> DEVOPTS in [3] we started passing -Wpedantic in combination with
>> -Werror to the compiler here.
>
> It is my impression though that header dependencies computation is
> something that might only work in gcc and clang (because of its gcc
> compatibility), so shouldn't this be restricted to those compilers
> instead of forcing all others to error?

I think it's better to just have those not supporting
COMPUTE_HEADER_DEPENDENCIES define that, wanting to auto-detect things
is what led to the current breakage.

I.e. our whitelisting of GCC and Clang will be out of date if some other
compiler grows support for this, and even if we whitelist gcc and clang
we'll need some script like detect-compiler etc.

> Also, why is this process run with DEVELOPER=1 to begin with, if we
> obviously don't need/want any compilations warnings?

Hrm? We run the COMPUTE_HEADER_DEPENDENCIES=yes process with and without
DEVELOPER=1, it's just that DEVELOPER=1 breaks it since it turns on
pedantic compilation flags now.

We want -Werror for the actual compilation under DEVELOPER=1, but a
one-off command to see if a compiler has support for something is
entirely different.

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Junio C Hamano]

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

> The "COMPUTE_HEADER_DEPENDENCIES" feature added in [1] was extended to
> use auto-detection in [2]. Then when -Wpedantic support was added to
> DEVOPTS in [3] we started passing -Wpedantic in combination with
> -Werror to the compiler here.
>
> This broke the auto-detection, but since we'd quieted it in [4] we
> didn't find out.

Are the references correct?  I am not seeing "quiet"ing in [4].  The
redirection 2>&1 to cram error messages also to $(dep_check), hence
making it impossible to match '0', was done in [2].

We did make the pedantic mode the default and pass both -pedantic
and -Wpedantic after [4].  Before we had only -pedantic.

> It was emitting all of this on STDERR under GCC:
>
>     /dev/null:1: error: ISO C forbids an empty translation unit
>     [-Werror=pedantic]
>     cc1: note: unrecognized command-line option
>     ‘-Wno-pedantic-ms-format’ may have been intended to silence
>     earlier diagnostics
>     cc1: all warnings being treated as errors
>
> Let's fix that bug by maintaining a NON_DEVELOPER_CFLAGS, it's like
> ALL_CFLAGS but without anything we add in config.mak.dev, and
> furthermore stop redirecting STDERR to /dev/null, this means that
> someone whose compiler doesn't support this will see this output, but
> also this new message:
>
>     Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect


Hmmmmmph.  

I recentaly saw many .depend directories (not necessarily empty)
left after "make distclean". After building on one branch, I often
check out a different branch then run distclean on the new branch,
so leftover build artifacts are not necessarily a bug in our
Makefile, but the bug you found may explain it?

While I agree with your analysis of the problem, I cannot shake this
nagging feeling that the proposed solution is barking up a wrong
tree.  After all, -pedantic and any other option that lets the
compiler notice that it is being asked to compile an empty source
can come directly from the end user (e.g. CC="gcc -pedantic" or as
part of CFLAGS)---realization of which makes me wonder if it is
essential to compile /dev/null for this check, or any reasonably
syntactically correct program would do.

I wonder if the attached (with clean-up to remove the tracing cruft)
would show us a better direction.  It feeds a single line

	int dummy_for_dep_check;

C "program" from the standard input of the compiler to tackle the
"you are not supposed to be compiling an empty compilation unit"
problem in a more direct way.

 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git c/Makefile w/Makefile
index 9df565f27b..0593ab7287 100644
--- c/Makefile
+++ w/Makefile
@@ -1277,9 +1277,9 @@ COMPUTE_HEADER_DEPENDENCIES = auto
 endif
 
 ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)
-dep_check = $(shell $(CC) $(ALL_CFLAGS) \
+dep_check = $(shell echo >&2 doing dep check; echo int dummy_for_dep_check\; | $(CC) $(ALL_CFLAGS) \
 	-c -MF /dev/null -MQ /dev/null -MMD -MP \
-	-x c /dev/null -o /dev/null 2>&1; \
+	-x c - -o /dev/null || echo >&2 oops; \
 	echo $$?)
 ifeq ($(dep_check),0)
 override COMPUTE_HEADER_DEPENDENCIES = yes

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Jeff King]

On Wed, Sep 22, 2021 at 09:08:00AM -0700, Junio C Hamano wrote:

> While I agree with your analysis of the problem, I cannot shake this
> nagging feeling that the proposed solution is barking up a wrong
> tree.  After all, -pedantic and any other option that lets the
> compiler notice that it is being asked to compile an empty source
> can come directly from the end user (e.g. CC="gcc -pedantic" or as
> part of CFLAGS)---realization of which makes me wonder if it is
> essential to compile /dev/null for this check, or any reasonably
> syntactically correct program would do.
> 
> I wonder if the attached (with clean-up to remove the tracing cruft)
> would show us a better direction.  It feeds a single line
> 
> 	int dummy_for_dep_check;
> 
> C "program" from the standard input of the compiler to tackle the
> "you are not supposed to be compiling an empty compilation unit"
> problem in a more direct way.

That feels a bit like we're playing a game of chicken with the compiler
in terms of what it may complain about. For example, sparse will
complain:

  foo.c:1:5: warning: symbol 'dummy_for_dep_check' was not declared. Should it be static?

Might compilers ever learn to warn of the same thing?

I kind of like the simplicity of Ævar's approach. We want to know if the
compiler can be invoked with options XYZ, so we do so. That should be
largely independent of our cflags, and there's prior art in how we
invoke it in the detect-compiler script.

So I'd argue we should go even simpler, like:

diff --git a/Makefile b/Makefile
index 3628d14f16..4597a126d0 100644
--- a/Makefile
+++ b/Makefile
@@ -1277,7 +1277,7 @@ COMPUTE_HEADER_DEPENDENCIES = auto
 endif
 
 ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)
-dep_check = $(shell $(CC) $(ALL_CFLAGS) \
+dep_check = $(shell $(CC) \
 	-c -MF /dev/null -MQ /dev/null -MMD -MP \
 	-x c /dev/null -o /dev/null 2>&1; \
 	echo $$?)

I'm also tempted by a hunk like this. Then we can set the REQUIRE flag
in a CI job (or locally for git devs who know they have gcc) and notice
an unexpected breakage in the auto test.

@@ -1295,6 +1295,9 @@ ifneq ($(COMPUTE_HEADER_DEPENDENCIES),no)
 $(error please set COMPUTE_HEADER_DEPENDENCIES to yes, no, or auto \
 (not "$(COMPUTE_HEADER_DEPENDENCIES)"))
 endif
+ifdef REQUIRE_COMPUTE_HEADER_DEPENDENCIES
+$(error computed header dependencies required, but auto-check did not find them)
+endif
 endif
 
 ifndef GENERATE_COMPILATION_DATABASE

-Peff

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I wonder if the attached (with clean-up to remove the tracing cruft)
>> would show us a better direction.  It feeds a single line
>> 
>> 	int dummy_for_dep_check;
>> 
>> C "program" from the standard input of the compiler to tackle the
>> "you are not supposed to be compiling an empty compilation unit"
>> problem in a more direct way.
>
> That feels a bit like we're playing a game of chicken with the compiler
> in terms of what it may complain about. For example, sparse will
> complain:
>
>   foo.c:1:5: warning: symbol 'dummy_for_dep_check' was not declared. Should it be static?
>
> Might compilers ever learn to warn of the same thing?

Certainly.  That is the reason why I said "direction", not
"solution", and I do not think it is beyond our capability to come
up with a minimal "C program" that would be lint clean to make it
as a part of the "solution".

For example, would sparse or compilers complain about this?

    extern int incr(int); int incr(int i) { return i + 1; }

> So I'd argue we should go even simpler, like:
>
> diff --git a/Makefile b/Makefile
> index 3628d14f16..4597a126d0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1277,7 +1277,7 @@ COMPUTE_HEADER_DEPENDENCIES = auto
>  endif
>  
>  ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)
> -dep_check = $(shell $(CC) $(ALL_CFLAGS) \
> +dep_check = $(shell $(CC) \
>  	-c -MF /dev/null -MQ /dev/null -MMD -MP \
>  	-x c /dev/null -o /dev/null 2>&1; \
>  	echo $$?)

I am all for this simplification.  It takes us back to the state
before 1816bf26 (Makefile: Improve compiler header dependency check,
2011-08-30).  But I think that is more or less orthogonal to the
"you are not supposed to feed an empty compilation unit" issue.

> I'm also tempted by a hunk like this. Then we can set the REQUIRE flag
> in a CI job (or locally for git devs who know they have gcc) and notice
> an unexpected breakage in the auto test.
>
> @@ -1295,6 +1295,9 @@ ifneq ($(COMPUTE_HEADER_DEPENDENCIES),no)
>  $(error please set COMPUTE_HEADER_DEPENDENCIES to yes, no, or auto \
>  (not "$(COMPUTE_HEADER_DEPENDENCIES)"))
>  endif
> +ifdef REQUIRE_COMPUTE_HEADER_DEPENDENCIES
> +$(error computed header dependencies required, but auto-check did not find them)
> +endif
>  endif

Yup, I like that, too.

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Carlo Arenas]

On Wed, Sep 22, 2021 at 11:28 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> >> I wonder if the attached (with clean-up to remove the tracing cruft)
> >> would show us a better direction.  It feeds a single line
> >>
> >>      int dummy_for_dep_check;
> >>
> >> C "program" from the standard input of the compiler to tackle the
> >> "you are not supposed to be compiling an empty compilation unit"
> >> problem in a more direct way.
> >
> > That feels a bit like we're playing a game of chicken with the compiler
> > in terms of what it may complain about. For example, sparse will
> > complain:
> >
> >   foo.c:1:5: warning: symbol 'dummy_for_dep_check' was not declared. Should it be static?
> >
> > Might compilers ever learn to warn of the same thing?
>
> Certainly.  That is the reason why I said "direction", not
> "solution", and I do not think it is beyond our capability to come
> up with a minimal "C program" that would be lint clean to make it
> as a part of the "solution".
>
> For example, would sparse or compilers complain about this?
>
>     extern int incr(int); int incr(int i) { return i + 1; }
>
> > So I'd argue we should go even simpler, like:
> >
> > diff --git a/Makefile b/Makefile
> > index 3628d14f16..4597a126d0 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1277,7 +1277,7 @@ COMPUTE_HEADER_DEPENDENCIES = auto
> >  endif
> >
> >  ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)
> > -dep_check = $(shell $(CC) $(ALL_CFLAGS) \
> > +dep_check = $(shell $(CC) \
> >       -c -MF /dev/null -MQ /dev/null -MMD -MP \
> >       -x c /dev/null -o /dev/null 2>&1; \
> >       echo $$?)
>
> I am all for this simplification.  It takes us back to the state
> before 1816bf26 (Makefile: Improve compiler header dependency check,
> 2011-08-30).  But I think that is more or less orthogonal to the
> "you are not supposed to feed an empty compilation unit" issue.

the problem really is IMHO, that we are passing around CFLAGS that
indicate 2 things:

1) attributes that are relevant to how we build and what
2) which diagnostics we want to use

Ævar's approach addresses implicitly part of it (most of the
diagnostics are added in config.mak.dev), but I think it still feels
like a knee jerk reaction to the problem, while creating an
maintaining something like "DIAGNOSTIC_FLAGS" which we will only pass
to the compiler when we want to use diagnostic (including -pedantic,
-Wpedantic and -Werror) would be a better approach.

Carlo

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 22 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> The "COMPUTE_HEADER_DEPENDENCIES" feature added in [1] was extended to
>> use auto-detection in [2]. Then when -Wpedantic support was added to
>> DEVOPTS in [3] we started passing -Wpedantic in combination with
>> -Werror to the compiler here.
>>
>> This broke the auto-detection, but since we'd quieted it in [4] we
>> didn't find out.
>
> Are the references correct?  I am not seeing "quiet"ing in [4].  The
> redirection 2>&1 to cram error messages also to $(dep_check), hence
> making it impossible to match '0', was done in [2].

Yes it's incorrect, I meant [2]. I had this right in my head, just got
the references wrong somehow,thanks.

> We did make the pedantic mode the default and pass both -pedantic
> and -Wpedantic after [4].  Before we had only -pedantic.

*nod*

>> It was emitting all of this on STDERR under GCC:
>>
>>     /dev/null:1: error: ISO C forbids an empty translation unit
>>     [-Werror=pedantic]
>>     cc1: note: unrecognized command-line option
>>     ‘-Wno-pedantic-ms-format’ may have been intended to silence
>>     earlier diagnostics
>>     cc1: all warnings being treated as errors
>>
>> Let's fix that bug by maintaining a NON_DEVELOPER_CFLAGS, it's like
>> ALL_CFLAGS but without anything we add in config.mak.dev, and
>> furthermore stop redirecting STDERR to /dev/null, this means that
>> someone whose compiler doesn't support this will see this output, but
>> also this new message:
>>
>>     Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect
>
>
> Hmmmmmph.  
>
> I recentaly saw many .depend directories (not necessarily empty)
> left after "make distclean". After building on one branch, I often
> check out a different branch then run distclean on the new branch,
> so leftover build artifacts are not necessarily a bug in our
> Makefile, but the bug you found may explain it?

Yes, I'll update the commit message, the problem is that we'll empty the
dep_dirs list if we're not *currently* making them, that's a logic error
in a few places in the Makefile, i.e. conflating currently building X
with wanting to clean X.

[Will respond to the rest with a re-roll and/or in other replies in-thread]

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Jeff King]

On Wed, Sep 22, 2021 at 11:28:38AM -0700, Junio C Hamano wrote:

> > That feels a bit like we're playing a game of chicken with the compiler
> > in terms of what it may complain about. For example, sparse will
> > complain:
> >
> >   foo.c:1:5: warning: symbol 'dummy_for_dep_check' was not declared. Should it be static?
> >
> > Might compilers ever learn to warn of the same thing?
> 
> Certainly.  That is the reason why I said "direction", not
> "solution", and I do not think it is beyond our capability to come
> up with a minimal "C program" that would be lint clean to make it
> as a part of the "solution".
> 
> For example, would sparse or compilers complain about this?
> 
>     extern int incr(int); int incr(int i) { return i + 1; }

Fair enough. It is a game of chicken, but it is one that is easy to win. :)

I almost suggested using "git.c" as the dummy file, since we know it
must compile anyway. But that probably has other problems (it's more
expensive, and if it _does_ have an error, the results may be
confusing).

It's a shame we can't just try to do the _real_ compiles using the
auto-dependency stuff, and then fall back if they fail. But I think
there's a chicken-and-egg problem there with "make" doing real work, and
figuring out the dependencies to do real work.

> >  ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)
> > -dep_check = $(shell $(CC) $(ALL_CFLAGS) \
> > +dep_check = $(shell $(CC) \
> >  	-c -MF /dev/null -MQ /dev/null -MMD -MP \
> >  	-x c /dev/null -o /dev/null 2>&1; \
> >  	echo $$?)
> 
> I am all for this simplification.  It takes us back to the state
> before 1816bf26 (Makefile: Improve compiler header dependency check,
> 2011-08-30).  But I think that is more or less orthogonal to the
> "you are not supposed to feed an empty compilation unit" issue.

Hmm, my suggestion was off the cuff without digging to see whether we
used to do something similar. ;)

I do worry a bit that we'd be regressing the case that commit tried to
fix. OTOH, I'm not sure I understand its commit message. It talks about
things in CFLAGS being a problem, but it looks like the original (and my
proposal here) would not look at CFLAGS at all? If people are putting
stuff into CC that will break when used without CFLAGS, then I feel like
the answer might be "don't do that". Or are there common situations
where $(CC) is not expected to behave sensibly on its own?

> > I'm also tempted by a hunk like this. Then we can set the REQUIRE flag
> > in a CI job (or locally for git devs who know they have gcc) and notice
> > an unexpected breakage in the auto test.
> >
> > @@ -1295,6 +1295,9 @@ ifneq ($(COMPUTE_HEADER_DEPENDENCIES),no)
> >  $(error please set COMPUTE_HEADER_DEPENDENCIES to yes, no, or auto \
> >  (not "$(COMPUTE_HEADER_DEPENDENCIES)"))
> >  endif
> > +ifdef REQUIRE_COMPUTE_HEADER_DEPENDENCIES
> > +$(error computed header dependencies required, but auto-check did not find them)
> > +endif
> >  endif
> 
> Yup, I like that, too.

I'm happy to submit that on top, or even turn the earlier hunk into a
patch.  But let's see what Ævar has to say to what's been discussed so
far. I don't want to derail his effort.

-Peff

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Carlo Arenas]

On Wed, Sep 22, 2021 at 1:17 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, Sep 22, 2021 at 11:28:38AM -0700, Junio C Hamano wrote:
>
> > >  ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)
> > > -dep_check = $(shell $(CC) $(ALL_CFLAGS) \
> > > +dep_check = $(shell $(CC) \
> > >     -c -MF /dev/null -MQ /dev/null -MMD -MP \
> > >     -x c /dev/null -o /dev/null 2>&1; \
> > >     echo $$?)
> >
> > I am all for this simplification.  It takes us back to the state
> > before 1816bf26 (Makefile: Improve compiler header dependency check,
> > 2011-08-30).  But I think that is more or less orthogonal to the
> > "you are not supposed to feed an empty compilation unit" issue.
>
> Hmm, my suggestion was off the cuff without digging to see whether we
> used to do something similar. ;)
>
> I do worry a bit that we'd be regressing the case that commit tried to
> fix. OTOH, I'm not sure I understand its commit message. It talks about
> things in CFLAGS being a problem, but it looks like the original (and my
> proposal here) would not look at CFLAGS at all? If people are putting
> stuff into CC that will break when used without CFLAGS, then I feel like
> the answer might be "don't do that". Or are there common situations
> where $(CC) is not expected to behave sensibly on its own?

Yes, the problem was ONLY with the gcc that came in macOS before they
moved to clang and that had a frankenstein set of options which will
prevent people building BOTH fat binaries and doing this header
dependency computation at the same time.

Ironically, modern clang makes his use case even uglier (running on
top of Ævar's fix) :

$ make CFLAGS="-arch x86_64 -arch arm64"
fatal error: /Library/Developer/CommandLineTools/usr/bin/lipo: can't
create temporary output file: /dev/null.lipo (Operation not permitted)
clang: error: lipo command failed with exit code 1 (use -v to see invocation)
fatal error: /Library/Developer/CommandLineTools/usr/bin/lipo: can't
create temporary output file: /dev/null.lipo (Operation not permitted)
clang: error: lipo command failed with exit code 1 (use -v to see invocation)
Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" t

so the test fails, and COMPUTE_HEADER_DEPENDENCIES is disabled, but it
shouldn't and works fine if the test is overridden with
COMPUTE_HEADER_DEPENDENCIES=yes

so by removing the CFLAGS from that test, we will actually be fixing
this use case as well IMHO.

Carlo

[PATCH v2 0/2] Makefile: "pedantic" fallout on .depend and "compdb"

[Ævar Arnfjörð Bjarmason]

In this v2 I just added an unconditional -Wno-pedantic and omitted the
change to spew error on STDERR. This more narrowly fixes the immediate
issue and doesn't get into whether we should use /dev/null or whatever
as input.

I then noticed that the same bug was present in
"GENERATE_COMPILATION_DATABASE=yes", so there's now a 2nd patch to fix
that.

Ævar Arnfjörð Bjarmason (2):
  Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with
    DEVOPTS=pedantic
  Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes

 Makefile | 2 ++
 1 file changed, 2 insertions(+)

Range-diff against v1:
1:  3ff8ea12bf3 ! 1:  31c871e9bf6 Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic
    @@ Commit message
         Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic
     
         The "COMPUTE_HEADER_DEPENDENCIES" feature added in [1] was extended to
    -    use auto-detection in [2]. Then when -Wpedantic support was added to
    -    DEVOPTS in [3] we started passing -Wpedantic in combination with
    -    -Werror to the compiler here.
    +    use auto-detection in [2], that "auto" detection has always piped
    +    STDERR to /dev/null, so any failures on compilers that didn't support
    +    these GCC flags would silently fall back to
    +    "COMPUTE_HEADER_DEPENDENCIES=no".
     
    -    This broke the auto-detection, but since we'd quieted it in [4] we
    -    didn't find out. It was emitting all of this on STDERR under GCC:
    +    Later when -Wpedantic support was added to DEVOPTS in [3] we started
    +    passing -Wpedantic in combination with -Werror to the compiler
    +    here. Note (to the pedantic): [3] actually passed "-pedantic", but it
    +    and "-Wpedantic" are synonyms.
    +
    +    Turning on -Wpedantic in [3] broke the auto-detection, since this
    +    relies on compiling an empty program. GCC would loudly complain on
    +    STDERR:
     
             /dev/null:1: error: ISO C forbids an empty translation unit
             [-Werror=pedantic]
    @@ Commit message
             earlier diagnostics
             cc1: all warnings being treated as errors
     
    -    Let's fix that bug by maintaining a NON_DEVELOPER_CFLAGS, it's like
    -    ALL_CFLAGS but without anything we add in config.mak.dev, and
    -    furthermore stop redirecting STDERR to /dev/null, this means that
    -    someone whose compiler doesn't support this will see this output, but
    -    also this new message:
    +    But as that ended up in the "$(dep_check)" variable due to the "2>&1"
    +    in [2] we didn't see it.
     
    -        Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect
    +    Then when [4] made DEVOPTS=pedantic the default specifying
    +    "DEVELOPER=1" would effectively set "COMPUTE_HEADER_DEPENDENCIES=no".
    +
    +    To fix these issues let's unconditionally pass -Wno-pedantic after
    +    $(ALL_CFLAGS), we might get a -Wpedantic via config.mak.dev after, or
    +    the builder might specify it via CFLAGS. In either case this will undo
    +    current and future problems with -Wpedantic.
     
    -    It's also possible that some compilers will emit warnings but still
    -    give a zero exit code, anyone using a compiler like that will
    -    potentially get more verbose output from the Makefile until they set
    -    COMPUTE_HEADER_DEPENDENCIES=no. E.g. on AIX's xlc we'll now emit:
    +    I think it would make sense to simply remove the "2>&1", it would mean
    +    that anyone using a non-GCC-like compiler would get warnings under
    +    COMPUTE_HEADER_DEPENDENCIES=auto, e.g on AIX's xlc would emit:
     
             /opt/IBM/xlc/13.1.3/bin/.orig/xlc: 1501-208 (S) command option D is missing a subargument
             Non-zero 40 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect
    @@ Commit message
             cc: refused to overwrite input file by output file: /dev/null
             Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect
     
    -    Both are quieted by setting COMPUTE_HEADER_DEPENDENCIES=no as
    -    suggested.
    -
    -    I considered piping the output and the exit code to a variable
    -    instead, but e.g. under GCC that would lose the coloring in the error
    -    messages.
    +    Both could be quieted by setting COMPUTE_HEADER_DEPENDENCIES=no
    +    explicitly, as suggested, but let's see if this'll fix it without
    +    emitting too much noise at those that aren't using "gcc" or "clang".
     
         1. f2fabbf76e4 (Teach Makefile to check header dependencies,
            2010-01-26)
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Makefile ##
    -@@ Makefile: ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
    - ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)
    - endif
    - 
    --ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)
    -+NON_DEVELOPER_CFLAGS = $(CPPFLAGS) $(CFLAGS)
    -+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(NON_DEVELOPER_CFLAGS)
    - ALL_LDFLAGS = $(LDFLAGS)
    - 
    - comma := ,
    -@@ Makefile: COMPUTE_HEADER_DEPENDENCIES = auto
    - endif
    +@@ Makefile: endif
      
      ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)
    --dep_check = $(shell $(CC) $(ALL_CFLAGS) \
    -+dep_check = $(shell $(CC) $(NON_DEVELOPER_CFLAGS) \
    + dep_check = $(shell $(CC) $(ALL_CFLAGS) \
    ++	-Wno-pedantic \
      	-c -MF /dev/null -MQ /dev/null -MMD -MP \
    --	-x c /dev/null -o /dev/null 2>&1; \
    -+	-x c /dev/null -o /dev/null; \
    + 	-x c /dev/null -o /dev/null 2>&1; \
      	echo $$?)
    - ifeq ($(dep_check),0)
    - override COMPUTE_HEADER_DEPENDENCIES = yes
    - else
    -+$(info Non-zero $(dep_check) exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect)
    - override COMPUTE_HEADER_DEPENDENCIES = no
    - endif
    - endif
-:  ----------- > 2:  6b18bd08894 Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes
-- 
2.33.0.1225.g9f062250122

[PATCH v2 1/2] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Ævar Arnfjörð Bjarmason]

The "COMPUTE_HEADER_DEPENDENCIES" feature added in [1] was extended to
use auto-detection in [2], that "auto" detection has always piped
STDERR to /dev/null, so any failures on compilers that didn't support
these GCC flags would silently fall back to
"COMPUTE_HEADER_DEPENDENCIES=no".

Later when -Wpedantic support was added to DEVOPTS in [3] we started
passing -Wpedantic in combination with -Werror to the compiler
here. Note (to the pedantic): [3] actually passed "-pedantic", but it
and "-Wpedantic" are synonyms.

Turning on -Wpedantic in [3] broke the auto-detection, since this
relies on compiling an empty program. GCC would loudly complain on
STDERR:

    /dev/null:1: error: ISO C forbids an empty translation unit
    [-Werror=pedantic]
    cc1: note: unrecognized command-line option
    ‘-Wno-pedantic-ms-format’ may have been intended to silence
    earlier diagnostics
    cc1: all warnings being treated as errors

But as that ended up in the "$(dep_check)" variable due to the "2>&1"
in [2] we didn't see it.

Then when [4] made DEVOPTS=pedantic the default specifying
"DEVELOPER=1" would effectively set "COMPUTE_HEADER_DEPENDENCIES=no".

To fix these issues let's unconditionally pass -Wno-pedantic after
$(ALL_CFLAGS), we might get a -Wpedantic via config.mak.dev after, or
the builder might specify it via CFLAGS. In either case this will undo
current and future problems with -Wpedantic.

I think it would make sense to simply remove the "2>&1", it would mean
that anyone using a non-GCC-like compiler would get warnings under
COMPUTE_HEADER_DEPENDENCIES=auto, e.g on AIX's xlc would emit:

    /opt/IBM/xlc/13.1.3/bin/.orig/xlc: 1501-208 (S) command option D is missing a subargument
    Non-zero 40 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect

And on Solaris with SunCC:

    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise
    cc: refused to overwrite input file by output file: /dev/null
    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise
    cc: refused to overwrite input file by output file: /dev/null
    Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect

Both could be quieted by setting COMPUTE_HEADER_DEPENDENCIES=no
explicitly, as suggested, but let's see if this'll fix it without
emitting too much noise at those that aren't using "gcc" or "clang".

1. f2fabbf76e4 (Teach Makefile to check header dependencies,
   2010-01-26)
2. 111ee18c31f (Makefile: Use computed header dependencies if the
   compiler supports it, 2011-08-18)
3. 729b3925ed9 (Makefile: add a DEVOPTS flag to get pedantic
   compilation, 2018-07-24)
4. 6a8cbc41bac (developer: enable pedantic by default, 2021-09-03)

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

diff --git a/Makefile b/Makefile
index 9df565f27bb..10ea12aae21 100644
--- a/Makefile
+++ b/Makefile
@@ -1278,6 +1278,7 @@ endif
 
 ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)
 dep_check = $(shell $(CC) $(ALL_CFLAGS) \
+	-Wno-pedantic \
 	-c -MF /dev/null -MQ /dev/null -MMD -MP \
 	-x c /dev/null -o /dev/null 2>&1; \
 	echo $$?)
-- 
2.33.0.1225.g9f062250122

[PATCH v2 2/2] Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes

[Ævar Arnfjörð Bjarmason]

The same bug fixed in the "COMPUTE_HEADER_DEPENDENCIES=auto" mode in
the preceding commit was also present with
"GENERATE_COMPILATION_DATABASE=yes". Let's fix it so it works again
with "DEVOPTS=1".

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

diff --git a/Makefile b/Makefile
index 10ea12aae21..94b116d4b37 100644
--- a/Makefile
+++ b/Makefile
@@ -1304,6 +1304,7 @@ endif
 
 ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
 compdb_check = $(shell $(CC) $(ALL_CFLAGS) \
+	-Wno-pedantic \
 	-c -MJ /dev/null \
 	-x c /dev/null -o /dev/null 2>&1; \
 	echo $$?)
-- 
2.33.0.1225.g9f062250122

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 22 2021, Jeff King wrote:

> It's a shame we can't just try to do the _real_ compiles using the
> auto-dependency stuff, and then fall back if they fail. But I think
> there's a chicken-and-egg problem there with "make" doing real work, and
> figuring out the dependencies to do real work.

Isn't this just a chicken & egg problem because we've made it a chicken
& egg problem? I.e. this WIP hack seems to work for me to avoid it:

===============================================================================
diff --git a/Makefile b/Makefile
index 0fe0a9aaa19..68e1714a100 100644
--- a/Makefile
+++ b/Makefile
@@ -1953,6 +1953,7 @@ endif
 ifneq ($(findstring s,$(MAKEFLAGS)),s)
 ifndef V
 	QUIET_CC       = @echo '   ' CC $@;
+	QUIET_DEP      = @echo '   ' DEP $@;
 	QUIET_AR       = @echo '   ' AR $@;
 	QUIET_LINK     = @echo '   ' LINK $@;
 	QUIET_BUILT_IN = @echo '   ' BUILTIN $@;
@@ -2454,24 +2455,6 @@ endif
 .PHONY: objects
 objects: $(OBJECTS)
 
-dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d)
-dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))
-
-ifeq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
-$(dep_dirs):
-	@mkdir -p $@
-
-missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))
-dep_file = $(dir $@).depend/$(notdir $@).d
-dep_args = -MF $(dep_file) -MQ $@ -MMD -MP
-endif
-
-ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
-dep_dirs =
-missing_dep_dirs =
-dep_args =
-endif
-
 compdb_dir = compile_commands
 
 ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
@@ -2489,10 +2472,22 @@ endif
 ASM_SRC := $(wildcard $(OBJECTS:o=S))
 ASM_OBJ := $(ASM_SRC:S=o)
 C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS))
+C_DEP_OBJ := $(OBJECTS:o=dep)
 
 .SUFFIXES:
 
-$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
+ifeq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
+$(C_DEP_OBJ): $(GENERATED_H)
+$(C_DEP_OBJ): %.dep : %.c command-list.h
+	$(QUIET_DEP)$(CC) -MF $*.dep -MQ $*.o -MM $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
+
+ALL.dep: $(C_DEP_OBJ)
+	$(QUIET_DEP)cat $(C_DEP_OBJ) >$@
+
+include ALL.dep
+endif
+
+$(C_OBJ): %.o: %.c %.dep GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
 $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
===============================================================================

This is just stealing a pattern we already use for
Documentation/doc.dep.

I.e. here every *.c file depends on a corresponding *.dep file, we
generate the *.dep files and *.c files seperately, instead of making the
*.dep a "while we're at it" as we make the *.o.

It's on obvious WIP, e.g. there's no ASM_DEP_OBJ yet, and e.g. a
"grep.dep" itself should in turn depend on say "strbuf.h", since if we
touch strbuf.h we should re-generate the grep.dep file before we make
the "grep.o" from the "grep.c".

It also means that if you have a clean tree and make "grep.o" we'll
generate all the *.dep files and the ALL.dep first, although skipping
that could be done with some clever use of $(MAKECMDGOALS) if anyone
cared.

It also makes "git clean -dxf; make -j8 all" take ~17s on my box instead
of ~13s.

I wonder if it's worth doing anyway, the cost of doing incremental
compiles isn't much affected.

I think the cases where the current schema bites us are rather obscure
though, but maybe I haven't thought of some obvious ones.

I.e. if we don't have the dep file we'll make it for the first time when
making the grep.o file, then include those generated dependencies.

If one of those dependencies changes we'll update the dependencies by
virtue of re-making the grep.o. I *think* the only edge cases are if
you've created a grep.o with COMPUTE_HEADER_DEPENDENCIES=yes, then
recompiled and it's gained a new dependency while using
COMPUTE_HEADER_DEPENDENCIES=no, and finally you try to recompile with
COMPUTE_HEADER_DEPENDENCIES=yes and it does nothing, because you're
referencing a stale ".depends" directory.

But that could be solved by just making the grep.o depend on its own
dependency file, which we don't do now, but could easily do.

So maybe I've talked myself out of there being an inherent dependency
graph violation with the current schema, i.e. the current one seems like
an easily solved bug.

One good thing the above approach still gives you is that you can use
GCC or Clang to make the dependency graph, but then use another compiler
to compile, i.e. we could have a $(GCC_LIKE) in addition to $(CC), so if
I'm compiling with xlc or suncc I can still use the present GCC just to
create the dependency graph.

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I almost suggested using "git.c" as the dummy file, since we know it
> must compile anyway. But that probably has other problems (it's more
> expensive, and if it _does_ have an error, the results may be
> confusing).
>
> It's a shame we can't just try to do the _real_ compiles using the
> auto-dependency stuff, and then fall back if they fail. But I think
> there's a chicken-and-egg problem there with "make" doing real work, and
> figuring out the dependencies to do real work.

Yeah, if compiling any of the real sources is inexpensive enough, I
would think that would be the happy way to go.  Do we have a trivial
source that almost never changes?  Perhaps version.c (especially if
we kick out two helper functions that do not really belong there)?

> ...
> I'm happy to submit that on top, or even turn the earlier hunk into a
> patch.  But let's see what Ævar has to say to what's been discussed so
> far. I don't want to derail his effort.

Yup.

Re: [PATCH v2 2/2] Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes

[Carlo Arenas]

FWIW a simpler fix was posted[1] earlier, and you were CC; sorry for the mess

Carlo

[1] https://lore.kernel.org/git/20210922185702.4328-1-carenas@gmail.com/

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Jeff King]

On Wed, Sep 22, 2021 at 05:03:58PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I almost suggested using "git.c" as the dummy file, since we know it
> > must compile anyway. But that probably has other problems (it's more
> > expensive, and if it _does_ have an error, the results may be
> > confusing).
> >
> > It's a shame we can't just try to do the _real_ compiles using the
> > auto-dependency stuff, and then fall back if they fail. But I think
> > there's a chicken-and-egg problem there with "make" doing real work, and
> > figuring out the dependencies to do real work.
> 
> Yeah, if compiling any of the real sources is inexpensive enough, I
> would think that would be the happy way to go.  Do we have a trivial
> source that almost never changes?  Perhaps version.c (especially if
> we kick out two helper functions that do not really belong there)?

Perhaps. TBH, I find Ævar's latest patch to just add -Wno-pedantic to be
the simplest and most obviously-correct fix.

-Peff

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Jeff King]

On Thu, Sep 23, 2021 at 12:40:04AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > It's a shame we can't just try to do the _real_ compiles using the
> > auto-dependency stuff, and then fall back if they fail. But I think
> > there's a chicken-and-egg problem there with "make" doing real work, and
> > figuring out the dependencies to do real work.
> 
> Isn't this just a chicken & egg problem because we've made it a chicken
> & egg problem? I.e. this WIP hack seems to work for me to avoid it:

Possibly, but...

> -$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
> +ifeq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
> +$(C_DEP_OBJ): $(GENERATED_H)
> +$(C_DEP_OBJ): %.dep : %.c command-list.h
> +	$(QUIET_DEP)$(CC) -MF $*.dep -MQ $*.o -MM $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<

I don't think this last dependency is quite right. The .dep for foo.c
must rely not only on foo.c, but also all of its recursive headers (if
it includes foo.h and then foo.h changes to include bar.h, we need to
update the dep file). So it really needs to depend on everything in its
own .dep file, too.

> I.e. here every *.c file depends on a corresponding *.dep file, we
> generate the *.dep files and *.c files seperately, instead of making the
> *.dep a "while we're at it" as we make the *.o.
> [...]
> It also makes "git clean -dxf; make -j8 all" take ~17s on my box instead
> of ~13s.

I definitely think that's easier to reason about, but I'm not surprised
you found that it was slower. To get a more apples-to-apples comparison,
I compiled with -j1. I also use -O0 as my default, which makes the
"real" compilation a relatively smaller portion. I got 47s without your
patch, and 60s with. That seems non-trivial.

I also noticed that "make clean" built all the deps. I think that could
be fixed, but we're piling fixes upon fixes.

So I'm not really sure what we're gaining by splitting this all up.
Given that our original problem is just fixing the test for "do we
support computed dependencies", I think we can solve that pretty easily
and move on.

> So maybe I've talked myself out of there being an inherent dependency
> graph violation with the current schema, i.e. the current one seems like
> an easily solved bug.

Yeah, AFAIK the current system is pretty robust and battle-tested. I'd
be cautious of messing with it unless there's a big gain to be seen.

> One good thing the above approach still gives you is that you can use
> GCC or Clang to make the dependency graph, but then use another compiler
> to compile, i.e. we could have a $(GCC_LIKE) in addition to $(CC), so if
> I'm compiling with xlc or suncc I can still use the present GCC just to
> create the dependency graph.

Keep in mind the worst case in not using the dependency graph is just
that a header update in an incremental compile may cause some extra
compilation. It's really not _that_ bad, and especially so for folks who
aren't actively doing development.

-Peff

Re: [PATCH v2 0/2] Makefile: "pedantic" fallout on .depend and "compdb"

[Jeff King]

On Thu, Sep 23, 2021 at 12:08:00AM +0200, Ævar Arnfjörð Bjarmason wrote:

> In this v2 I just added an unconditional -Wno-pedantic and omitted the
> change to spew error on STDERR. This more narrowly fixes the immediate
> issue and doesn't get into whether we should use /dev/null or whatever
> as input.

FWIW, this seems perfectly fine to me. Removing $(ALL_CFLAGS) entirely
seems OK to me, too, but this is a smaller change, and would help any
cases where those flags are somehow important to getting the compiler to
function at all.

It's possible some compiler _does_ understand -MF, etc, but not
-Wno-pedantic, but that seems unlikely to me. (And of course there is
always the fallback of setting COMPUTE_HEADER_DEPENDENCIES yourself on
such a system; this is really just about doing the right thing on most
people's setups).

Another related alternative is to use -Wno-error, which would fix the
pedantic problem, along with any other warnings a compiler chooses to
bring up for an empty file. I don't have a strong opinion on one versus
the other.

-Peff

Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 22, 2021 at 05:03:58PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > I almost suggested using "git.c" as the dummy file, since we know it
>> > must compile anyway. But that probably has other problems (it's more
>> > expensive, and if it _does_ have an error, the results may be
>> > confusing).
>> >
>> > It's a shame we can't just try to do the _real_ compiles using the
>> > auto-dependency stuff, and then fall back if they fail. But I think
>> > there's a chicken-and-egg problem there with "make" doing real work, and
>> > figuring out the dependencies to do real work.
>> 
>> Yeah, if compiling any of the real sources is inexpensive enough, I
>> would think that would be the happy way to go.  Do we have a trivial
>> source that almost never changes?  Perhaps version.c (especially if
>> we kick out two helper functions that do not really belong there)?
>
> Perhaps. TBH, I find Ævar's latest patch to just add -Wno-pedantic to be
> the simplest and most obviously-correct fix.

Yes, that is very much to-the-point.  If we have trouble with being
pedantic, forcing us not to be is a fine solution ;-)

Re: [PATCH v2 2/2] Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes

[Junio C Hamano]

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

> The same bug fixed in the "COMPUTE_HEADER_DEPENDENCIES=auto" mode in
> the preceding commit was also present with
> "GENERATE_COMPILATION_DATABASE=yes". Let's fix it so it works again
> with "DEVOPTS=1".
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Makefile b/Makefile
> index 10ea12aae21..94b116d4b37 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1304,6 +1304,7 @@ endif
>  
>  ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
>  compdb_check = $(shell $(CC) $(ALL_CFLAGS) \
> +	-Wno-pedantic \

Yup, I like reusing the same approach from the other fix;
removing $(ALL_CFLAGS) may work fine, too, and that is what I
already queued, but I may swap it out with this later (if I do not
forget, that is).

>  	-c -MJ /dev/null \
>  	-x c /dev/null -o /dev/null 2>&1; \
>  	echo $$?)