{"thread":{"id":"49368","subject":"[PATCH 1/9] Makefile: add a hdr-check target","startedAt":"2018-09-19T00:07:12Z","lastAt":"2018-09-20T18:49:55Z","messageCount":7,"participants":["Ramsay Jones","Martin Ågren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"358425","messageId":"d24df21a-7ab2-84f6-8b18-83fd9c8c2b30@ramsayjones.plus.com","threadId":"49368","inReplyTo":null,"subject":"[PATCH 1/9] Makefile: add a hdr-check target","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-09-19T00:07:08Z","receivedAt":"2018-09-19T00:07:12Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nCommit ef3ca95475 (\"Add missing includes and forward declarations\",\n2018-08-15) resulted from the author employing a manual method to\ncreate a C file consisting of a pair of pre-processor #include\nlines (for 'git-compat-util.h' and a given toplevel header), and\nfixing any resulting compiler errors or warnings.\n\nAdd a Makefile target to automate this process. This implementation\nrelies on the '-include' and '-xc' arguments to the 'gcc' and 'clang'\ncompilers, which allows us to effectively create the required C\ncompilation unit on-the-fly. This limits the portability of this\nsolution to those systems which have such a compiler.\n\nThe new 'hdr-check' target can be used to check most header files in\nthe project (for various reasons, the 'compat' and 'xdiff' directories\nare not included). Also, note that individual header files can be\nchecked directly using the '.hco' extension (read: Hdr-Check Object)\nlike so:\n\n    $ make config.hco\n        HDR config.h\n    $\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n Makefile | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex b567ccca45..835030e22b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1793,6 +1793,7 @@ ifndef V\n \tQUIET_MSGFMT   = @echo '   ' MSGFMT $@;\n \tQUIET_GCOV     = @echo '   ' GCOV $@;\n \tQUIET_SP       = @echo '   ' SP $<;\n+\tQUIET_HDR      = @echo '   ' HDR $<;\n \tQUIET_RC       = @echo '   ' RC $@;\n \tQUIET_SUBDIR0  = +@subdir=\n \tQUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \\\n@@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n .PHONY: sparse $(SP_OBJ)\n sparse: $(SP_OBJ)\n \n+GEN_HDRS := command-list.h unicode-width.h\n+EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%\n+CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n+HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n+\n+$(HCO): %.hco: %.h FORCE\n+\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n+\n+.PHONY: hdr-check $(HCO)\n+hdr-check: $(HCO)\n+\n .PHONY: style\n style:\n \tgit clang-format --style file --diff --extensions c,h\n-- \n2.19.0\n"},{"id":"358470","messageId":"CAN0heSpNT25PrNwdbQKZo=Q8kpSfwnzuOz34xKHtfF+Fq9ZXmg@mail.gmail.com","threadId":"49368","inReplyTo":"d24df21a-7ab2-84f6-8b18-83fd9c8c2b30@ramsayjones.plus.com","subject":"Re: [PATCH 1/9] Makefile: add a hdr-check target","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-09-19T17:49:57Z","receivedAt":"2018-09-19T17:50:11Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Ramsay,\n\nOn Wed, 19 Sep 2018 at 02:07, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:\n> @@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n>  .PHONY: sparse $(SP_OBJ)\n>  sparse: $(SP_OBJ)\n>\n> +GEN_HDRS := command-list.h unicode-width.h\n\nMost of the things happening around here are obvious steps towards the\nend-goal and seem to logically belong here, together. This one though,\n\"generated headers\"(?) seems like it is more general in nature, so could\nperhaps live somewhere else.\n\nActually, we have a variable `GENERATED_H` which seems to try to do more\nor less the same thing. It lists just one file, though, command-list.h.\nAnd unicode-width.h seems to be tracked in git.git.\n\nMaybe use `GENERATED_H` instead, and list unicode-width.h on the next\nline instead? Or am I completely misreading \"GEN_HDRS\"?\n\n> +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%\n> +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n> +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n> +\n> +$(HCO): %.hco: %.h FORCE\n> +       $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n> +\n> +.PHONY: hdr-check $(HCO)\n> +hdr-check: $(HCO)\n> +\n>  .PHONY: style\n>  style:\n>         git clang-format --style file --diff --extensions c,h\n"},{"id":"358481","messageId":"89478060-85c1-416a-0ad9-54d3f8f1111b@ramsayjones.plus.com","threadId":"49368","inReplyTo":"CAN0heSpNT25PrNwdbQKZo=Q8kpSfwnzuOz34xKHtfF+Fq9ZXmg@mail.gmail.com","subject":"Re: [PATCH 1/9] Makefile: add a hdr-check target","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-09-19T19:37:29Z","receivedAt":"2018-09-19T19:37:33Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 19/09/18 18:49, Martin Ågren wrote:\n> Hi Ramsay,\n> \n> On Wed, 19 Sep 2018 at 02:07, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:\n>> @@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n>>  .PHONY: sparse $(SP_OBJ)\n>>  sparse: $(SP_OBJ)\n>>\n>> +GEN_HDRS := command-list.h unicode-width.h\n> \n> Most of the things happening around here are obvious steps towards the\n> end-goal and seem to logically belong here, together. This one though,\n> \"generated headers\"(?) seems like it is more general in nature, so could\n> perhaps live somewhere else.\n\nYes, generated headers, but no, not more general. ;-)\n\nI originally included those headers directly in the\nEXCEPT_HDRS macro, along with the list of excluded\ndirectories (which was much longer at one point).\n\nThe 'command-list.h' is generated as part of the build\n(and so may or may not be part of the LIB_H macro), whereas\nthe unicode-width.h header is only re-generated when someone\nruns the 'contrib/update-unicode/update_unicode.sh' script\n(presumably when a new standard version is announced) and\ncommits the result.\n\nBoth headers fail the 'hdr-check', although both generator\nscripts could be 'fixed' so that they passed. I just didn't\nthink it was worth the effort - neither header was likely\nto be #included anywhere else. I guess I could eliminate\nthat macro, absorbing it into EXCEPT_HDRS, if that would\nlead to less confusion ...\n\n[I suspect the fact that LIB_H (almost always) contains\n'command-list.h' has not been noticed ... :-P ]\n\n> Actually, we have a variable `GENERATED_H` which seems to try to do more\n> or less the same thing. It lists just one file, though, command-list.h.\n> And unicode-width.h seems to be tracked in git.git.\n\nHmm, GENERATED_H seems only to be used by the i18n part of the\nmakefile and, as a result of its appearance in LIB_H, sometimes\nresults in command-list.h appearing twice in LOCALIZED_C.\n(which is probably not a problem).\n\nATB,\nRamsay Jones\n\n> Maybe use `GENERATED_H` instead, and list unicode-width.h on the next\n> line instead? Or am I completely misreading \"GEN_HDRS\"?\n> \n>> +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%\n>> +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n>> +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n>> +\n>> +$(HCO): %.hco: %.h FORCE\n>> +       $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n>> +\n>> +.PHONY: hdr-check $(HCO)\n>> +hdr-check: $(HCO)\n>> +\n>>  .PHONY: style\n>>  style:\n>>         git clang-format --style file --diff --extensions c,h\n> \n"},{"id":"358500","messageId":"CAN0heSq8U7ue-SB-1iP1ZdW2hgrR0fwuKh_q68KvJ4K1dphpCw@mail.gmail.com","threadId":"49368","inReplyTo":"89478060-85c1-416a-0ad9-54d3f8f1111b@ramsayjones.plus.com","subject":"Re: [PATCH 1/9] Makefile: add a hdr-check target","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-09-20T05:53:32Z","receivedAt":"2018-09-20T05:53:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 19 Sep 2018 at 22:15, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:\n> On 19/09/18 18:49, Martin Ågren wrote:\n> > On Wed, 19 Sep 2018 at 02:07, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:\n> >> +GEN_HDRS := command-list.h unicode-width.h\n> >\n> > Most of the things happening around here are obvious steps towards the\n> > end-goal and seem to logically belong here, together. This one though,\n> > \"generated headers\"(?) seems like it is more general in nature, so could\n> > perhaps live somewhere else.\n>\n> Yes, generated headers, but no, not more general. ;-)\n\n> The 'command-list.h' is generated as part of the build\n> (and so may or may not be part of the LIB_H macro), whereas\n> the unicode-width.h header is only re-generated when someone\n> runs the 'contrib/update-unicode/update_unicode.sh' script\n> (presumably when a new standard version is announced) and\n> commits the result.\n\nAh, I wasn't aware of how unicode-width.h works, thanks.\n\n> Both headers fail the 'hdr-check', although both generator\n> scripts could be 'fixed' so that they passed. I just didn't\n> think it was worth the effort - neither header was likely\n> to be #included anywhere else.\n\nWith the above background, I'd tend to agree.\n\n> I guess I could eliminate\n> that macro, absorbing it into EXCEPT_HDRS, if that would\n> lead to less confusion ...\n\nI'm just a single data point, so don't trust my experience too much.\n\n> > Actually, we have a variable `GENERATED_H` which seems to try to do more\n> > or less the same thing. It lists just one file, though, command-list.h.\n>\n> Hmm, GENERATED_H seems only to be used by the i18n part of the\n> makefile and, as a result of its appearance in LIB_H, sometimes\n> results in command-list.h appearing twice in LOCALIZED_C.\n\nJust thinking out loud, I suppose you could use $(GENERATED_H) instead\nof hard-coding command-list.h in your patch. But with the background you\nexplained above, I think there's a good counter-argument to be made:\n\nWhen we gain new auto-generated headers, we wouldn't actually mind `make\nhdr-check` failing. We'd get the opportunity to decide whether making\nthe new header pass the check is worth it, or whether the correct\nsolution is to blacklist the auto-generated header. That's probably\nbetter than just blacklisting the new header by default so that we don't\neven notice that it has a potential problem.\n\nSo I think your approach makes sense. I stumbled on the similarity of\nthe name to something we already have. Maybe something like\n\n  # Ignore various generated files, rather than updating the\n  # generating scripts for little or no benefit.\n  GEN_HDRS := ...\n\nor a remark in the commit message, or rolling this into EXCEPT_HDRS, but\nI'd be perfectly fine just leaving this as it is. Please trust your own\njudgment more than mine.\n\nThanks\nMartin\n"},{"id":"358512","messageId":"xmqqbm8s2qjd.fsf@gitster-ct.c.googlers.com","threadId":"49368","inReplyTo":"d24df21a-7ab2-84f6-8b18-83fd9c8c2b30@ramsayjones.plus.com","subject":"Re: [PATCH 1/9] Makefile: add a hdr-check target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-20T14:26:46Z","receivedAt":"2018-09-20T14:26:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> Commit ef3ca95475 (\"Add missing includes and forward declarations\",\n> 2018-08-15) resulted from the author employing a manual method to\n> create a C file consisting of a pair of pre-processor #include\n> lines (for 'git-compat-util.h' and a given toplevel header), and\n> fixing any resulting compiler errors or warnings.\n\nIt makes sense to have tool do what we do not have to do manually.\n\nOne thing that makes me wonder with the patch is that the new check\ncommand does not seem to need to see what is on CFLAGS and friends.\nHaving seen that \"make DEVELOPER=1\" adds more -W... on the command\nline of the compiler and makes a build fail on a source that\notherwise would build, I am wondering if there are some (subset of)\noptions that the header-check command line wants to give to the\ncompiler.\n\nOf course, there are also conditionally compiled sections of code,\nwhich are affected by the choice of -DMACRO=VALUE; how does this new\nfeature handle that?\n\n> Add a Makefile target to automate this process. This implementation\n> relies on the '-include' and '-xc' arguments to the 'gcc' and 'clang'\n> compilers, which allows us to effectively create the required C\n> compilation unit on-the-fly. This limits the portability of this\n> solution to those systems which have such a compiler.\n"},{"id":"358516","messageId":"b7b9b26d-c175-05b9-7144-101bcaf592d4@ramsayjones.plus.com","threadId":"49368","inReplyTo":"xmqqbm8s2qjd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/9] Makefile: add a hdr-check target","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-09-20T16:03:05Z","receivedAt":"2018-09-20T16:03:10Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 20/09/18 15:26, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n>> Commit ef3ca95475 (\"Add missing includes and forward declarations\",\n>> 2018-08-15) resulted from the author employing a manual method to\n>> create a C file consisting of a pair of pre-processor #include\n>> lines (for 'git-compat-util.h' and a given toplevel header), and\n>> fixing any resulting compiler errors or warnings.\n> \n> It makes sense to have tool do what we do not have to do manually.\n> \n> One thing that makes me wonder with the patch is that the new check\n> command does not seem to need to see what is on CFLAGS and friends.\n> Having seen that \"make DEVELOPER=1\" adds more -W... on the command\n> line of the compiler and makes a build fail on a source that\n> otherwise would build, I am wondering if there are some (subset of)\n> options that the header-check command line wants to give to the\n> compiler.\n\nYes, this was one of my first concerns (I even asked Elijah what\ncompiler options he used), but I was getting useful results without\npassing CFLAGS, so I just ignored that issue ... :-D\n\n[The 'on-the-fly' compilation units don't correspond to any _actual_\ncompilation unit, so it's not easy to use existing rules ... but we\ncould use 'hco' rule specific definitions to add flags, I suppose ...]\n\n> Of course, there are also conditionally compiled sections of code,\n> which are affected by the choice of -DMACRO=VALUE; how does this new\n> feature handle that?\n\nIndeed. This bothered me as well. The 'compat' directory does not\nfollow the 'usual pattern' of the main headers and is particularly\nsensitive to the lack of various -DMACROs. I had initially included\n_all_ sub-directories in the 'exclude list' (to follow what Elijah\nhad done), but then removed one at a time ...\n\nI am open to suggestions for improvements. ;-)\n\nATB,\nRamsay Jones\n\n\n\n"},{"id":"358530","messageId":"xmqqpnx8ypf5.fsf@gitster-ct.c.googlers.com","threadId":"49368","inReplyTo":"b7b9b26d-c175-05b9-7144-101bcaf592d4@ramsayjones.plus.com","subject":"Re: [PATCH 1/9] Makefile: add a hdr-check target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-20T18:49:50Z","receivedAt":"2018-09-20T18:49:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> Yes, this was one of my first concerns (I even asked Elijah what\n> compiler options he used), but I was getting useful results without\n> passing CFLAGS, so I just ignored that issue ... :-D\n> ...\n> Indeed. This bothered me as well. The 'compat' directory does not\n> follow the 'usual pattern' of the main headers and is particularly\n> sensitive to the lack of various -DMACROs. I had initially included\n> _all_ sub-directories in the 'exclude list' (to follow what Elijah\n> had done), but then removed one at a time ...\n>\n> I am open to suggestions for improvements. ;-)\n\nPerhaps it is sufficient to add a mention of these two issues in log\nmessage and an in-code comment nearby the .hco rule in Makefile, so\nthat people can come back later to discover what design choice we\nmade (i.e. \"without worrying about these two issues, we are getting\nuseful enough results, so we decided it is OK at least for now not\nto worry about them\").\n\nThanks.\n"}]}