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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2021, 18:28 UTC
Message-ID: <xmqqzgs479d5.fsf@gitster.g>
In-Reply-To: <YUtiNuUiBU4Xg4gw@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

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

Show 15 quoted lines

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

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