{"thread":{"id":"56563","subject":"[PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","startedAt":"2021-09-22T10:38:24Z","lastAt":"2021-09-23T21:33:38Z","messageCount":21,"participants":["Ævar Arnfjörð Bjarmason","Carlo Arenas","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"436710","messageId":"patch-1.1-6b2e9af5e67-20210922T103749Z-avarab@gmail.com","threadId":"56563","inReplyTo":null,"subject":"[PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-22T10:38:07Z","receivedAt":"2021-09-22T10:38:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"COMPUTE_HEADER_DEPENDENCIES\" feature added in [1] was extended to\nuse auto-detection in [2]. Then when -Wpedantic support was added to\nDEVOPTS in [3] we started passing -Wpedantic in combination with\n-Werror to the compiler here.\n\nThis broke the auto-detection, but since we'd quieted it in [4] we\ndidn't find out. It was emitting all of this on STDERR under GCC:\n\n    /dev/null:1: error: ISO C forbids an empty translation unit\n    [-Werror=pedantic]\n    cc1: note: unrecognized command-line option\n    ‘-Wno-pedantic-ms-format’ may have been intended to silence\n    earlier diagnostics\n    cc1: all warnings being treated as errors\n\nLet's fix that bug by maintaining a NON_DEVELOPER_CFLAGS, it's like\nALL_CFLAGS but without anything we add in config.mak.dev, and\nfurthermore stop redirecting STDERR to /dev/null, this means that\nsomeone whose compiler doesn't support this will see this output, but\nalso this new message:\n\n    Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect\n\nIt's also possible that some compilers will emit warnings but still\ngive a zero exit code, anyone using a compiler like that will\npotentially get more verbose output from the Makefile until they set\nCOMPUTE_HEADER_DEPENDENCIES=no. E.g. on AIX's xlc we'll now emit:\n\n    /opt/IBM/xlc/13.1.3/bin/.orig/xlc: 1501-208 (S) command option D is missing a subargument\n    Non-zero 40 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect\n\nAnd on Solaris with SunCC:\n\n    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise\n    cc: refused to overwrite input file by output file: /dev/null\n    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise\n    cc: refused to overwrite input file by output file: /dev/null\n    Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect\n\nBoth are quieted by setting COMPUTE_HEADER_DEPENDENCIES=no as\nsuggested.\n\nI considered piping the output and the exit code to a variable\ninstead, but e.g. under GCC that would lose the coloring in the error\nmessages.\n\n1. f2fabbf76e4 (Teach Makefile to check header dependencies,\n   2010-01-26)\n2. 111ee18c31f (Makefile: Use computed header dependencies if the\n   compiler supports it, 2011-08-18)\n3. 729b3925ed9 (Makefile: add a DEVOPTS flag to get pedantic\n   compilation, 2018-07-24)\n4. 6a8cbc41bac (developer: enable pedantic by default, 2021-09-03)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9df565f27bb..c30f3b8b204 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1242,7 +1242,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)\n ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)\n endif\n \n-ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)\n+NON_DEVELOPER_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(NON_DEVELOPER_CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n \n comma := ,\n@@ -1277,13 +1278,14 @@ COMPUTE_HEADER_DEPENDENCIES = auto\n endif\n \n ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)\n-dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n+dep_check = $(shell $(CC) $(NON_DEVELOPER_CFLAGS) \\\n \t-c -MF /dev/null -MQ /dev/null -MMD -MP \\\n-\t-x c /dev/null -o /dev/null 2>&1; \\\n+\t-x c /dev/null -o /dev/null; \\\n \techo $$?)\n ifeq ($(dep_check),0)\n override COMPUTE_HEADER_DEPENDENCIES = yes\n else\n+$(info Non-zero $(dep_check) exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect)\n override COMPUTE_HEADER_DEPENDENCIES = no\n endif\n endif\n-- \n2.33.0.1225.g17f21f53d74\n\n"},{"id":"436711","messageId":"CAPUEsph8TYokqLmiZjvu3VgmzLh4AFhp+0vh0nv+2+m351=n_A@mail.gmail.com","threadId":"56563","inReplyTo":"patch-1.1-6b2e9af5e67-20210922T103749Z-avarab@gmail.com","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-22T10:55:26Z","receivedAt":"2021-09-22T10:55:53Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Sep 22, 2021 at 3:38 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> The \"COMPUTE_HEADER_DEPENDENCIES\" feature added in [1] was extended to\n> use auto-detection in [2]. Then when -Wpedantic support was added to\n> DEVOPTS in [3] we started passing -Wpedantic in combination with\n> -Werror to the compiler here.\n\nIt is my impression though that header dependencies computation is\nsomething that might only work in gcc and clang (because of its gcc\ncompatibility), so shouldn't this be restricted to those compilers\ninstead of forcing all others to error?\n\nAlso, why is this process run with DEVELOPER=1 to begin with, if we\nobviously don't need/want any compilations warnings?\n\nCarlo\n"},{"id":"436712","messageId":"87o88k7tr6.fsf@evledraar.gmail.com","threadId":"56563","inReplyTo":"CAPUEsph8TYokqLmiZjvu3VgmzLh4AFhp+0vh0nv+2+m351=n_A@mail.gmail.com","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-22T11:04:48Z","receivedAt":"2021-09-22T11:08:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 22 2021, Carlo Arenas wrote:\n\n> On Wed, Sep 22, 2021 at 3:38 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>\n>> The \"COMPUTE_HEADER_DEPENDENCIES\" feature added in [1] was extended to\n>> use auto-detection in [2]. Then when -Wpedantic support was added to\n>> DEVOPTS in [3] we started passing -Wpedantic in combination with\n>> -Werror to the compiler here.\n>\n> It is my impression though that header dependencies computation is\n> something that might only work in gcc and clang (because of its gcc\n> compatibility), so shouldn't this be restricted to those compilers\n> instead of forcing all others to error?\n\nI think it's better to just have those not supporting\nCOMPUTE_HEADER_DEPENDENCIES define that, wanting to auto-detect things\nis what led to the current breakage.\n\nI.e. our whitelisting of GCC and Clang will be out of date if some other\ncompiler grows support for this, and even if we whitelist gcc and clang\nwe'll need some script like detect-compiler etc.\n\n> Also, why is this process run with DEVELOPER=1 to begin with, if we\n> obviously don't need/want any compilations warnings?\n\nHrm? We run the COMPUTE_HEADER_DEPENDENCIES=yes process with and without\nDEVELOPER=1, it's just that DEVELOPER=1 breaks it since it turns on\npedantic compilation flags now.\n\nWe want -Werror for the actual compilation under DEVELOPER=1, but a\none-off command to see if a compiler has support for something is\nentirely different.\n"},{"id":"436728","messageId":"xmqqmto48ufz.fsf@gitster.g","threadId":"56563","inReplyTo":"patch-1.1-6b2e9af5e67-20210922T103749Z-avarab@gmail.com","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T16:08:00Z","receivedAt":"2021-09-22T16:08:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> The \"COMPUTE_HEADER_DEPENDENCIES\" feature added in [1] was extended to\n> use auto-detection in [2]. Then when -Wpedantic support was added to\n> DEVOPTS in [3] we started passing -Wpedantic in combination with\n> -Werror to the compiler here.\n>\n> This broke the auto-detection, but since we'd quieted it in [4] we\n> didn't find out.\n\nAre the references correct?  I am not seeing \"quiet\"ing in [4].  The\nredirection 2>&1 to cram error messages also to $(dep_check), hence\nmaking it impossible to match '0', was done in [2].\n\nWe did make the pedantic mode the default and pass both -pedantic\nand -Wpedantic after [4].  Before we had only -pedantic.\n\n> It was emitting all of this on STDERR under GCC:\n>\n>     /dev/null:1: error: ISO C forbids an empty translation unit\n>     [-Werror=pedantic]\n>     cc1: note: unrecognized command-line option\n>     ‘-Wno-pedantic-ms-format’ may have been intended to silence\n>     earlier diagnostics\n>     cc1: all warnings being treated as errors\n>\n> Let's fix that bug by maintaining a NON_DEVELOPER_CFLAGS, it's like\n> ALL_CFLAGS but without anything we add in config.mak.dev, and\n> furthermore stop redirecting STDERR to /dev/null, this means that\n> someone whose compiler doesn't support this will see this output, but\n> also this new message:\n>\n>     Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect\n\n\nHmmmmmph.  \n\nI recentaly saw many .depend directories (not necessarily empty)\nleft after \"make distclean\". After building on one branch, I often\ncheck out a different branch then run distclean on the new branch,\nso leftover build artifacts are not necessarily a bug in our\nMakefile, but the bug you found may explain it?\n\nWhile I agree with your analysis of the problem, I cannot shake this\nnagging feeling that the proposed solution is barking up a wrong\ntree.  After all, -pedantic and any other option that lets the\ncompiler notice that it is being asked to compile an empty source\ncan come directly from the end user (e.g. CC=\"gcc -pedantic\" or as\npart of CFLAGS)---realization of which makes me wonder if it is\nessential to compile /dev/null for this check, or any reasonably\nsyntactically correct program would do.\n\nI wonder if the attached (with clean-up to remove the tracing cruft)\nwould show us a better direction.  It feeds a single line\n\n\tint dummy_for_dep_check;\n\nC \"program\" from the standard input of the compiler to tackle the\n\"you are not supposed to be compiling an empty compilation unit\"\nproblem in a more direct way.\n\n Makefile | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git c/Makefile w/Makefile\nindex 9df565f27b..0593ab7287 100644\n--- c/Makefile\n+++ w/Makefile\n@@ -1277,9 +1277,9 @@ COMPUTE_HEADER_DEPENDENCIES = auto\n endif\n \n ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)\n-dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n+dep_check = $(shell echo >&2 doing dep check; echo int dummy_for_dep_check\\; | $(CC) $(ALL_CFLAGS) \\\n \t-c -MF /dev/null -MQ /dev/null -MMD -MP \\\n-\t-x c /dev/null -o /dev/null 2>&1; \\\n+\t-x c - -o /dev/null || echo >&2 oops; \\\n \techo $$?)\n ifeq ($(dep_check),0)\n override COMPUTE_HEADER_DEPENDENCIES = yes\n"},{"id":"436736","messageId":"YUtiNuUiBU4Xg4gw@coredump.intra.peff.net","threadId":"56563","inReplyTo":"xmqqmto48ufz.fsf@gitster.g","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-22T17:04:54Z","receivedAt":"2021-09-22T17:04:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2021 at 09:08:00AM -0700, Junio C Hamano wrote:\n\n> While I agree with your analysis of the problem, I cannot shake this\n> nagging feeling that the proposed solution is barking up a wrong\n> tree.  After all, -pedantic and any other option that lets the\n> compiler notice that it is being asked to compile an empty source\n> can come directly from the end user (e.g. CC=\"gcc -pedantic\" or as\n> part of CFLAGS)---realization of which makes me wonder if it is\n> essential to compile /dev/null for this check, or any reasonably\n> syntactically correct program would do.\n> \n> I wonder if the attached (with clean-up to remove the tracing cruft)\n> would show us a better direction.  It feeds a single line\n> \n> \tint dummy_for_dep_check;\n> \n> C \"program\" from the standard input of the compiler to tackle the\n> \"you are not supposed to be compiling an empty compilation unit\"\n> problem in a more direct way.\n\nThat feels a bit like we're playing a game of chicken with the compiler\nin terms of what it may complain about. For example, sparse will\ncomplain:\n\n  foo.c:1:5: warning: symbol 'dummy_for_dep_check' was not declared. Should it be static?\n\nMight compilers ever learn to warn of the same thing?\n\nI kind of like the simplicity of Ævar's approach. We want to know if the\ncompiler can be invoked with options XYZ, so we do so. That should be\nlargely independent of our cflags, and there's prior art in how we\ninvoke it in the detect-compiler script.\n\nSo I'd argue we should go even simpler, like:\n\ndiff --git a/Makefile b/Makefile\nindex 3628d14f16..4597a126d0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1277,7 +1277,7 @@ COMPUTE_HEADER_DEPENDENCIES = auto\n endif\n \n ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)\n-dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n+dep_check = $(shell $(CC) \\\n \t-c -MF /dev/null -MQ /dev/null -MMD -MP \\\n \t-x c /dev/null -o /dev/null 2>&1; \\\n \techo $$?)\n\nI'm also tempted by a hunk like this. Then we can set the REQUIRE flag\nin a CI job (or locally for git devs who know they have gcc) and notice\nan unexpected breakage in the auto test.\n\n@@ -1295,6 +1295,9 @@ ifneq ($(COMPUTE_HEADER_DEPENDENCIES),no)\n $(error please set COMPUTE_HEADER_DEPENDENCIES to yes, no, or auto \\\n (not \"$(COMPUTE_HEADER_DEPENDENCIES)\"))\n endif\n+ifdef REQUIRE_COMPUTE_HEADER_DEPENDENCIES\n+$(error computed header dependencies required, but auto-check did not find them)\n+endif\n endif\n \n ifndef GENERATE_COMPILATION_DATABASE\n\n-Peff\n"},{"id":"436745","messageId":"xmqqzgs479d5.fsf@gitster.g","threadId":"56563","inReplyTo":"YUtiNuUiBU4Xg4gw@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T18:28:38Z","receivedAt":"2021-09-22T18:28:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> I wonder if the attached (with clean-up to remove the tracing cruft)\n>> would show us a better direction.  It feeds a single line\n>> \n>> \tint dummy_for_dep_check;\n>> \n>> C \"program\" from the standard input of the compiler to tackle the\n>> \"you are not supposed to be compiling an empty compilation unit\"\n>> problem in a more direct way.\n>\n> That feels a bit like we're playing a game of chicken with the compiler\n> in terms of what it may complain about. For example, sparse will\n> complain:\n>\n>   foo.c:1:5: warning: symbol 'dummy_for_dep_check' was not declared. Should it be static?\n>\n> Might compilers ever learn to warn of the same thing?\n\nCertainly.  That is the reason why I said \"direction\", not\n\"solution\", and I do not think it is beyond our capability to come\nup with a minimal \"C program\" that would be lint clean to make it\nas a part of the \"solution\".\n\nFor example, would sparse or compilers complain about this?\n\n    extern int incr(int); int incr(int i) { return i + 1; }\n\n> So I'd argue we should go even simpler, like:\n>\n> diff --git a/Makefile b/Makefile\n> index 3628d14f16..4597a126d0 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1277,7 +1277,7 @@ COMPUTE_HEADER_DEPENDENCIES = auto\n>  endif\n>  \n>  ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)\n> -dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n> +dep_check = $(shell $(CC) \\\n>  \t-c -MF /dev/null -MQ /dev/null -MMD -MP \\\n>  \t-x c /dev/null -o /dev/null 2>&1; \\\n>  \techo $$?)\n\nI am all for this simplification.  It takes us back to the state\nbefore 1816bf26 (Makefile: Improve compiler header dependency check,\n2011-08-30).  But I think that is more or less orthogonal to the\n\"you are not supposed to feed an empty compilation unit\" issue.\n\n> I'm also tempted by a hunk like this. Then we can set the REQUIRE flag\n> in a CI job (or locally for git devs who know they have gcc) and notice\n> an unexpected breakage in the auto test.\n>\n> @@ -1295,6 +1295,9 @@ ifneq ($(COMPUTE_HEADER_DEPENDENCIES),no)\n>  $(error please set COMPUTE_HEADER_DEPENDENCIES to yes, no, or auto \\\n>  (not \"$(COMPUTE_HEADER_DEPENDENCIES)\"))\n>  endif\n> +ifdef REQUIRE_COMPUTE_HEADER_DEPENDENCIES\n> +$(error computed header dependencies required, but auto-check did not find them)\n> +endif\n>  endif\n\nYup, I like that, too.\n"},{"id":"436748","messageId":"CAPUEspjjfTJGaPUxvaNeGFFyKJYRurxoraX+2DNg2GF02WXn-g@mail.gmail.com","threadId":"56563","inReplyTo":"xmqqzgs479d5.fsf@gitster.g","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-22T18:44:01Z","receivedAt":"2021-09-22T18:44:16Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Sep 22, 2021 at 11:28 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jeff King <peff@peff.net> writes:\n>\n> >> I wonder if the attached (with clean-up to remove the tracing cruft)\n> >> would show us a better direction.  It feeds a single line\n> >>\n> >>      int dummy_for_dep_check;\n> >>\n> >> C \"program\" from the standard input of the compiler to tackle the\n> >> \"you are not supposed to be compiling an empty compilation unit\"\n> >> problem in a more direct way.\n> >\n> > That feels a bit like we're playing a game of chicken with the compiler\n> > in terms of what it may complain about. For example, sparse will\n> > complain:\n> >\n> >   foo.c:1:5: warning: symbol 'dummy_for_dep_check' was not declared. Should it be static?\n> >\n> > Might compilers ever learn to warn of the same thing?\n>\n> Certainly.  That is the reason why I said \"direction\", not\n> \"solution\", and I do not think it is beyond our capability to come\n> up with a minimal \"C program\" that would be lint clean to make it\n> as a part of the \"solution\".\n>\n> For example, would sparse or compilers complain about this?\n>\n>     extern int incr(int); int incr(int i) { return i + 1; }\n>\n> > So I'd argue we should go even simpler, like:\n> >\n> > diff --git a/Makefile b/Makefile\n> > index 3628d14f16..4597a126d0 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -1277,7 +1277,7 @@ COMPUTE_HEADER_DEPENDENCIES = auto\n> >  endif\n> >\n> >  ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)\n> > -dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n> > +dep_check = $(shell $(CC) \\\n> >       -c -MF /dev/null -MQ /dev/null -MMD -MP \\\n> >       -x c /dev/null -o /dev/null 2>&1; \\\n> >       echo $$?)\n>\n> I am all for this simplification.  It takes us back to the state\n> before 1816bf26 (Makefile: Improve compiler header dependency check,\n> 2011-08-30).  But I think that is more or less orthogonal to the\n> \"you are not supposed to feed an empty compilation unit\" issue.\n\nthe problem really is IMHO, that we are passing around CFLAGS that\nindicate 2 things:\n\n1) attributes that are relevant to how we build and what\n2) which diagnostics we want to use\n\nÆvar's approach addresses implicitly part of it (most of the\ndiagnostics are added in config.mak.dev), but I think it still feels\nlike a knee jerk reaction to the problem, while creating an\nmaintaining something like \"DIAGNOSTIC_FLAGS\" which we will only pass\nto the compiler when we want to use diagnostic (including -pedantic,\n-Wpedantic and -Werror) would be a better approach.\n\nCarlo\n"},{"id":"436764","messageId":"87fstw75iv.fsf@evledraar.gmail.com","threadId":"56563","inReplyTo":"xmqqmto48ufz.fsf@gitster.g","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-22T19:45:52Z","receivedAt":"2021-09-22T19:51:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 22 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> The \"COMPUTE_HEADER_DEPENDENCIES\" feature added in [1] was extended to\n>> use auto-detection in [2]. Then when -Wpedantic support was added to\n>> DEVOPTS in [3] we started passing -Wpedantic in combination with\n>> -Werror to the compiler here.\n>>\n>> This broke the auto-detection, but since we'd quieted it in [4] we\n>> didn't find out.\n>\n> Are the references correct?  I am not seeing \"quiet\"ing in [4].  The\n> redirection 2>&1 to cram error messages also to $(dep_check), hence\n> making it impossible to match '0', was done in [2].\n\nYes it's incorrect, I meant [2]. I had this right in my head, just got\nthe references wrong somehow,thanks.\n\n> We did make the pedantic mode the default and pass both -pedantic\n> and -Wpedantic after [4].  Before we had only -pedantic.\n\n*nod*\n\n>> It was emitting all of this on STDERR under GCC:\n>>\n>>     /dev/null:1: error: ISO C forbids an empty translation unit\n>>     [-Werror=pedantic]\n>>     cc1: note: unrecognized command-line option\n>>     ‘-Wno-pedantic-ms-format’ may have been intended to silence\n>>     earlier diagnostics\n>>     cc1: all warnings being treated as errors\n>>\n>> Let's fix that bug by maintaining a NON_DEVELOPER_CFLAGS, it's like\n>> ALL_CFLAGS but without anything we add in config.mak.dev, and\n>> furthermore stop redirecting STDERR to /dev/null, this means that\n>> someone whose compiler doesn't support this will see this output, but\n>> also this new message:\n>>\n>>     Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect\n>\n>\n> Hmmmmmph.  \n>\n> I recentaly saw many .depend directories (not necessarily empty)\n> left after \"make distclean\". After building on one branch, I often\n> check out a different branch then run distclean on the new branch,\n> so leftover build artifacts are not necessarily a bug in our\n> Makefile, but the bug you found may explain it?\n\nYes, I'll update the commit message, the problem is that we'll empty the\ndep_dirs list if we're not *currently* making them, that's a logic error\nin a few places in the Makefile, i.e. conflating currently building X\nwith wanting to clean X.\n\n[Will respond to the rest with a re-roll and/or in other replies in-thread]\n"},{"id":"436773","messageId":"YUuPYDkxjDIvIfwI@coredump.intra.peff.net","threadId":"56563","inReplyTo":"xmqqzgs479d5.fsf@gitster.g","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-22T20:17:36Z","receivedAt":"2021-09-22T20:17:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2021 at 11:28:38AM -0700, Junio C Hamano wrote:\n\n> > That feels a bit like we're playing a game of chicken with the compiler\n> > in terms of what it may complain about. For example, sparse will\n> > complain:\n> >\n> >   foo.c:1:5: warning: symbol 'dummy_for_dep_check' was not declared. Should it be static?\n> >\n> > Might compilers ever learn to warn of the same thing?\n> \n> Certainly.  That is the reason why I said \"direction\", not\n> \"solution\", and I do not think it is beyond our capability to come\n> up with a minimal \"C program\" that would be lint clean to make it\n> as a part of the \"solution\".\n> \n> For example, would sparse or compilers complain about this?\n> \n>     extern int incr(int); int incr(int i) { return i + 1; }\n\nFair enough. It is a game of chicken, but it is one that is easy to win. :)\n\nI almost suggested using \"git.c\" as the dummy file, since we know it\nmust compile anyway. But that probably has other problems (it's more\nexpensive, and if it _does_ have an error, the results may be\nconfusing).\n\nIt's a shame we can't just try to do the _real_ compiles using the\nauto-dependency stuff, and then fall back if they fail. But I think\nthere's a chicken-and-egg problem there with \"make\" doing real work, and\nfiguring out the dependencies to do real work.\n\n> >  ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)\n> > -dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n> > +dep_check = $(shell $(CC) \\\n> >  \t-c -MF /dev/null -MQ /dev/null -MMD -MP \\\n> >  \t-x c /dev/null -o /dev/null 2>&1; \\\n> >  \techo $$?)\n> \n> I am all for this simplification.  It takes us back to the state\n> before 1816bf26 (Makefile: Improve compiler header dependency check,\n> 2011-08-30).  But I think that is more or less orthogonal to the\n> \"you are not supposed to feed an empty compilation unit\" issue.\n\nHmm, my suggestion was off the cuff without digging to see whether we\nused to do something similar. ;)\n\nI do worry a bit that we'd be regressing the case that commit tried to\nfix. OTOH, I'm not sure I understand its commit message. It talks about\nthings in CFLAGS being a problem, but it looks like the original (and my\nproposal here) would not look at CFLAGS at all? If people are putting\nstuff into CC that will break when used without CFLAGS, then I feel like\nthe answer might be \"don't do that\". Or are there common situations\nwhere $(CC) is not expected to behave sensibly on its own?\n\n> > I'm also tempted by a hunk like this. Then we can set the REQUIRE flag\n> > in a CI job (or locally for git devs who know they have gcc) and notice\n> > an unexpected breakage in the auto test.\n> >\n> > @@ -1295,6 +1295,9 @@ ifneq ($(COMPUTE_HEADER_DEPENDENCIES),no)\n> >  $(error please set COMPUTE_HEADER_DEPENDENCIES to yes, no, or auto \\\n> >  (not \"$(COMPUTE_HEADER_DEPENDENCIES)\"))\n> >  endif\n> > +ifdef REQUIRE_COMPUTE_HEADER_DEPENDENCIES\n> > +$(error computed header dependencies required, but auto-check did not find them)\n> > +endif\n> >  endif\n> \n> Yup, I like that, too.\n\nI'm happy to submit that on top, or even turn the earlier hunk into a\npatch.  But let's see what Ævar has to say to what's been discussed so\nfar. I don't want to derail his effort.\n\n-Peff\n"},{"id":"436779","messageId":"CAPUEspi5B8gQ5XVsgkAFDDrkE+Uo__9dzFm7UTZzCZUeExA=8w@mail.gmail.com","threadId":"56563","inReplyTo":"YUuPYDkxjDIvIfwI@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-22T20:36:50Z","receivedAt":"2021-09-22T20:37:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Sep 22, 2021 at 1:17 PM Jeff King <peff@peff.net> wrote:\n>\n> On Wed, Sep 22, 2021 at 11:28:38AM -0700, Junio C Hamano wrote:\n>\n> > >  ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)\n> > > -dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n> > > +dep_check = $(shell $(CC) \\\n> > >     -c -MF /dev/null -MQ /dev/null -MMD -MP \\\n> > >     -x c /dev/null -o /dev/null 2>&1; \\\n> > >     echo $$?)\n> >\n> > I am all for this simplification.  It takes us back to the state\n> > before 1816bf26 (Makefile: Improve compiler header dependency check,\n> > 2011-08-30).  But I think that is more or less orthogonal to the\n> > \"you are not supposed to feed an empty compilation unit\" issue.\n>\n> Hmm, my suggestion was off the cuff without digging to see whether we\n> used to do something similar. ;)\n>\n> I do worry a bit that we'd be regressing the case that commit tried to\n> fix. OTOH, I'm not sure I understand its commit message. It talks about\n> things in CFLAGS being a problem, but it looks like the original (and my\n> proposal here) would not look at CFLAGS at all? If people are putting\n> stuff into CC that will break when used without CFLAGS, then I feel like\n> the answer might be \"don't do that\". Or are there common situations\n> where $(CC) is not expected to behave sensibly on its own?\n\nYes, the problem was ONLY with the gcc that came in macOS before they\nmoved to clang and that had a frankenstein set of options which will\nprevent people building BOTH fat binaries and doing this header\ndependency computation at the same time.\n\nIronically, modern clang makes his use case even uglier (running on\ntop of Ævar's fix) :\n\n$ make CFLAGS=\"-arch x86_64 -arch arm64\"\nfatal error: /Library/Developer/CommandLineTools/usr/bin/lipo: can't\ncreate temporary output file: /dev/null.lipo (Operation not permitted)\nclang: error: lipo command failed with exit code 1 (use -v to see invocation)\nfatal error: /Library/Developer/CommandLineTools/usr/bin/lipo: can't\ncreate temporary output file: /dev/null.lipo (Operation not permitted)\nclang: error: lipo command failed with exit code 1 (use -v to see invocation)\nNon-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" t\n\nso the test fails, and COMPUTE_HEADER_DEPENDENCIES is disabled, but it\nshouldn't and works fine if the test is overridden with\nCOMPUTE_HEADER_DEPENDENCIES=yes\n\nso by removing the CFLAGS from that test, we will actually be fixing\nthis use case as well IMHO.\n\nCarlo\n"},{"id":"436785","messageId":"patch-v2-1.2-31c871e9bf6-20210922T220532Z-avarab@gmail.com","threadId":"56563","inReplyTo":"cover-v2-0.2-00000000000-20210922T220532Z-avarab@gmail.com","subject":"[PATCH v2 1/2] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-22T22:08:01Z","receivedAt":"2021-09-22T22:08:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"COMPUTE_HEADER_DEPENDENCIES\" feature added in [1] was extended to\nuse auto-detection in [2], that \"auto\" detection has always piped\nSTDERR to /dev/null, so any failures on compilers that didn't support\nthese GCC flags would silently fall back to\n\"COMPUTE_HEADER_DEPENDENCIES=no\".\n\nLater when -Wpedantic support was added to DEVOPTS in [3] we started\npassing -Wpedantic in combination with -Werror to the compiler\nhere. Note (to the pedantic): [3] actually passed \"-pedantic\", but it\nand \"-Wpedantic\" are synonyms.\n\nTurning on -Wpedantic in [3] broke the auto-detection, since this\nrelies on compiling an empty program. GCC would loudly complain on\nSTDERR:\n\n    /dev/null:1: error: ISO C forbids an empty translation unit\n    [-Werror=pedantic]\n    cc1: note: unrecognized command-line option\n    ‘-Wno-pedantic-ms-format’ may have been intended to silence\n    earlier diagnostics\n    cc1: all warnings being treated as errors\n\nBut as that ended up in the \"$(dep_check)\" variable due to the \"2>&1\"\nin [2] we didn't see it.\n\nThen when [4] made DEVOPTS=pedantic the default specifying\n\"DEVELOPER=1\" would effectively set \"COMPUTE_HEADER_DEPENDENCIES=no\".\n\nTo fix these issues let's unconditionally pass -Wno-pedantic after\n$(ALL_CFLAGS), we might get a -Wpedantic via config.mak.dev after, or\nthe builder might specify it via CFLAGS. In either case this will undo\ncurrent and future problems with -Wpedantic.\n\nI think it would make sense to simply remove the \"2>&1\", it would mean\nthat anyone using a non-GCC-like compiler would get warnings under\nCOMPUTE_HEADER_DEPENDENCIES=auto, e.g on AIX's xlc would emit:\n\n    /opt/IBM/xlc/13.1.3/bin/.orig/xlc: 1501-208 (S) command option D is missing a subargument\n    Non-zero 40 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect\n\nAnd on Solaris with SunCC:\n\n    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise\n    cc: refused to overwrite input file by output file: /dev/null\n    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise\n    cc: refused to overwrite input file by output file: /dev/null\n    Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect\n\nBoth could be quieted by setting COMPUTE_HEADER_DEPENDENCIES=no\nexplicitly, as suggested, but let's see if this'll fix it without\nemitting too much noise at those that aren't using \"gcc\" or \"clang\".\n\n1. f2fabbf76e4 (Teach Makefile to check header dependencies,\n   2010-01-26)\n2. 111ee18c31f (Makefile: Use computed header dependencies if the\n   compiler supports it, 2011-08-18)\n3. 729b3925ed9 (Makefile: add a DEVOPTS flag to get pedantic\n   compilation, 2018-07-24)\n4. 6a8cbc41bac (developer: enable pedantic by default, 2021-09-03)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 9df565f27bb..10ea12aae21 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1278,6 +1278,7 @@ endif\n \n ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)\n dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n+\t-Wno-pedantic \\\n \t-c -MF /dev/null -MQ /dev/null -MMD -MP \\\n \t-x c /dev/null -o /dev/null 2>&1; \\\n \techo $$?)\n-- \n2.33.0.1225.g9f062250122\n\n"},{"id":"436786","messageId":"cover-v2-0.2-00000000000-20210922T220532Z-avarab@gmail.com","threadId":"56563","inReplyTo":"patch-1.1-6b2e9af5e67-20210922T103749Z-avarab@gmail.com","subject":"[PATCH v2 0/2] Makefile: \"pedantic\" fallout on .depend and \"compdb\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-22T22:08:00Z","receivedAt":"2021-09-22T22:08:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In this v2 I just added an unconditional -Wno-pedantic and omitted the\nchange to spew error on STDERR. This more narrowly fixes the immediate\nissue and doesn't get into whether we should use /dev/null or whatever\nas input.\n\nI then noticed that the same bug was present in\n\"GENERATE_COMPILATION_DATABASE=yes\", so there's now a 2nd patch to fix\nthat.\n\nÆvar Arnfjörð Bjarmason (2):\n  Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with\n    DEVOPTS=pedantic\n  Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes\n\n Makefile | 2 ++\n 1 file changed, 2 insertions(+)\n\nRange-diff against v1:\n1:  3ff8ea12bf3 ! 1:  31c871e9bf6 Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic\n    @@ Commit message\n         Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic\n     \n         The \"COMPUTE_HEADER_DEPENDENCIES\" feature added in [1] was extended to\n    -    use auto-detection in [2]. Then when -Wpedantic support was added to\n    -    DEVOPTS in [3] we started passing -Wpedantic in combination with\n    -    -Werror to the compiler here.\n    +    use auto-detection in [2], that \"auto\" detection has always piped\n    +    STDERR to /dev/null, so any failures on compilers that didn't support\n    +    these GCC flags would silently fall back to\n    +    \"COMPUTE_HEADER_DEPENDENCIES=no\".\n     \n    -    This broke the auto-detection, but since we'd quieted it in [4] we\n    -    didn't find out. It was emitting all of this on STDERR under GCC:\n    +    Later when -Wpedantic support was added to DEVOPTS in [3] we started\n    +    passing -Wpedantic in combination with -Werror to the compiler\n    +    here. Note (to the pedantic): [3] actually passed \"-pedantic\", but it\n    +    and \"-Wpedantic\" are synonyms.\n    +\n    +    Turning on -Wpedantic in [3] broke the auto-detection, since this\n    +    relies on compiling an empty program. GCC would loudly complain on\n    +    STDERR:\n     \n             /dev/null:1: error: ISO C forbids an empty translation unit\n             [-Werror=pedantic]\n    @@ Commit message\n             earlier diagnostics\n             cc1: all warnings being treated as errors\n     \n    -    Let's fix that bug by maintaining a NON_DEVELOPER_CFLAGS, it's like\n    -    ALL_CFLAGS but without anything we add in config.mak.dev, and\n    -    furthermore stop redirecting STDERR to /dev/null, this means that\n    -    someone whose compiler doesn't support this will see this output, but\n    -    also this new message:\n    +    But as that ended up in the \"$(dep_check)\" variable due to the \"2>&1\"\n    +    in [2] we didn't see it.\n     \n    -        Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect\n    +    Then when [4] made DEVOPTS=pedantic the default specifying\n    +    \"DEVELOPER=1\" would effectively set \"COMPUTE_HEADER_DEPENDENCIES=no\".\n    +\n    +    To fix these issues let's unconditionally pass -Wno-pedantic after\n    +    $(ALL_CFLAGS), we might get a -Wpedantic via config.mak.dev after, or\n    +    the builder might specify it via CFLAGS. In either case this will undo\n    +    current and future problems with -Wpedantic.\n     \n    -    It's also possible that some compilers will emit warnings but still\n    -    give a zero exit code, anyone using a compiler like that will\n    -    potentially get more verbose output from the Makefile until they set\n    -    COMPUTE_HEADER_DEPENDENCIES=no. E.g. on AIX's xlc we'll now emit:\n    +    I think it would make sense to simply remove the \"2>&1\", it would mean\n    +    that anyone using a non-GCC-like compiler would get warnings under\n    +    COMPUTE_HEADER_DEPENDENCIES=auto, e.g on AIX's xlc would emit:\n     \n             /opt/IBM/xlc/13.1.3/bin/.orig/xlc: 1501-208 (S) command option D is missing a subargument\n             Non-zero 40 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect\n    @@ Commit message\n             cc: refused to overwrite input file by output file: /dev/null\n             Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect\n     \n    -    Both are quieted by setting COMPUTE_HEADER_DEPENDENCIES=no as\n    -    suggested.\n    -\n    -    I considered piping the output and the exit code to a variable\n    -    instead, but e.g. under GCC that would lose the coloring in the error\n    -    messages.\n    +    Both could be quieted by setting COMPUTE_HEADER_DEPENDENCIES=no\n    +    explicitly, as suggested, but let's see if this'll fix it without\n    +    emitting too much noise at those that aren't using \"gcc\" or \"clang\".\n     \n         1. f2fabbf76e4 (Teach Makefile to check header dependencies,\n            2010-01-26)\n    @@ Commit message\n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Makefile ##\n    -@@ Makefile: ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)\n    - ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)\n    - endif\n    - \n    --ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)\n    -+NON_DEVELOPER_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n    -+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(NON_DEVELOPER_CFLAGS)\n    - ALL_LDFLAGS = $(LDFLAGS)\n    - \n    - comma := ,\n    -@@ Makefile: COMPUTE_HEADER_DEPENDENCIES = auto\n    - endif\n    +@@ Makefile: endif\n      \n      ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)\n    --dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n    -+dep_check = $(shell $(CC) $(NON_DEVELOPER_CFLAGS) \\\n    + dep_check = $(shell $(CC) $(ALL_CFLAGS) \\\n    ++\t-Wno-pedantic \\\n      \t-c -MF /dev/null -MQ /dev/null -MMD -MP \\\n    --\t-x c /dev/null -o /dev/null 2>&1; \\\n    -+\t-x c /dev/null -o /dev/null; \\\n    + \t-x c /dev/null -o /dev/null 2>&1; \\\n      \techo $$?)\n    - ifeq ($(dep_check),0)\n    - override COMPUTE_HEADER_DEPENDENCIES = yes\n    - else\n    -+$(info Non-zero $(dep_check) exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to \"yes\" or \"no\" to quiet auto-detect)\n    - override COMPUTE_HEADER_DEPENDENCIES = no\n    - endif\n    - endif\n-:  ----------- > 2:  6b18bd08894 Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes\n-- \n2.33.0.1225.g9f062250122\n\n"},{"id":"436787","messageId":"patch-v2-2.2-6b18bd08894-20210922T220532Z-avarab@gmail.com","threadId":"56563","inReplyTo":"cover-v2-0.2-00000000000-20210922T220532Z-avarab@gmail.com","subject":"[PATCH v2 2/2] Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-22T22:08:02Z","receivedAt":"2021-09-22T22:08:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The same bug fixed in the \"COMPUTE_HEADER_DEPENDENCIES=auto\" mode in\nthe preceding commit was also present with\n\"GENERATE_COMPILATION_DATABASE=yes\". Let's fix it so it works again\nwith \"DEVOPTS=1\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 10ea12aae21..94b116d4b37 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1304,6 +1304,7 @@ endif\n \n ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n compdb_check = $(shell $(CC) $(ALL_CFLAGS) \\\n+\t-Wno-pedantic \\\n \t-c -MJ /dev/null \\\n \t-x c /dev/null -o /dev/null 2>&1; \\\n \techo $$?)\n-- \n2.33.0.1225.g9f062250122\n\n"},{"id":"436799","messageId":"8735pw6w8z.fsf@evledraar.gmail.com","threadId":"56563","inReplyTo":"YUuPYDkxjDIvIfwI@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-22T22:40:04Z","receivedAt":"2021-09-22T23:12:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 22 2021, Jeff King wrote:\n\n> It's a shame we can't just try to do the _real_ compiles using the\n> auto-dependency stuff, and then fall back if they fail. But I think\n> there's a chicken-and-egg problem there with \"make\" doing real work, and\n> figuring out the dependencies to do real work.\n\nIsn't this just a chicken & egg problem because we've made it a chicken\n& egg problem? I.e. this WIP hack seems to work for me to avoid it:\n\n===============================================================================\ndiff --git a/Makefile b/Makefile\nindex 0fe0a9aaa19..68e1714a100 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1953,6 +1953,7 @@ endif\n ifneq ($(findstring s,$(MAKEFLAGS)),s)\n ifndef V\n \tQUIET_CC       = @echo '   ' CC $@;\n+\tQUIET_DEP      = @echo '   ' DEP $@;\n \tQUIET_AR       = @echo '   ' AR $@;\n \tQUIET_LINK     = @echo '   ' LINK $@;\n \tQUIET_BUILT_IN = @echo '   ' BUILTIN $@;\n@@ -2454,24 +2455,6 @@ endif\n .PHONY: objects\n objects: $(OBJECTS)\n \n-dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d)\n-dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))\n-\n-ifeq ($(COMPUTE_HEADER_DEPENDENCIES),yes)\n-$(dep_dirs):\n-\t@mkdir -p $@\n-\n-missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))\n-dep_file = $(dir $@).depend/$(notdir $@).d\n-dep_args = -MF $(dep_file) -MQ $@ -MMD -MP\n-endif\n-\n-ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)\n-dep_dirs =\n-missing_dep_dirs =\n-dep_args =\n-endif\n-\n compdb_dir = compile_commands\n \n ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n@@ -2489,10 +2472,22 @@ endif\n ASM_SRC := $(wildcard $(OBJECTS:o=S))\n ASM_OBJ := $(ASM_SRC:S=o)\n C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS))\n+C_DEP_OBJ := $(OBJECTS:o=dep)\n \n .SUFFIXES:\n \n-$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)\n+ifeq ($(COMPUTE_HEADER_DEPENDENCIES),yes)\n+$(C_DEP_OBJ): $(GENERATED_H)\n+$(C_DEP_OBJ): %.dep : %.c command-list.h\n+\t$(QUIET_DEP)$(CC) -MF $*.dep -MQ $*.o -MM $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<\n+\n+ALL.dep: $(C_DEP_OBJ)\n+\t$(QUIET_DEP)cat $(C_DEP_OBJ) >$@\n+\n+include ALL.dep\n+endif\n+\n+$(C_OBJ): %.o: %.c %.dep GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)\n \t$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<\n $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)\n \t$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<\n===============================================================================\n\nThis is just stealing a pattern we already use for\nDocumentation/doc.dep.\n\nI.e. here every *.c file depends on a corresponding *.dep file, we\ngenerate the *.dep files and *.c files seperately, instead of making the\n*.dep a \"while we're at it\" as we make the *.o.\n\nIt's on obvious WIP, e.g. there's no ASM_DEP_OBJ yet, and e.g. a\n\"grep.dep\" itself should in turn depend on say \"strbuf.h\", since if we\ntouch strbuf.h we should re-generate the grep.dep file before we make\nthe \"grep.o\" from the \"grep.c\".\n\nIt also means that if you have a clean tree and make \"grep.o\" we'll\ngenerate all the *.dep files and the ALL.dep first, although skipping\nthat could be done with some clever use of $(MAKECMDGOALS) if anyone\ncared.\n\nIt also makes \"git clean -dxf; make -j8 all\" take ~17s on my box instead\nof ~13s.\n\nI wonder if it's worth doing anyway, the cost of doing incremental\ncompiles isn't much affected.\n\nI think the cases where the current schema bites us are rather obscure\nthough, but maybe I haven't thought of some obvious ones.\n\nI.e. if we don't have the dep file we'll make it for the first time when\nmaking the grep.o file, then include those generated dependencies.\n\nIf one of those dependencies changes we'll update the dependencies by\nvirtue of re-making the grep.o. I *think* the only edge cases are if\nyou've created a grep.o with COMPUTE_HEADER_DEPENDENCIES=yes, then\nrecompiled and it's gained a new dependency while using\nCOMPUTE_HEADER_DEPENDENCIES=no, and finally you try to recompile with\nCOMPUTE_HEADER_DEPENDENCIES=yes and it does nothing, because you're\nreferencing a stale \".depends\" directory.\n\nBut that could be solved by just making the grep.o depend on its own\ndependency file, which we don't do now, but could easily do.\n\nSo maybe I've talked myself out of there being an inherent dependency\ngraph violation with the current schema, i.e. the current one seems like\nan easily solved bug.\n\nOne good thing the above approach still gives you is that you can use\nGCC or Clang to make the dependency graph, but then use another compiler\nto compile, i.e. we could have a $(GCC_LIKE) in addition to $(CC), so if\nI'm compiling with xlc or suncc I can still use the present GCC just to\ncreate the dependency graph.\n"},{"id":"436803","messageId":"xmqqo88k40pd.fsf@gitster.g","threadId":"56563","inReplyTo":"YUuPYDkxjDIvIfwI@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-23T00:03:58Z","receivedAt":"2021-09-23T00:04:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I almost suggested using \"git.c\" as the dummy file, since we know it\n> must compile anyway. But that probably has other problems (it's more\n> expensive, and if it _does_ have an error, the results may be\n> confusing).\n>\n> It's a shame we can't just try to do the _real_ compiles using the\n> auto-dependency stuff, and then fall back if they fail. But I think\n> there's a chicken-and-egg problem there with \"make\" doing real work, and\n> figuring out the dependencies to do real work.\n\nYeah, if compiling any of the real sources is inexpensive enough, I\nwould think that would be the happy way to go.  Do we have a trivial\nsource that almost never changes?  Perhaps version.c (especially if\nwe kick out two helper functions that do not really belong there)?\n\n> ...\n> I'm happy to submit that on top, or even turn the earlier hunk into a\n> patch.  But let's see what Ævar has to say to what's been discussed so\n> far. I don't want to derail his effort.\n\nYup.\n"},{"id":"436804","messageId":"CAPUEspikbd_YDM1PaEjLZsHoJtdaqv+AqCdg0KdB_xdJ9sUtFg@mail.gmail.com","threadId":"56563","inReplyTo":"patch-v2-2.2-6b18bd08894-20210922T220532Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/2] Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-23T00:05:48Z","receivedAt":"2021-09-23T00:06:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"FWIW a simpler fix was posted[1] earlier, and you were CC; sorry for the mess\n\nCarlo\n\n[1] https://lore.kernel.org/git/20210922185702.4328-1-carenas@gmail.com/\n"},{"id":"436865","messageId":"YUypOEywhzEWpoef@coredump.intra.peff.net","threadId":"56563","inReplyTo":"xmqqo88k40pd.fsf@gitster.g","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-23T16:20:08Z","receivedAt":"2021-09-23T16:20:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 22, 2021 at 05:03:58PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I almost suggested using \"git.c\" as the dummy file, since we know it\n> > must compile anyway. But that probably has other problems (it's more\n> > expensive, and if it _does_ have an error, the results may be\n> > confusing).\n> >\n> > It's a shame we can't just try to do the _real_ compiles using the\n> > auto-dependency stuff, and then fall back if they fail. But I think\n> > there's a chicken-and-egg problem there with \"make\" doing real work, and\n> > figuring out the dependencies to do real work.\n> \n> Yeah, if compiling any of the real sources is inexpensive enough, I\n> would think that would be the happy way to go.  Do we have a trivial\n> source that almost never changes?  Perhaps version.c (especially if\n> we kick out two helper functions that do not really belong there)?\n\nPerhaps. TBH, I find Ævar's latest patch to just add -Wno-pedantic to be\nthe simplest and most obviously-correct fix.\n\n-Peff\n"},{"id":"436874","messageId":"YUy6QoCG/X9qslim@coredump.intra.peff.net","threadId":"56563","inReplyTo":"8735pw6w8z.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-23T17:32:50Z","receivedAt":"2021-09-23T17:32:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 23, 2021 at 12:40:04AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > It's a shame we can't just try to do the _real_ compiles using the\n> > auto-dependency stuff, and then fall back if they fail. But I think\n> > there's a chicken-and-egg problem there with \"make\" doing real work, and\n> > figuring out the dependencies to do real work.\n> \n> Isn't this just a chicken & egg problem because we've made it a chicken\n> & egg problem? I.e. this WIP hack seems to work for me to avoid it:\n\nPossibly, but...\n\n> -$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)\n> +ifeq ($(COMPUTE_HEADER_DEPENDENCIES),yes)\n> +$(C_DEP_OBJ): $(GENERATED_H)\n> +$(C_DEP_OBJ): %.dep : %.c command-list.h\n> +\t$(QUIET_DEP)$(CC) -MF $*.dep -MQ $*.o -MM $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<\n\nI don't think this last dependency is quite right. The .dep for foo.c\nmust rely not only on foo.c, but also all of its recursive headers (if\nit includes foo.h and then foo.h changes to include bar.h, we need to\nupdate the dep file). So it really needs to depend on everything in its\nown .dep file, too.\n\n> I.e. here every *.c file depends on a corresponding *.dep file, we\n> generate the *.dep files and *.c files seperately, instead of making the\n> *.dep a \"while we're at it\" as we make the *.o.\n> [...]\n> It also makes \"git clean -dxf; make -j8 all\" take ~17s on my box instead\n> of ~13s.\n\nI definitely think that's easier to reason about, but I'm not surprised\nyou found that it was slower. To get a more apples-to-apples comparison,\nI compiled with -j1. I also use -O0 as my default, which makes the\n\"real\" compilation a relatively smaller portion. I got 47s without your\npatch, and 60s with. That seems non-trivial.\n\nI also noticed that \"make clean\" built all the deps. I think that could\nbe fixed, but we're piling fixes upon fixes.\n\nSo I'm not really sure what we're gaining by splitting this all up.\nGiven that our original problem is just fixing the test for \"do we\nsupport computed dependencies\", I think we can solve that pretty easily\nand move on.\n\n> So maybe I've talked myself out of there being an inherent dependency\n> graph violation with the current schema, i.e. the current one seems like\n> an easily solved bug.\n\nYeah, AFAIK the current system is pretty robust and battle-tested. I'd\nbe cautious of messing with it unless there's a big gain to be seen.\n\n> One good thing the above approach still gives you is that you can use\n> GCC or Clang to make the dependency graph, but then use another compiler\n> to compile, i.e. we could have a $(GCC_LIKE) in addition to $(CC), so if\n> I'm compiling with xlc or suncc I can still use the present GCC just to\n> create the dependency graph.\n\nKeep in mind the worst case in not using the dependency graph is just\nthat a header update in an incremental compile may cause some extra\ncompilation. It's really not _that_ bad, and especially so for folks who\naren't actively doing development.\n\n-Peff\n"},{"id":"436875","messageId":"YUy7k3Yo0YeiFk6u@coredump.intra.peff.net","threadId":"56563","inReplyTo":"cover-v2-0.2-00000000000-20210922T220532Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/2] Makefile: \"pedantic\" fallout on .depend and \"compdb\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-23T17:38:27Z","receivedAt":"2021-09-23T17:38:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 23, 2021 at 12:08:00AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> In this v2 I just added an unconditional -Wno-pedantic and omitted the\n> change to spew error on STDERR. This more narrowly fixes the immediate\n> issue and doesn't get into whether we should use /dev/null or whatever\n> as input.\n\nFWIW, this seems perfectly fine to me. Removing $(ALL_CFLAGS) entirely\nseems OK to me, too, but this is a smaller change, and would help any\ncases where those flags are somehow important to getting the compiler to\nfunction at all.\n\nIt's possible some compiler _does_ understand -MF, etc, but not\n-Wno-pedantic, but that seems unlikely to me. (And of course there is\nalways the fallback of setting COMPUTE_HEADER_DEPENDENCIES yourself on\nsuch a system; this is really just about doing the right thing on most\npeople's setups).\n\nAnother related alternative is to use -Wno-error, which would fix the\npedantic problem, along with any other warnings a compiler chooses to\nbring up for an empty file. I don't have a strong opinion on one versus\nthe other.\n\n-Peff\n"},{"id":"436879","messageId":"xmqqpmsz196b.fsf@gitster.g","threadId":"56563","inReplyTo":"YUypOEywhzEWpoef@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-23T17:41:32Z","receivedAt":"2021-09-23T17:41:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Sep 22, 2021 at 05:03:58PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > I almost suggested using \"git.c\" as the dummy file, since we know it\n>> > must compile anyway. But that probably has other problems (it's more\n>> > expensive, and if it _does_ have an error, the results may be\n>> > confusing).\n>> >\n>> > It's a shame we can't just try to do the _real_ compiles using the\n>> > auto-dependency stuff, and then fall back if they fail. But I think\n>> > there's a chicken-and-egg problem there with \"make\" doing real work, and\n>> > figuring out the dependencies to do real work.\n>> \n>> Yeah, if compiling any of the real sources is inexpensive enough, I\n>> would think that would be the happy way to go.  Do we have a trivial\n>> source that almost never changes?  Perhaps version.c (especially if\n>> we kick out two helper functions that do not really belong there)?\n>\n> Perhaps. TBH, I find Ævar's latest patch to just add -Wno-pedantic to be\n> the simplest and most obviously-correct fix.\n\nYes, that is very much to-the-point.  If we have trouble with being\npedantic, forcing us not to be is a fine solution ;-)\n"},{"id":"436898","messageId":"xmqqzgs3x9hv.fsf@gitster.g","threadId":"56563","inReplyTo":"patch-v2-2.2-6b18bd08894-20210922T220532Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/2] Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-23T21:33:32Z","receivedAt":"2021-09-23T21:33:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> The same bug fixed in the \"COMPUTE_HEADER_DEPENDENCIES=auto\" mode in\n> the preceding commit was also present with\n> \"GENERATE_COMPILATION_DATABASE=yes\". Let's fix it so it works again\n> with \"DEVOPTS=1\".\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  Makefile | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/Makefile b/Makefile\n> index 10ea12aae21..94b116d4b37 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1304,6 +1304,7 @@ endif\n>  \n>  ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n>  compdb_check = $(shell $(CC) $(ALL_CFLAGS) \\\n> +\t-Wno-pedantic \\\n\nYup, I like reusing the same approach from the other fix;\nremoving $(ALL_CFLAGS) may work fine, too, and that is what I\nalready queued, but I may swap it out with this later (if I do not\nforget, that is).\n\n>  \t-c -MJ /dev/null \\\n>  \t-x c /dev/null -o /dev/null 2>&1; \\\n>  \techo $$?)\n"}]}