| Message ID | xmqq1rt7hkp6.fsf@gitster-ct.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: drop GEN_HDRS | expand |
On Fri, Dec 13, 2019 at 03:25:41PM -0800, Junio C Hamano wrote: > Get rid of GEN_HDRS, which is used only once to list the headers we > do not run hdr-check test on, and instead explicitly list that the > ones, either tracked or generated, that we exempt from the test. Yeah, I think this is an improvement by itself. After reading this, though: > - If we value the header cleanliness check, we eventually want to > teach our header generating scripts to produce clean headers. > Keeping the blanket "generated headers can be left as dirty as we > want" exception does not nudge us in the right direction. I did expect to see the actual hdr-check behavior move towards checking these generated versions. However, both are kind of interesting. unicode-width.h isn't a "real" header file; it's meant to be included in the middle of a function. I think it _could_ be changed to define "struct interval" itself, and then be a static file-scope variable. But there's not really a compelling reason to do so. But "command-list.h" is more of a traditional header file, being included at the top of help.c. In theory the hdr-check target could add a dependency on it, and then we could check it along with everything else. But even without that first step, if I remove it from EXCEPT_HDRS, nothing happens! That's because LIB_H is created by running find in the local filesystem. So until it's generated, we don't realize it's there to check. I kind of wonder if it should be part of LIB_H. I suspect that on some systems, we'd fail to notice a rebuild when command-list.txt is updated (but nobody noticed, because it is only systems that do not have compiler-supported dependency tracking, and most developers are no modern platforms that do). -Peff
On Fri, Dec 13, 2019 at 07:38:21PM -0500, Jeff King wrote: > That's because LIB_H is created by running find in the local filesystem. > So until it's generated, we don't realize it's there to check. I kind of > wonder if it should be part of LIB_H. I suspect that on some systems, > we'd fail to notice a rebuild when command-list.txt is updated (but > nobody noticed, because it is only systems that do not have > compiler-supported dependency tracking, and most developers are no > modern platforms that do). Actually, this probably isn't true. We have an explicit dependency for help.o on command-list.h, so it would get built properly then. Its inclusion in LIB_H is still wonky, though. It sometimes is included and sometimes not, depending on whether ls-files or find is used. -Peff
Jeff King <peff@peff.net> writes: > On Fri, Dec 13, 2019 at 07:38:21PM -0500, Jeff King wrote: > >> That's because LIB_H is created by running find in the local filesystem. >> So until it's generated, we don't realize it's there to check. I kind of >> wonder if it should be part of LIB_H. I suspect that on some systems, >> we'd fail to notice a rebuild when command-list.txt is updated (but >> nobody noticed, because it is only systems that do not have >> compiler-supported dependency tracking, and most developers are no >> modern platforms that do). > > Actually, this probably isn't true. We have an explicit dependency for > help.o on command-list.h, so it would get built properly then. > > Its inclusion in LIB_H is still wonky, though. It sometimes is included > and sometimes not, depending on whether ls-files or find is used. As long as GENERATED_H is maintained properly to list headers that are actually used (e.g. if we ever start creating and using a header only when some Makefile macro tells us to, we make sure to place the header in GENERATED_H only when we create and use it), I think we should just add it to LIB_H, regardless of what is tracked. LIB_H could contain command-list.h (and other GENERATED_H files) if we did this, but dups in dependency does not hurt in general, and I did not find anything potentially problematic in the existing use of $(LIB_H) in our Makefile. How about doing this as a further clean-up? I am reasonably sure the status-quo description is correct, but I find the justification a bit weak (in other words, I do not have a good answer to "who cares if those that depend on $(LIB_H) are not rebuilt when command-list.h gets rebuilt?") --- >8 --- Makefile: include GENERATED_H in LIB_H $(LIB_H), which is meant to be the list of header files that can affect (hence trigger recompilation) the objects that go in libgit.a, in a directory extracted from a tarball is computed by running "find \*.h" but instead computed with "ls-files \*.h" in a working tree managed by a git repository. The former can include generated header files after a build, and omit them in a clean state. The latter would not, as generated header files are by definition not tracked. Explicitly add $(GENERATED_H) to $(LIB_H) to make things consistent. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 9a9d35637d..552c43c3d8 100644 --- a/Makefile +++ b/Makefile @@ -822,6 +822,7 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat -name t -prune -o \ -name Documentation -prune -o \ -name '*.h' -print))) +LIB_H += $(GENERATED_H) LIB_OBJS += abspath.o LIB_OBJS += add-interactive.o @@ -2399,7 +2400,7 @@ else # should _not_ be included here, since they are necessary even when # building an object for the first time. -$(OBJECTS): $(LIB_H) $(GENERATED_H) +$(OBJECTS): $(LIB_H) endif exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX @@ -2521,7 +2522,7 @@ XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \ --keyword=gettextln --keyword=eval_gettextln XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \ --keyword=__ --keyword=N__ --keyword="__n:1,2" -LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H) +LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) LOCALIZED_SH = $(SCRIPT_SH) LOCALIZED_SH += git-parse-remote.sh LOCALIZED_SH += git-rebase--preserve-merges.sh
On Mon, Dec 16, 2019 at 10:55:40AM -0800, Junio C Hamano wrote: > LIB_H could contain command-list.h (and other GENERATED_H files) if > we did this, but dups in dependency does not hurt in general, and I > did not find anything potentially problematic in the existing use of > $(LIB_H) in our Makefile. > > How about doing this as a further clean-up? I am reasonably sure > the status-quo description is correct, but I find the justification > a bit weak (in other words, I do not have a good answer to "who > cares if those that depend on $(LIB_H) are not rebuilt when > command-list.h gets rebuilt?") Yeah, I don't think there's any change in behavior here, since with the exception of hdr-check, every mention of $(LIB_H) also mentioned $(GENERATED_H). And in the case of hdr-check, we explicitly exclude the only item found in $(GENERATED_H). But this would enable us to start checking command-list.h. I'm on the fence on whether that's useful or not; the patch below makes it pass, but I'm not sure if that is really turning up any useful problems. I suppose somebody besides help.c could include command-list.h, in which case some of those MAYBE_UNUSED bits could become useful. I actually wonder if the whole thing would be simpler if command-list.h was a static tracked file with the declarations, and we generated command-list.c with "extern const char *command_list[]", etc. > --- >8 --- > Makefile: include GENERATED_H in LIB_H > > $(LIB_H), which is meant to be the list of header files that can > affect (hence trigger recompilation) the objects that go in > libgit.a, in a directory extracted from a tarball is computed by > running "find \*.h" but instead computed with "ls-files \*.h" in a > working tree managed by a git repository. The former can include > generated header files after a build, and omit them in a clean > state. The latter would not, as generated header files are by > definition not tracked. > > Explicitly add $(GENERATED_H) to $(LIB_H) to make things consistent. I do think this is slightly simpler to reason about than the existing setup (though see my "should it just be a C file?" above). Here's the patch that would make hdr-check work: --- diff --git a/Makefile b/Makefile index 87b68962ed..1eac8e7a7a 100644 --- a/Makefile +++ b/Makefile @@ -2780,7 +2780,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE .PHONY: sparse $(SP_OBJ) sparse: $(SP_OBJ) -GEN_HDRS := command-list.h unicode-width.h +GEN_HDRS := unicode-width.h EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/% ifndef GCRYPT_SHA256 EXCEPT_HDRS += sha256/gcrypt.h diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh index 71158f7d8b..7b0751e3e1 100755 --- a/generate-cmdlist.sh +++ b/generate-cmdlist.sh @@ -48,6 +48,7 @@ define_categories () { define_category_names () { echo echo "/* Category names */" + echo "MAYBE_UNUSED" echo "static const char *category_names[] = {" bit=0 category_list "$1" | @@ -61,6 +62,7 @@ define_category_names () { } print_command_list () { + echo "MAYBE_UNUSED" echo "static struct cmdname_help command_list[] = {" command_list "$1" | @@ -78,6 +80,7 @@ print_command_list () { print_config_list () { cat <<EOF +MAYBE_UNUSED static const char *config_name_list[] = { EOF grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt | @@ -101,7 +104,8 @@ do shift done -echo "/* Automatically generated by generate-cmdlist.sh */ +echo "#include \"gettext.h\" +/* Automatically generated by generate-cmdlist.sh */ struct cmdname_help { const char *name; const char *help;
On Mon, Dec 16, 2019 at 02:20:14PM -0500, Jeff King wrote: > On Mon, Dec 16, 2019 at 10:55:40AM -0800, Junio C Hamano wrote: > > > LIB_H could contain command-list.h (and other GENERATED_H files) if > > we did this, but dups in dependency does not hurt in general, and I > > did not find anything potentially problematic in the existing use of > > $(LIB_H) in our Makefile. > > > > How about doing this as a further clean-up? I am reasonably sure > > the status-quo description is correct, but I find the justification > > a bit weak (in other words, I do not have a good answer to "who > > cares if those that depend on $(LIB_H) are not rebuilt when > > command-list.h gets rebuilt?") > > Yeah, I don't think there's any change in behavior here, since with the > exception of hdr-check, every mention of $(LIB_H) also mentioned > $(GENERATED_H). And in the case of hdr-check, we explicitly exclude the > only item found in $(GENERATED_H). To check my understanding - hdr-check just says "the headers are syntactically correct", right? $HCO's target '-o /dev/null' says "don't save the output", '-c' says "just compile, don't link", and '-xc' says "in C"; it expands to a target for each file ending in .h but not in $EXCEPT_HDRS, and hdr-check calls each one of those expanded targets, so I think I understand hdr-check is compiling each header... > > But this would enable us to start checking command-list.h. I'm on the > fence on whether that's useful or not; the patch below makes it pass, > but I'm not sure if that is really turning up any useful problems. I > suppose somebody besides help.c could include command-list.h, in which > case some of those MAYBE_UNUSED bits could become useful. Firstly, I think if someone besides help.c includes command-list.h it blows up because there's no include guards :) My gut wants to say, "I need to be sure my generated header is correct!" But it seems that will also be checked when I try to include that header from something actually important. So maybe it's not actually useful. But then it seems like hdr-check target isn't that useful for anybody, since those headers should always be included sometime down the road (or why have them). So that makes me think I must still be missing something, like maybe I parsed hdr-check wrong. > > I actually wonder if the whole thing would be simpler if command-list.h > was a static tracked file with the declarations, and we generated > command-list.c with "extern const char *command_list[]", etc. > > > --- >8 --- > > Makefile: include GENERATED_H in LIB_H > > > > $(LIB_H), which is meant to be the list of header files that can > > affect (hence trigger recompilation) the objects that go in > > libgit.a, in a directory extracted from a tarball is computed by > > running "find \*.h" but instead computed with "ls-files \*.h" in a > > working tree managed by a git repository. The former can include > > generated header files after a build, and omit them in a clean > > state. The latter would not, as generated header files are by > > definition not tracked. > > > > Explicitly add $(GENERATED_H) to $(LIB_H) to make things consistent. > > I do think this is slightly simpler to reason about than the existing > setup (though see my "should it just be a C file?" above). > > Here's the patch that would make hdr-check work: > > --- > diff --git a/Makefile b/Makefile > index 87b68962ed..1eac8e7a7a 100644 > --- a/Makefile > +++ b/Makefile > @@ -2780,7 +2780,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE > .PHONY: sparse $(SP_OBJ) > sparse: $(SP_OBJ) > > -GEN_HDRS := command-list.h unicode-width.h > +GEN_HDRS := unicode-width.h > EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/% > ifndef GCRYPT_SHA256 > EXCEPT_HDRS += sha256/gcrypt.h > diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh > index 71158f7d8b..7b0751e3e1 100755 > --- a/generate-cmdlist.sh > +++ b/generate-cmdlist.sh > @@ -48,6 +48,7 @@ define_categories () { > define_category_names () { > echo > echo "/* Category names */" > + echo "MAYBE_UNUSED" > echo "static const char *category_names[] = {" > bit=0 > category_list "$1" | > @@ -61,6 +62,7 @@ define_category_names () { > } > > print_command_list () { > + echo "MAYBE_UNUSED" > echo "static struct cmdname_help command_list[] = {" > > command_list "$1" | > @@ -78,6 +80,7 @@ print_command_list () { > > print_config_list () { > cat <<EOF > +MAYBE_UNUSED > static const char *config_name_list[] = { > EOF > grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt | > @@ -101,7 +104,8 @@ do > shift > done > > -echo "/* Automatically generated by generate-cmdlist.sh */ > +echo "#include \"gettext.h\" > +/* Automatically generated by generate-cmdlist.sh */ > struct cmdname_help { > const char *name; > const char *help;
Emily Shaffer wrote: > On Mon, Dec 16, 2019 at 02:20:14PM -0500, Jeff King wrote: >> But this would enable us to start checking command-list.h. I'm on the >> fence on whether that's useful or not; the patch below makes it pass, >> but I'm not sure if that is really turning up any useful problems. I >> suppose somebody besides help.c could include command-list.h, in which >> case some of those MAYBE_UNUSED bits could become useful. > > Firstly, I think if someone besides help.c includes command-list.h it > blows up because there's no include guards :) > > My gut wants to say, "I need to be sure my generated header is correct!" > But it seems that will also be checked when I try to include that header > from something actually important. So maybe it's not actually useful. > But then it seems like hdr-check target isn't that useful for anybody, > since those headers should always be included sometime down the road (or > why have them). So that makes me think I must still be missing > something, like maybe I parsed hdr-check wrong. "git log -Shdr-check Makefile" gives a hint: $ git log -Shdr-check Makefile commit ebb7baf02f69f2164b1f89148945d18c376fc6a8 Author: Ramsay Jones <ramsay@ramsayjones.plus.com> Date: Wed Sep 19 01:07:08 2018 +0100 Makefile: add a hdr-check target Commit ef3ca95475 ("Add missing includes and forward declarations", 2018-08-15) resulted from the author employing a manual method to create a C file consisting of a pair of pre-processor #include lines (for 'git-compat-util.h' and a given toplevel header), and fixing any resulting compiler errors or warnings. Add a Makefile target to automate this process. So it's primarily about making sure that each header is #include-able on its own (i.e., that it #includes the headers for any types it relies on). That seems like something I'd want to hold for command-list.h, too. :) Thanks, Jonathan
Jeff King wrote: > I actually wonder if the whole thing would be simpler if command-list.h > was a static tracked file with the declarations, and we generated > command-list.c with "extern const char *command_list[]", etc. Right, or a "command-list.inc" file. extern-ing it seems like the simplest way to go. Thanks, Jonathan
On Mon, Dec 16, 2019 at 05:27:56PM -0800, Emily Shaffer wrote: > > Yeah, I don't think there's any change in behavior here, since with the > > exception of hdr-check, every mention of $(LIB_H) also mentioned > > $(GENERATED_H). And in the case of hdr-check, we explicitly exclude the > > only item found in $(GENERATED_H). > > To check my understanding - hdr-check just says "the headers are > syntactically correct", right? $HCO's target '-o /dev/null' says > "don't save the output", '-c' says "just compile, don't link", and '-xc' > says "in C"; it expands to a target for each file ending in .h but not > in $EXCEPT_HDRS, and hdr-check calls each one of those expanded targets, > so I think I understand hdr-check is compiling each header... Sort of. It's less about "syntactically correct" (which we'd find out easily when we try to compile it) and more about "does not have unexpected dependencies on other files". E.g., imagine I write a header foo.h that mentions "struct bar", which is declared in bar.h. If the only C file that uses that does: #include "bar.h" #include "foo.h" then the compiler is happy. But I've laid a trap for somebody later, who does just: #include "foo.h" and gets an annoying compiler error (which is trivial to fix in this example, but can sometimes get complicated to untangle). So we declared a rule that header files should be self-sufficient (modulo git-compat-util.h), and hdr-check is the way to find out if that is true. > > but I'm not sure if that is really turning up any useful problems. I > > suppose somebody besides help.c could include command-list.h, in which > > case some of those MAYBE_UNUSED bits could become useful. > > Firstly, I think if someone besides help.c includes command-list.h it > blows up because there's no include guards :) Only if another header file does it, which could easily cause double inclusion within the same source file. It's perfectly fine for another translation unit to include it. (Side note: builtin/help.c is declared in the Makefile as depending on it, but doesn't seem to actually include it). > My gut wants to say, "I need to be sure my generated header is correct!" > But it seems that will also be checked when I try to include that header > from something actually important. So maybe it's not actually useful. > But then it seems like hdr-check target isn't that useful for anybody, > since those headers should always be included sometime down the road (or > why have them). So that makes me think I must still be missing > something, like maybe I parsed hdr-check wrong. I think this is the "it compiles now but you've laid a trap..." thing mentioned above. As it is, command-list.h _does_ have such a trap; you need to include gettext.h first. But since so few callers use it (and are likely to use it) nobody has really noticed or cared. -Peff
On Mon, Dec 16, 2019 at 05:43:21PM -0800, Jonathan Nieder wrote: > Jeff King wrote: > > > I actually wonder if the whole thing would be simpler if command-list.h > > was a static tracked file with the declarations, and we generated > > command-list.c with "extern const char *command_list[]", etc. > > Right, or a "command-list.inc" file. > > extern-ing it seems like the simplest way to go. If only. I took a brief look at this. Besides the Makefile chaos (did you know that the vcxproj rule manually builds and git-adds command-list.h? No idea what is going on there), it looks like we dynamically generate the category bitfield, which is then used directly in help.c. We _could_ declare those bitfields as externs themselves, but part of the point is that the full list of categories is generated dynamically from command-list.txt. So we'd either split the list into two (one list of categories special enough to be manually declared, and the rest that get generated automatically) or we'd end up just duplicating the whole list. Certainly this could all be untangled, but given that the system is working just fine as it is, it doesn't seem worth anybody's time (and the risk of weird follow-on problems) to adjust it. -Peff
Hi Peff, On Tue, 17 Dec 2019, Jeff King wrote: > (did you know that the vcxproj rule manually builds and git-adds > command-list.h? No idea what is going on there), The idea of this is that contributors can clone the `vs/master` branch from Git for Windows repository, check it out, and open it in Visual Studio, then build. Meaning: we cannot use any Unix shell scripts or Makefile targets to generate _anything_. That's just not an option. It is such a foreign concept in Visual Studio projects, it is very much Unix-y to think that everything uses `make` anyway and everybody has access to a Unix shell interpreter and the many Unix tools including `sed`, `awk`, etc. Therefore we pre-generate all of those generated files, commit them, and the user does not have to worry about getting ~700MB worth of compressed data, unpack that to a ~2GB build environment that I like to call "Git for Windows SDK", _just_ to generate those files. That's what's going on there. Ciao, Dscho
On Tue, Dec 17, 2019 at 12:35:58PM +0100, Johannes Schindelin wrote: > On Tue, 17 Dec 2019, Jeff King wrote: > > > (did you know that the vcxproj rule manually builds and git-adds > > command-list.h? No idea what is going on there), > > The idea of this is that contributors can clone the `vs/master` branch > from Git for Windows repository, check it out, and open it in Visual > Studio, then build. > [...] > That's what's going on there. Thanks for satisfying my curiosity. Though I still think I stand by my statement that the cost/benefit isn't really there to switch how we handle command-list.h. -Peff
diff --git a/Makefile b/Makefile index b7d7374dac..9a9d35637d 100644 --- a/Makefile +++ b/Makefile @@ -2779,8 +2779,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE .PHONY: sparse $(SP_OBJ) sparse: $(SP_OBJ) -GEN_HDRS := command-list.h unicode-width.h -EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/% +EXCEPT_HDRS := command-list.h unicode-width.h compat/% xdiff/% ifndef GCRYPT_SHA256 EXCEPT_HDRS += sha256/gcrypt.h endif @@ -3105,7 +3104,7 @@ clean: profile-clean coverage-clean cocciclean $(RM) $(HCC) $(RM) -r bin-wrappers $(dep_dirs) $(RM) -r po/build/ - $(RM) *.pyc *.pyo */*.pyc */*.pyo command-list.h $(ETAGS_TARGET) tags cscope* + $(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope* $(RM) -r $(GIT_TARNAME) .doc-tmp-dir $(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz $(RM) $(htmldocs).tar.gz $(manpages).tar.gz
When ebb7baf0 ("Makefile: add a hdr-check target", 2018-09-19) implemented hdr-check target, it wanted to leave some header files exempt from the stricter check the target implements, and added GEN_HDRS macro to list them This however was probably a bad move for two and half reasons: - If we value the header cleanliness check, we eventually want to teach our header generating scripts to produce clean headers. Keeping the blanket "generated headers can be left as dirty as we want" exception does not nudge us in the right direction. - There is a list of generated header files, GENERATED_H, which is used to keep track of dependencies. Presence of GEN_HDRS that is too similarly named would confuse developers who are adding new generated header files which list to add theirs. - Even though unicode-width.h could be generated using a contrib/ script, as far as our build infrastructure is concerned, it is a source file that is tracked in the source control system. Its presence in GEN_HDRS list is doubly misleading. Get rid of GEN_HDRS, which is used only once to list the headers we do not run hdr-check test on, and instead explicitly list that the ones, either tracked or generated, that we exempt from the test. This allows GENERATED_H to be the sole "here are build artifact header files that are expendable" list, so use it in the clean target to $(RM) them. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Makefile | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)