git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org,
	"Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>
Subject: Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic
Date: Wed, 22 Sep 2021 13:04:54 -0400	[thread overview]
Message-ID: <YUtiNuUiBU4Xg4gw@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqmto48ufz.fsf@gitster.g>

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

  reply	other threads:[~2021-09-22 17:06 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-22 10:38 [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic Ævar Arnfjörð Bjarmason
2021-09-22 10:55 ` Carlo Arenas
2021-09-22 11:04   ` Ævar Arnfjörð Bjarmason
2021-09-22 16:08 ` Junio C Hamano
2021-09-22 17:04   ` Jeff King [this message]
2021-09-22 18:28     ` Junio C Hamano
2021-09-22 18:44       ` Carlo Arenas
2021-09-22 20:17       ` Jeff King
2021-09-22 20:36         ` Carlo Arenas
2021-09-22 22:40         ` Ævar Arnfjörð Bjarmason
2021-09-23 17:32           ` Jeff King
2021-09-23  0:03         ` Junio C Hamano
2021-09-23 16:20           ` Jeff King
2021-09-23 17:41             ` Junio C Hamano
2021-09-22 19:45   ` Ævar Arnfjörð Bjarmason
2021-09-22 22:08 ` [PATCH v2 0/2] Makefile: "pedantic" fallout on .depend and "compdb" Ævar Arnfjörð Bjarmason
2021-09-22 22:08   ` [PATCH v2 1/2] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic Ævar Arnfjörð Bjarmason
2021-09-22 22:08   ` [PATCH v2 2/2] Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes Ævar Arnfjörð Bjarmason
2021-09-23  0:05     ` Carlo Arenas
2021-09-23 21:33     ` Junio C Hamano
2021-09-23 17:38   ` [PATCH v2 0/2] Makefile: "pedantic" fallout on .depend and "compdb" Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YUtiNuUiBU4Xg4gw@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).