{"thread":{"id":"56565","subject":"[PATCH] Makefile: avoid breaking compilation database generation with DEPELOPER","startedAt":"2021-09-22T18:33:51Z","lastAt":"2021-09-23T02:09:49Z","messageCount":8,"participants":["Carlo Marcelo Arenas Belón","Eric Sunshine","Philippe Blain","Ævar Arnfjörð Bjarmason","brian m. carlson","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"436746","messageId":"20210922183311.3766-1-carenas@gmail.com","threadId":"56565","inReplyTo":null,"subject":"[PATCH] Makefile: avoid breaking compilation database generation with DEPELOPER","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-22T18:33:11Z","receivedAt":"2021-09-22T18:33:51Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"3821c38068 (Makefile: add support for generating JSON compilation\ndatabase, 2020-09-03), adds a feature to be used with clang to generate\na compilation database by copying most of what was done before with the\nheader dependency, but by doing so includes on its availability check\nthe CFLAGS which became specially problematic once DEVELOPER=1 implied\n-pedantic as pointed out by Ævar[1].\n\nRemove the unnecessary flags in the availability test, so it will work\nregardless of which other warnings are enabled or if the compilers has\nbeen told to error on them.\n\n[1] https://lore.kernel.org/git/patch-1.1-6b2e9af5e67-20210922T103749Z-avarab@gmail.com/\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9df565f27b..d5c6d0ea3b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1302,7 +1302,7 @@ GENERATE_COMPILATION_DATABASE = no\n endif\n \n ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n-compdb_check = $(shell $(CC) $(ALL_CFLAGS) \\\n+compdb_check = $(shell $(CC) \\\n \t-c -MJ /dev/null \\\n \t-x c /dev/null -o /dev/null 2>&1; \\\n \techo $$?)\n-- \n2.33.0.911.gbe391d4e11\n\n"},{"id":"436749","messageId":"CAPig+cSTiM-NWLOnSeYEhW8j5POYZ034m-cbMGdROFHp3kWXxQ@mail.gmail.com","threadId":"56565","inReplyTo":"20210922183311.3766-1-carenas@gmail.com","subject":"Re: [PATCH] Makefile: avoid breaking compilation database generation with DEPELOPER","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-22T18:45:24Z","receivedAt":"2021-09-22T18:45:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 22, 2021 at 2:33 PM Carlo Marcelo Arenas Belón\n wrote:\n> Makefile: avoid breaking compilation database generation with DEPELOPER\n\ns/DEPELOPER/DEVELOPER/\n\n> 3821c38068 (Makefile: add support for generating JSON compilation\n> database, 2020-09-03), adds a feature to be used with clang to generate\n> a compilation database by copying most of what was done before with the\n> header dependency, but by doing so includes on its availability check\n> the CFLAGS which became specially problematic once DEVELOPER=1 implied\n> -pedantic as pointed out by Ævar[1].\n>\n> Remove the unnecessary flags in the availability test, so it will work\n> regardless of which other warnings are enabled or if the compilers has\n\ns/compilers/compiler/\n\n> been told to error on them.\n>\n> [1] https://lore.kernel.org/git/patch-1.1-6b2e9af5e67-20210922T103749Z-avarab@gmail.com/\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n"},{"id":"436751","messageId":"20210922185702.4328-1-carenas@gmail.com","threadId":"56565","inReplyTo":"20210922183311.3766-1-carenas@gmail.com","subject":"[PATCH v2] Makefile: avoid breaking compilation database generation with DEVELOPER","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-22T18:57:02Z","receivedAt":"2021-09-22T18:57:13Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"3821c38068 (Makefile: add support for generating JSON compilation\ndatabase, 2020-09-03), adds a feature to be used with clang to generate\na compilation database by copying most of what was done before with the\nheader dependency, but by doing so includes on its availability check\nthe CFLAGS which became specially problematic once DEVELOPER=1 implied\n-pedantic as pointed out by Ævar[1].\n\nRemove the unnecessary flags in the availability test, so it will work\nregardless of which other warnings are enabled or if the compiler has\nbeen told to error on them.\n\n[1] https://lore.kernel.org/git/patch-1.1-6b2e9af5e67-20210922T103749Z-avarab@gmail.com/\n\nHelped-by: Eric Sunshine \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9df565f27b..d5c6d0ea3b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1302,7 +1302,7 @@ GENERATE_COMPILATION_DATABASE = no\n endif\n \n ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n-compdb_check = $(shell $(CC) $(ALL_CFLAGS) \\\n+compdb_check = $(shell $(CC) \\\n \t-c -MJ /dev/null \\\n \t-x c /dev/null -o /dev/null 2>&1; \\\n \techo $$?)\n-- \n2.33.0.911.gbe391d4e11\n\n"},{"id":"436754","messageId":"0e6f9744-e5b2-6afc-cc67-60cc838b07b1@gmail.com","threadId":"56565","inReplyTo":"20210922185702.4328-1-carenas@gmail.com","subject":"Re: [PATCH v2] Makefile: avoid breaking compilation database generation with DEVELOPER","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-09-22T19:07:10Z","receivedAt":"2021-09-22T19:07:14Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Carlo,\n\nLe 2021-09-22 à 14:57, Carlo Marcelo Arenas Belón a écrit :\n> 3821c38068 (Makefile: add support for generating JSON compilation\n> database, 2020-09-03), adds a feature to be used with clang to generate\n> a compilation database by copying most of what was done before with the\n> header dependency, but by doing so includes on its availability check\n> the CFLAGS which became specially problematic once DEVELOPER=1 implied\n> -pedantic as pointed out by Ævar[1].\n> \n> Remove the unnecessary flags in the availability test, so it will work\n> regardless of which other warnings are enabled or if the compiler has\n> been told to error on them.\n> \n> [1] https://lore.kernel.org/git/patch-1.1-6b2e9af5e67-20210922T103749Z-avarab@gmail.com/\n> \n> Helped-by: Eric Sunshine \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> Makefile | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/Makefile b/Makefile\n> index 9df565f27b..d5c6d0ea3b 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1302,7 +1302,7 @@ GENERATE_COMPILATION_DATABASE = no\n> endif\n> \n> ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n> -compdb_check = $(shell $(CC) $(ALL_CFLAGS) \\\n> +compdb_check = $(shell $(CC) \\\n> \t-c -MJ /dev/null \\\n> \t-x c /dev/null -o /dev/null 2>&1; \\\n> \techo $$?)\n\nThanks for cleaning that up.\n\nAcked-by: Philippe Blain \n\nPhilippe.\n"},{"id":"436809","messageId":"87tuic5cdo.fsf@evledraar.gmail.com","threadId":"56565","inReplyTo":"20210922183311.3766-1-carenas@gmail.com","subject":"Re: [PATCH] Makefile: avoid breaking compilation database generation with DEPELOPER","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-23T00:55:36Z","receivedAt":"2021-09-23T01:06:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 22 2021, Carlo Marcelo Arenas Belón wrote:\n\n> 3821c38068 (Makefile: add support for generating JSON compilation\n> database, 2020-09-03), adds a feature to be used with clang to generate\n> a compilation database by copying most of what was done before with the\n> header dependency, but by doing so includes on its availability check\n> the CFLAGS which became specially problematic once DEVELOPER=1 implied\n> -pedantic as pointed out by Ævar[1].\n>\n> Remove the unnecessary flags in the availability test, so it will work\n> regardless of which other warnings are enabled or if the compilers has\n> been told to error on them.\n>\n> [1] https://lore.kernel.org/git/patch-1.1-6b2e9af5e67-20210922T103749Z-avarab@gmail.com/\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> Makefile | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 9df565f27b..d5c6d0ea3b 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1302,7 +1302,7 @@ GENERATE_COMPILATION_DATABASE = no\n> endif\n> \n> ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n> -compdb_check = $(shell $(CC) $(ALL_CFLAGS) \\\n> +compdb_check = $(shell $(CC) \\\n> \t-c -MJ /dev/null \\\n> \t-x c /dev/null -o /dev/null 2>&1; \\\n> \techo $$?)\n\nSorry about the overlap in\nhttps://lore.kernel.org/git/patch-v2-2.2-6b18bd08894-20210922T220532Z-avarab@gmail.com/;\nI didn't see this thread before sending my version.\n\nI think your patch here is better than mine. FWIW I also had this on top\nof mine, i.e. emitting output to stderr unconditionally:\nhttps://github.com/avar/git/commit/d4bcc0e617e52df803870df29df82aa3b2205d84\n\nBut thinking about it again I think with the rationale in that\nnot-on-list commit of mine the below is better than either of our\nversions v1.\n\nI.e. for COMPUTE_HEADER_DEPENDENCIES the point of the test is that we\nturn it on automatically, so it needs to not suck by default. The reason\nwe're doing this is, per the comment in 3821c38068:\n\n If this variable is set, check that $(CC) indeed supports the `-MJ`\n flag, following what is done for automatic dependencies.\n\nAnyone using GENERATE_COMPILATION_DATABASE is turning it on explicitly,\nand I daresay if they're using it at all they're either not using\nanything but clang, or is keenly aware of the difference.\n\nSo do we really need to carry those 17 lines of the Makefile logic\nsimply to avoid showing this error on say \"CC=gcc\nGENERATE_COMPILATION_DATABASE=yes\":\n\n gcc: error: unrecognized command-line option ‘-MJ’; did you mean ‘-J’?\n\nIt doesn't seem worth it to me, especially as we document that we'll use\nthe \"-MJ\" flag in the Makefile comment that the person turning on\nGENERATE_COMPILATION_DATABASE=yes must have read.\n\nAnyway, I'll leave you to do what you think is best here, and I'm also\nfine with just going for the v1 you've got here, it just seems to me\nlike we're both fixing logic that's been copy/pasted from\nCOMPUTE_HEADER_DEPENDENCIES, and the reasons we need it for that\nfacility don't apply at all to GENERATE_COMPILATION_DATABASE.\n\n-- >8 --\ndiff --git a/Makefile b/Makefile\nindex 9df565f27bb..32538f9e858 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1301,23 +1301,6 @@ ifndef GENERATE_COMPILATION_DATABASE\n GENERATE_COMPILATION_DATABASE = no\n endif\n \n-ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n-compdb_check = $(shell $(CC) $(ALL_CFLAGS) \\\n-\t-c -MJ /dev/null \\\n-\t-x c /dev/null -o /dev/null 2>&1; \\\n-\techo $$?)\n-ifneq ($(compdb_check),0)\n-override GENERATE_COMPILATION_DATABASE = no\n-$(warning GENERATE_COMPILATION_DATABASE is set to \"yes\", but your compiler does not \\\n-support generating compilation database entries)\n-endif\n-else\n-ifneq ($(GENERATE_COMPILATION_DATABASE),no)\n-$(error please set GENERATE_COMPILATION_DATABASE to \"yes\" or \"no\" \\\n-(not \"$(GENERATE_COMPILATION_DATABASE)\"))\n-endif\n-endif\n-\n ifdef SANE_TOOL_PATH\n SANE_TOOL_PATH_SQ = $(subst ','\\'',$(SANE_TOOL_PATH))\n BROKEN_PATH_FIX = 's|^\\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix \"$(SANE_TOOL_PATH_SQ)\"|'\n\n\n"},{"id":"436813","messageId":"YUvbU9HX6JA+MKd0@camp.crustytoothpaste.net","threadId":"56565","inReplyTo":"20210922185702.4328-1-carenas@gmail.com","subject":"Re: [PATCH v2] Makefile: avoid breaking compilation database generation with DEVELOPER","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-09-23T01:41:39Z","receivedAt":"2021-09-23T01:41:47Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-09-22 at 18:57:02, Carlo Marcelo Arenas Belón wrote:\n> 3821c38068 (Makefile: add support for generating JSON compilation\n> database, 2020-09-03), adds a feature to be used with clang to generate\n> a compilation database by copying most of what was done before with the\n> header dependency, but by doing so includes on its availability check\n> the CFLAGS which became specially problematic once DEVELOPER=1 implied\n> -pedantic as pointed out by Ævar[1].\n> \n> Remove the unnecessary flags in the availability test, so it will work\n> regardless of which other warnings are enabled or if the compiler has\n> been told to error on them.\n> \n> [1] https://lore.kernel.org/git/patch-1.1-6b2e9af5e67-20210922T103749Z-avarab@gmail.com/\n> \n> Helped-by: Eric Sunshine \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> Makefile | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/Makefile b/Makefile\n> index 9df565f27b..d5c6d0ea3b 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1302,7 +1302,7 @@ GENERATE_COMPILATION_DATABASE = no\n> endif\n> \n> ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n> -compdb_check = $(shell $(CC) $(ALL_CFLAGS) \\\n> +compdb_check = $(shell $(CC) \\\n> \t-c -MJ /dev/null \\\n> \t-x c /dev/null -o /dev/null 2>&1; \\\n> \techo $$?)\n\nAre you sure this results in a functional set of files? As I understand\nit, the reason that clangd needs these files is because it needs to know\nwhat include arguments and headers are supposed to be used, since C\nprograms don't have a standard layout. In this case, it looks like\nyou're removing all of the -I arguments, so in that case clangd wouldn't\nbe able to find all the files it's supposed to.\n\nOf course, if I've misunderstood, and somehow we get those arguments\nelsewhere, that's fine, but I just want to be sure we don't regress the\nbehavior.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"436814","messageId":"CAPUEspjJdkPgpWvLJKNks+Uu-tZmahw6U3pa+XBkMB6_m_dtRQ@mail.gmail.com","threadId":"56565","inReplyTo":"YUvbU9HX6JA+MKd0@camp.crustytoothpaste.net","subject":"Re: [PATCH v2] Makefile: avoid breaking compilation database generation with DEVELOPER","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-23T01:59:26Z","receivedAt":"2021-09-23T01:59:40Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Sep 22, 2021 at 6:41 PM brian m. carlson\n wrote:\n\n> > diff --git a/Makefile b/Makefile\n> > index 9df565f27b..d5c6d0ea3b 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -1302,7 +1302,7 @@ GENERATE_COMPILATION_DATABASE = no\n> > endif\n> >\n> > ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n> > -compdb_check = $(shell $(CC) $(ALL_CFLAGS) \\\n> > +compdb_check = $(shell $(CC) \\\n> > -c -MJ /dev/null \\\n> > -x c /dev/null -o /dev/null 2>&1; \\\n> > echo $$?)\n>\n> Are you sure this results in a functional set of files?\n\nno; it does not\n\nThis call is only meant to be used to check if your compiler supports\nthe feature (which as Ævar points out[1], might not be the best thing\nto do in this case), though\n\nAfter this fix the files are being generated (in a different place\nwith their expected flags) and look healthy, but would be helpful to\nknow you see no regressions.\n\nCarlo\n\n[1] https://lore.kernel.org/git/87tuic5cdo.fsf@evledraar.gmail.com/\n"},{"id":"436819","messageId":"87czp059g7.fsf@evledraar.gmail.com","threadId":"56565","inReplyTo":"CAPUEspjJdkPgpWvLJKNks+Uu-tZmahw6U3pa+XBkMB6_m_dtRQ@mail.gmail.com","subject":"Re: [PATCH v2] Makefile: avoid breaking compilation database generation with DEVELOPER","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-23T02:08:29Z","receivedAt":"2021-09-23T02:09:49Z","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 6:41 PM brian m. carlson\n> wrote:\n>\n>> > diff --git a/Makefile b/Makefile\n>> > index 9df565f27b..d5c6d0ea3b 100644\n>> > --- a/Makefile\n>> > +++ b/Makefile\n>> > @@ -1302,7 +1302,7 @@ GENERATE_COMPILATION_DATABASE = no\n>> > endif\n>> >\n>> > ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n>> > -compdb_check = $(shell $(CC) $(ALL_CFLAGS) \\\n>> > +compdb_check = $(shell $(CC) \\\n>> > -c -MJ /dev/null \\\n>> > -x c /dev/null -o /dev/null 2>&1; \\\n>> > echo $$?)\n>>\n>> Are you sure this results in a functional set of files?\n>\n> no; it does not\n>\n> This call is only meant to be used to check if your compiler supports\n> the feature (which as Ævar points out[1], might not be the best thing\n> to do in this case), though\n>\n> After this fix the files are being generated (in a different place\n> with their expected flags) and look healthy, but would be helpful to\n> know you see no regressions.\n\nI had the same thought as brian, but you're right, since we never use\nthe result of this it's OK.\n\nIOW this check is really functionally equivalent to something like:\n\n cc --help | grep -q -F -- -MJ\n\nOr whatever, i.e. we're just checking if it's clang & supports the -MJ\noption.\n\n> [1] https://lore.kernel.org/git/87tuic5cdo.fsf@evledraar.gmail.com/\n\n"}]}