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

[Jeff King <peff@peff.net>]

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