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

From: Jeff King <peff@peff.net>
Date: Sep 22, 2021, 20:17 UTC
Message-ID: <YUuPYDkxjDIvIfwI@coredump.intra.peff.net>
In-Reply-To: <xmqqzgs479d5.fsf@gitster.g>

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

Show 16 quoted lines

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

Show 11 quoted lines

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

Show 14 quoted lines

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