{"thread":{"id":"52453","subject":"[PATCH] Makefile: drop GEN_HDRS","startedAt":"2019-12-13T23:25:48Z","lastAt":"2019-12-19T04:51:26Z","messageCount":12,"participants":["Junio C Hamano","Jeff King","Emily Shaffer","Jonathan Nieder","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"388184","messageId":"xmqq1rt7hkp6.fsf@gitster-ct.c.googlers.com","threadId":"52453","inReplyTo":null,"subject":"[PATCH] Makefile: drop GEN_HDRS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-13T23:25:41Z","receivedAt":"2019-12-13T23:25:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When ebb7baf0 (\"Makefile: add a hdr-check target\", 2018-09-19)\nimplemented hdr-check target, it wanted to leave some header files\nexempt from the stricter check the target implements, and added\nGEN_HDRS macro to list them\n\nThis however was probably a bad move for two and half reasons:\n\n - If we value the header cleanliness check, we eventually want to\n teach our header generating scripts to produce clean headers.\n Keeping the blanket \"generated headers can be left as dirty as we\n want\" exception does not nudge us in the right direction.\n\n - There is a list of generated header files, GENERATED_H, which is\n used to keep track of dependencies. Presence of GEN_HDRS that is\n too similarly named would confuse developers who are adding new\n generated header files which list to add theirs.\n\n - Even though unicode-width.h could be generated using a contrib/\n script, as far as our build infrastructure is concerned, it is a\n source file that is tracked in the source control system. Its\n presence in GEN_HDRS list is doubly misleading.\n\nGet rid of GEN_HDRS, which is used only once to list the headers we\ndo not run hdr-check test on, and instead explicitly list that the\nones, either tracked or generated, that we exempt from the test.\n\nThis allows GENERATED_H to be the sole \"here are build artifact\nheader files that are expendable\" list, so use it in the clean\ntarget to $(RM) them.\n\nSigned-off-by: Junio C Hamano \n---\n Makefile | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex b7d7374dac..9a9d35637d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2779,8 +2779,7 @@ $(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+EXCEPT_HDRS := command-list.h unicode-width.h compat/% xdiff/%\n ifndef GCRYPT_SHA256\n \tEXCEPT_HDRS += sha256/gcrypt.h\n endif\n@@ -3105,7 +3104,7 @@ clean: profile-clean coverage-clean cocciclean\n \t$(RM) $(HCC)\n \t$(RM) -r bin-wrappers $(dep_dirs)\n \t$(RM) -r po/build/\n-\t$(RM) *.pyc *.pyo */*.pyc */*.pyo command-list.h $(ETAGS_TARGET) tags cscope*\n+\t$(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope*\n \t$(RM) -r $(GIT_TARNAME) .doc-tmp-dir\n \t$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz\n \t$(RM) $(htmldocs).tar.gz $(manpages).tar.gz\n-- \n2.24.1-672-gdc02b1e6c6\n\n"},{"id":"388200","messageId":"20191214003820.GA927924@coredump.intra.peff.net","threadId":"52453","inReplyTo":"xmqq1rt7hkp6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Makefile: drop GEN_HDRS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-14T00:38:20Z","receivedAt":"2019-12-14T00:38:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 13, 2019 at 03:25:41PM -0800, Junio C Hamano wrote:\n\n> Get rid of GEN_HDRS, which is used only once to list the headers we\n> do not run hdr-check test on, and instead explicitly list that the\n> ones, either tracked or generated, that we exempt from the test.\n\nYeah, I think this is an improvement by itself.\n\nAfter reading this, though:\n\n> - If we value the header cleanliness check, we eventually want to\n> teach our header generating scripts to produce clean headers.\n> Keeping the blanket \"generated headers can be left as dirty as we\n> want\" exception does not nudge us in the right direction.\n\nI did expect to see the actual hdr-check behavior move towards checking\nthese generated versions. However, both are kind of interesting.\n\nunicode-width.h isn't a \"real\" header file; it's meant to be included in\nthe middle of a function. I think it _could_ be changed to define\n\"struct interval\" itself, and then be a static file-scope variable. But\nthere's not really a compelling reason to do so.\n\nBut \"command-list.h\" is more of a traditional header file, being\nincluded at the top of help.c. In theory the hdr-check target could add\na dependency on it, and then we could check it along with everything\nelse. But even without that first step, if I remove it from EXCEPT_HDRS,\nnothing happens!\n\nThat's because LIB_H is created by running find in the local filesystem.\nSo until it's generated, we don't realize it's there to check. I kind of\nwonder if it should be part of LIB_H. I suspect that on some systems,\nwe'd fail to notice a rebuild when command-list.txt is updated (but\nnobody noticed, because it is only systems that do not have\ncompiler-supported dependency tracking, and most developers are no\nmodern platforms that do).\n\n-Peff\n"},{"id":"388201","messageId":"20191214010002.GA945704@coredump.intra.peff.net","threadId":"52453","inReplyTo":"20191214003820.GA927924@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: drop GEN_HDRS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-14T01:00:02Z","receivedAt":"2019-12-14T01:01:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 13, 2019 at 07:38:21PM -0500, Jeff King wrote:\n\n> That's because LIB_H is created by running find in the local filesystem.\n> So until it's generated, we don't realize it's there to check. I kind of\n> wonder if it should be part of LIB_H. I suspect that on some systems,\n> we'd fail to notice a rebuild when command-list.txt is updated (but\n> nobody noticed, because it is only systems that do not have\n> compiler-supported dependency tracking, and most developers are no\n> modern platforms that do).\n\nActually, this probably isn't true. We have an explicit dependency for\nhelp.o on command-list.h, so it would get built properly then.\n\nIts inclusion in LIB_H is still wonky, though. It sometimes is included\nand sometimes not, depending on whether ls-files or find is used.\n\n-Peff\n"},{"id":"388290","messageId":"xmqqlfrcje1f.fsf@gitster-ct.c.googlers.com","threadId":"52453","inReplyTo":"20191214010002.GA945704@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: drop GEN_HDRS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-16T18:55:40Z","receivedAt":"2019-12-16T18:55:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Dec 13, 2019 at 07:38:21PM -0500, Jeff King wrote:\n>\n>> That's because LIB_H is created by running find in the local filesystem.\n>> So until it's generated, we don't realize it's there to check. I kind of\n>> wonder if it should be part of LIB_H. I suspect that on some systems,\n>> we'd fail to notice a rebuild when command-list.txt is updated (but\n>> nobody noticed, because it is only systems that do not have\n>> compiler-supported dependency tracking, and most developers are no\n>> modern platforms that do).\n>\n> Actually, this probably isn't true. We have an explicit dependency for\n> help.o on command-list.h, so it would get built properly then.\n>\n> Its inclusion in LIB_H is still wonky, though. It sometimes is included\n> and sometimes not, depending on whether ls-files or find is used.\n\nAs long as GENERATED_H is maintained properly to list headers that\nare actually used (e.g. if we ever start creating and using a header\nonly when some Makefile macro tells us to, we make sure to place the\nheader in GENERATED_H only when we create and use it), I think we\nshould just add it to LIB_H, regardless of what is tracked.\n\nLIB_H could contain command-list.h (and other GENERATED_H files) if\nwe did this, but dups in dependency does not hurt in general, and I\ndid not find anything potentially problematic in the existing use of\n$(LIB_H) in our Makefile.\n\nHow about doing this as a further clean-up? I am reasonably sure\nthe status-quo description is correct, but I find the justification\na bit weak (in other words, I do not have a good answer to \"who\ncares if those that depend on $(LIB_H) are not rebuilt when\ncommand-list.h gets rebuilt?\")\n\n--- >8 ---\nMakefile: include GENERATED_H in LIB_H\n\n$(LIB_H), which is meant to be the list of header files that can\naffect (hence trigger recompilation) the objects that go in\nlibgit.a, in a directory extracted from a tarball is computed by\nrunning \"find \\*.h\" but instead computed with \"ls-files \\*.h\" in a\nworking tree managed by a git repository. The former can include\ngenerated header files after a build, and omit them in a clean\nstate. The latter would not, as generated header files are by\ndefinition not tracked.\n\nExplicitly add $(GENERATED_H) to $(LIB_H) to make things consistent.\n\nSigned-off-by: Junio C Hamano \n---\n Makefile | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9a9d35637d..552c43c3d8 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -822,6 +822,7 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat\n \t-name t -prune -o \\\n \t-name Documentation -prune -o \\\n \t-name '*.h' -print)))\n+LIB_H += $(GENERATED_H)\n \n LIB_OBJS += abspath.o\n LIB_OBJS += add-interactive.o\n@@ -2399,7 +2400,7 @@ else\n # should _not_ be included here, since they are necessary even when\n # building an object for the first time.\n \n-$(OBJECTS): $(LIB_H) $(GENERATED_H)\n+$(OBJECTS): $(LIB_H)\n endif\n \n exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX\n@@ -2521,7 +2522,7 @@ XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \\\n \t--keyword=gettextln --keyword=eval_gettextln\n XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \\\n \t--keyword=__ --keyword=N__ --keyword=\"__n:1,2\"\n-LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)\n+LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H)\n LOCALIZED_SH = $(SCRIPT_SH)\n LOCALIZED_SH += git-parse-remote.sh\n LOCALIZED_SH += git-rebase--preserve-merges.sh\n\n\n"},{"id":"388294","messageId":"20191216192014.GA2678964@coredump.intra.peff.net","threadId":"52453","inReplyTo":"xmqqlfrcje1f.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Makefile: drop GEN_HDRS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-16T19:20:14Z","receivedAt":"2019-12-16T19:20:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 16, 2019 at 10:55:40AM -0800, Junio C Hamano wrote:\n\n> LIB_H could contain command-list.h (and other GENERATED_H files) if\n> we did this, but dups in dependency does not hurt in general, and I\n> did not find anything potentially problematic in the existing use of\n> $(LIB_H) in our Makefile.\n> \n> How about doing this as a further clean-up? I am reasonably sure\n> the status-quo description is correct, but I find the justification\n> a bit weak (in other words, I do not have a good answer to \"who\n> cares if those that depend on $(LIB_H) are not rebuilt when\n> command-list.h gets rebuilt?\")\n\nYeah, I don't think there's any change in behavior here, since with the\nexception of hdr-check, every mention of $(LIB_H) also mentioned\n$(GENERATED_H). And in the case of hdr-check, we explicitly exclude the\nonly item found in $(GENERATED_H).\n\nBut this would enable us to start checking command-list.h. I'm on the\nfence on whether that's useful or not; the patch below makes it pass,\nbut I'm not sure if that is really turning up any useful problems. I\nsuppose somebody besides help.c could include command-list.h, in which\ncase some of those MAYBE_UNUSED bits could become useful.\n\nI actually wonder if the whole thing would be simpler if command-list.h\nwas a static tracked file with the declarations, and we generated\ncommand-list.c with \"extern const char *command_list[]\", etc.\n\n> --- >8 ---\n> Makefile: include GENERATED_H in LIB_H\n> \n> $(LIB_H), which is meant to be the list of header files that can\n> affect (hence trigger recompilation) the objects that go in\n> libgit.a, in a directory extracted from a tarball is computed by\n> running \"find \\*.h\" but instead computed with \"ls-files \\*.h\" in a\n> working tree managed by a git repository. The former can include\n> generated header files after a build, and omit them in a clean\n> state. The latter would not, as generated header files are by\n> definition not tracked.\n> \n> Explicitly add $(GENERATED_H) to $(LIB_H) to make things consistent.\n\nI do think this is slightly simpler to reason about than the existing\nsetup (though see my \"should it just be a C file?\" above).\n\nHere's the patch that would make hdr-check work:\n\n---\ndiff --git a/Makefile b/Makefile\nindex 87b68962ed..1eac8e7a7a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2780,7 +2780,7 @@ $(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+GEN_HDRS := unicode-width.h\n EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/%\n ifndef GCRYPT_SHA256\n \tEXCEPT_HDRS += sha256/gcrypt.h\ndiff --git a/generate-cmdlist.sh b/generate-cmdlist.sh\nindex 71158f7d8b..7b0751e3e1 100755\n--- a/generate-cmdlist.sh\n+++ b/generate-cmdlist.sh\n@@ -48,6 +48,7 @@ define_categories () {\n define_category_names () {\n \techo\n \techo \"/* Category names */\"\n+\techo \"MAYBE_UNUSED\"\n \techo \"static const char *category_names[] = {\"\n \tbit=0\n \tcategory_list \"$1\" |\n@@ -61,6 +62,7 @@ define_category_names () {\n }\n \n print_command_list () {\n+\techo \"MAYBE_UNUSED\"\n \techo \"static struct cmdname_help command_list[] = {\"\n \n \tcommand_list \"$1\" |\n@@ -78,6 +80,7 @@ print_command_list () {\n \n print_config_list () {\n \tcat < On Mon, Dec 16, 2019 at 10:55:40AM -0800, Junio C Hamano wrote:\n> \n> > LIB_H could contain command-list.h (and other GENERATED_H files) if\n> > we did this, but dups in dependency does not hurt in general, and I\n> > did not find anything potentially problematic in the existing use of\n> > $(LIB_H) in our Makefile.\n> > \n> > How about doing this as a further clean-up? I am reasonably sure\n> > the status-quo description is correct, but I find the justification\n> > a bit weak (in other words, I do not have a good answer to \"who\n> > cares if those that depend on $(LIB_H) are not rebuilt when\n> > command-list.h gets rebuilt?\")\n> \n> Yeah, I don't think there's any change in behavior here, since with the\n> exception of hdr-check, every mention of $(LIB_H) also mentioned\n> $(GENERATED_H). And in the case of hdr-check, we explicitly exclude the\n> only item found in $(GENERATED_H).\n\nTo check my understanding - hdr-check just says \"the headers are\nsyntactically correct\", right? $HCO's target '-o /dev/null' says\n\"don't save the output\", '-c' says \"just compile, don't link\", and '-xc'\nsays \"in C\"; it expands to a target for each file ending in .h but not\nin $EXCEPT_HDRS, and hdr-check calls each one of those expanded targets,\nso I think I understand hdr-check is compiling each header...\n\n> \n> But this would enable us to start checking command-list.h. I'm on the\n> fence on whether that's useful or not; the patch below makes it pass,\n> but I'm not sure if that is really turning up any useful problems. I\n> suppose somebody besides help.c could include command-list.h, in which\n> case some of those MAYBE_UNUSED bits could become useful.\n\nFirstly, I think if someone besides help.c includes command-list.h it\nblows up because there's no include guards :)\n\nMy gut wants to say, \"I need to be sure my generated header is correct!\"\nBut it seems that will also be checked when I try to include that header\nfrom something actually important. So maybe it's not actually useful.\nBut then it seems like hdr-check target isn't that useful for anybody,\nsince those headers should always be included sometime down the road (or\nwhy have them). So that makes me think I must still be missing\nsomething, like maybe I parsed hdr-check wrong.\n\n> \n> I actually wonder if the whole thing would be simpler if command-list.h\n> was a static tracked file with the declarations, and we generated\n> command-list.c with \"extern const char *command_list[]\", etc.\n> \n> > --- >8 ---\n> > Makefile: include GENERATED_H in LIB_H\n> > \n> > $(LIB_H), which is meant to be the list of header files that can\n> > affect (hence trigger recompilation) the objects that go in\n> > libgit.a, in a directory extracted from a tarball is computed by\n> > running \"find \\*.h\" but instead computed with \"ls-files \\*.h\" in a\n> > working tree managed by a git repository. The former can include\n> > generated header files after a build, and omit them in a clean\n> > state. The latter would not, as generated header files are by\n> > definition not tracked.\n> > \n> > Explicitly add $(GENERATED_H) to $(LIB_H) to make things consistent.\n> \n> I do think this is slightly simpler to reason about than the existing\n> setup (though see my \"should it just be a C file?\" above).\n> \n> Here's the patch that would make hdr-check work:\n> \n> ---\n> diff --git a/Makefile b/Makefile\n> index 87b68962ed..1eac8e7a7a 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2780,7 +2780,7 @@ $(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> +GEN_HDRS := unicode-width.h\n> EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/%\n> ifndef GCRYPT_SHA256\n> \tEXCEPT_HDRS += sha256/gcrypt.h\n> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh\n> index 71158f7d8b..7b0751e3e1 100755\n> --- a/generate-cmdlist.sh\n> +++ b/generate-cmdlist.sh\n> @@ -48,6 +48,7 @@ define_categories () {\n> define_category_names () {\n> \techo\n> \techo \"/* Category names */\"\n> +\techo \"MAYBE_UNUSED\"\n> \techo \"static const char *category_names[] = {\"\n> \tbit=0\n> \tcategory_list \"$1\" |\n> @@ -61,6 +62,7 @@ define_category_names () {\n> }\n> \n> print_command_list () {\n> +\techo \"MAYBE_UNUSED\"\n> \techo \"static struct cmdname_help command_list[] = {\"\n> \n> \tcommand_list \"$1\" |\n> @@ -78,6 +80,7 @@ print_command_list () {\n> \n> print_config_list () {\n> \tcat < +MAYBE_UNUSED\n> static const char *config_name_list[] = {\n> EOF\n> \tgrep -h '^[a-zA-Z].*\\..*::$' Documentation/*config.txt Documentation/config/*.txt |\n> @@ -101,7 +104,8 @@ do\n> \tshift\n> done\n> \n> -echo \"/* Automatically generated by generate-cmdlist.sh */\n> +echo \"#include \\\"gettext.h\\\"\n> +/* Automatically generated by generate-cmdlist.sh */\n> struct cmdname_help {\n> \tconst char *name;\n> \tconst char *help;\n"},{"id":"388321","messageId":"20191217014057.GE10734@google.com","threadId":"52453","inReplyTo":"20191217012756.GQ135450@google.com","subject":"Re: [PATCH] Makefile: drop GEN_HDRS","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-17T01:40:57Z","receivedAt":"2019-12-17T01:41:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Emily Shaffer wrote:\n> On Mon, Dec 16, 2019 at 02:20:14PM -0500, Jeff King wrote:\n\n>> But this would enable us to start checking command-list.h. I'm on the\n>> fence on whether that's useful or not; the patch below makes it pass,\n>> but I'm not sure if that is really turning up any useful problems. I\n>> suppose somebody besides help.c could include command-list.h, in which\n>> case some of those MAYBE_UNUSED bits could become useful.\n>\n> Firstly, I think if someone besides help.c includes command-list.h it\n> blows up because there's no include guards :)\n>\n> My gut wants to say, \"I need to be sure my generated header is correct!\"\n> But it seems that will also be checked when I try to include that header\n> from something actually important. So maybe it's not actually useful.\n> But then it seems like hdr-check target isn't that useful for anybody,\n> since those headers should always be included sometime down the road (or\n> why have them). So that makes me think I must still be missing\n> something, like maybe I parsed hdr-check wrong.\n\n\"git log -Shdr-check Makefile\" gives a hint:\n\n $ git log -Shdr-check Makefile\n commit ebb7baf02f69f2164b1f89148945d18c376fc6a8\n Author: Ramsay Jones \n Date: Wed Sep 19 01:07:08 2018 +0100\n\n Makefile: add a hdr-check target\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 Add a Makefile target to automate this process.\n\nSo it's primarily about making sure that each header is #include-able\non its own (i.e., that it #includes the headers for any types it\nrelies on).\n\nThat seems like something I'd want to hold for command-list.h, too. :)\n\nThanks,\nJonathan\n"},{"id":"388322","messageId":"20191217014321.GF10734@google.com","threadId":"52453","inReplyTo":"20191216192014.GA2678964@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: drop GEN_HDRS","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-17T01:43:21Z","receivedAt":"2019-12-17T01:43:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I actually wonder if the whole thing would be simpler if command-list.h\n> was a static tracked file with the declarations, and we generated\n> command-list.c with \"extern const char *command_list[]\", etc.\n\nRight, or a \"command-list.inc\" file.\n\nextern-ing it seems like the simplest way to go.\n\nThanks,\nJonathan\n"},{"id":"388327","messageId":"20191217052224.GA2762303@coredump.intra.peff.net","threadId":"52453","inReplyTo":"20191217012756.GQ135450@google.com","subject":"Re: [PATCH] Makefile: drop GEN_HDRS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-17T05:22:24Z","receivedAt":"2019-12-17T05:22:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 16, 2019 at 05:27:56PM -0800, Emily Shaffer wrote:\n\n> > Yeah, I don't think there's any change in behavior here, since with the\n> > exception of hdr-check, every mention of $(LIB_H) also mentioned\n> > $(GENERATED_H). And in the case of hdr-check, we explicitly exclude the\n> > only item found in $(GENERATED_H).\n> \n> To check my understanding - hdr-check just says \"the headers are\n> syntactically correct\", right? $HCO's target '-o /dev/null' says\n> \"don't save the output\", '-c' says \"just compile, don't link\", and '-xc'\n> says \"in C\"; it expands to a target for each file ending in .h but not\n> in $EXCEPT_HDRS, and hdr-check calls each one of those expanded targets,\n> so I think I understand hdr-check is compiling each header...\n\nSort of. It's less about \"syntactically correct\" (which we'd find out\neasily when we try to compile it) and more about \"does not have\nunexpected dependencies on other files\".\n\nE.g., imagine I write a header foo.h that mentions \"struct bar\", which\nis declared in bar.h. If the only C file that uses that does:\n\n #include \"bar.h\"\n #include \"foo.h\"\n\nthen the compiler is happy. But I've laid a trap for somebody later, who\ndoes just:\n\n #include \"foo.h\"\n\nand gets an annoying compiler error (which is trivial to fix in this\nexample, but can sometimes get complicated to untangle).\n\nSo we declared a rule that header files should be self-sufficient\n(modulo git-compat-util.h), and hdr-check is the way to find out if that\nis true.\n\n> > but I'm not sure if that is really turning up any useful problems. I\n> > suppose somebody besides help.c could include command-list.h, in which\n> > case some of those MAYBE_UNUSED bits could become useful.\n> \n> Firstly, I think if someone besides help.c includes command-list.h it\n> blows up because there's no include guards :)\n\nOnly if another header file does it, which could easily cause double\ninclusion within the same source file. It's perfectly fine for another\ntranslation unit to include it.\n\n(Side note: builtin/help.c is declared in the Makefile as depending on\nit, but doesn't seem to actually include it).\n\n> My gut wants to say, \"I need to be sure my generated header is correct!\"\n> But it seems that will also be checked when I try to include that header\n> from something actually important. So maybe it's not actually useful.\n> But then it seems like hdr-check target isn't that useful for anybody,\n> since those headers should always be included sometime down the road (or\n> why have them). So that makes me think I must still be missing\n> something, like maybe I parsed hdr-check wrong.\n\nI think this is the \"it compiles now but you've laid a trap...\" thing\nmentioned above.\n\nAs it is, command-list.h _does_ have such a trap; you need to include\ngettext.h first. But since so few callers use it (and are likely to use\nit) nobody has really noticed or cared.\n\n-Peff\n"},{"id":"388329","messageId":"20191217052812.GB2762303@coredump.intra.peff.net","threadId":"52453","inReplyTo":"20191217014321.GF10734@google.com","subject":"Re: [PATCH] Makefile: drop GEN_HDRS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-17T05:28:12Z","receivedAt":"2019-12-17T05:28:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 16, 2019 at 05:43:21PM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > I actually wonder if the whole thing would be simpler if command-list.h\n> > was a static tracked file with the declarations, and we generated\n> > command-list.c with \"extern const char *command_list[]\", etc.\n> \n> Right, or a \"command-list.inc\" file.\n> \n> extern-ing it seems like the simplest way to go.\n\nIf only. I took a brief look at this. Besides the Makefile chaos (did\nyou know that the vcxproj rule manually builds and git-adds\ncommand-list.h? No idea what is going on there), it looks like we\ndynamically generate the category bitfield, which is then used directly\nin help.c. We _could_ declare those bitfields as externs themselves, but\npart of the point is that the full list of categories is generated\ndynamically from command-list.txt.\n\nSo we'd either split the list into two (one list of categories special\nenough to be manually declared, and the rest that get generated\nautomatically) or we'd end up just duplicating the whole list.\n\nCertainly this could all be untangled, but given that the system is\nworking just fine as it is, it doesn't seem worth anybody's time (and\nthe risk of weird follow-on problems) to adjust it.\n\n-Peff\n"},{"id":"388355","messageId":"nycvar.QRO.7.76.6.1912171231450.46@tvgsbejvaqbjf.bet","threadId":"52453","inReplyTo":"20191217052812.GB2762303@coredump.intra.peff.net","subject":"vcxproj target, was Re: [PATCH] Makefile: drop GEN_HDRS","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-17T11:35:58Z","receivedAt":"2019-12-17T11:36:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 17 Dec 2019, Jeff King wrote:\n\n> (did you know that the vcxproj rule manually builds and git-adds\n> command-list.h? No idea what is going on there),\n\nThe idea of this is that contributors can clone the `vs/master` branch\nfrom Git for Windows repository, check it out, and open it in Visual\nStudio, then build.\n\nMeaning: we cannot use any Unix shell scripts or Makefile targets to\ngenerate _anything_. That's just not an option. It is such a foreign\nconcept in Visual Studio projects, it is very much Unix-y to think that\neverything uses `make` anyway and everybody has access to a Unix shell\ninterpreter and the many Unix tools including `sed`, `awk`, etc.\n\nTherefore we pre-generate all of those generated files, commit them, and\nthe user does not have to worry about getting ~700MB worth of compressed\ndata, unpack that to a ~2GB build environment that I like to call \"Git\nfor Windows SDK\", _just_ to generate those files.\n\nThat's what's going on there.\n\nCiao,\nDscho\n"},{"id":"388504","messageId":"20191219045124.GB89333@coredump.intra.peff.net","threadId":"52453","inReplyTo":"nycvar.QRO.7.76.6.1912171231450.46@tvgsbejvaqbjf.bet","subject":"Re: vcxproj target, was Re: [PATCH] Makefile: drop GEN_HDRS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-19T04:51:24Z","receivedAt":"2019-12-19T04:51:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 17, 2019 at 12:35:58PM +0100, Johannes Schindelin wrote:\n\n> On Tue, 17 Dec 2019, Jeff King wrote:\n> \n> > (did you know that the vcxproj rule manually builds and git-adds\n> > command-list.h? No idea what is going on there),\n> \n> The idea of this is that contributors can clone the `vs/master` branch\n> from Git for Windows repository, check it out, and open it in Visual\n> Studio, then build.\n> [...]\n> That's what's going on there.\n\nThanks for satisfying my curiosity. Though I still think I stand by my\nstatement that the cost/benefit isn't really there to switch how we\nhandle command-list.h.\n\n-Peff\n"}]}