Ævar Arnfjörð Bjarmason [off-list ref] 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
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
On Wed, Sep 22 2021, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
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*
quoted
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]