{"thread":{"id":"50623","subject":"[PATCH 0/1] Avoid calling find in the Makefile, if possible","startedAt":"2019-03-01T19:57:49Z","lastAt":"2019-03-06T22:43:00Z","messageCount":31,"participants":["Johannes Schindelin via GitGitGadget","Jeff King","Johannes Schindelin","Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"370446","messageId":"pull.130.git.gitgitgadget@gmail.com","threadId":"50623","inReplyTo":null,"subject":"[PATCH 0/1] Avoid calling find in the Makefile, if possible","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-01T19:57:45Z","receivedAt":"2019-03-01T19:57:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I noticed this quite a bit of time ago, but did not get a chance to look\ninto it in detail: all of a sudden, make started really slowly over here.\n\nThe culprit turned out to be a find call, which was in the Makefile for\nages, so I was puzzled why it only caused problems recently.\n\nAfter some digging, I found out that the hdr-check target is the culprit:\nbefore it was introduced, $(LIB_H) did not need to be expanded, and as a\nconsequence the find call was not executed. Once hdr-check made it into \nmaster, though, $(LIB_H) must be expanded to define that rule.\n\nSince I have tons of worktrees as subdirectories of my principal Git clone,\nand since I also have a 3rdparty/ directory with tons of repositories I use\nfor various testing/contributing purposes, this find is quite a little slow\nover here.\n\nSo here is my suggested fix. It is based on similar code we already had in\nthe Makefile, obviously also intended to avoid an expensive find invocation.\n\nJohannes Schindelin (1):\n Makefile: use `git ls-files` to list header files, if possible\n\n Makefile | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\n\nbase-commit: c65a2884eae159bad540135479bc8afe20ff62d1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-130%2Fdscho%2Favoid-find-in-makefile-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-130/dscho/avoid-find-in-makefile-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/130\n-- \ngitgitgadget\n"},{"id":"370447","messageId":"0b5529406b9458d37f3f5cdf38baa2d6a0a70a65.1551470265.git.gitgitgadget@gmail.com","threadId":"50623","inReplyTo":"pull.130.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-01T19:57:46Z","receivedAt":"2019-03-01T19:57:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn d85b0dff72 (Makefile: use `find` to determine static header\ndependencies, 2014-08-25), we switched from a static list of header\nfiles to a dynamically-generated one, asking `find` to enumerate them.\n\nBack in those days, we did not use `$(LIB_H)` by default, and many a\n`make` implementation seems smart enough not to run that `find` command\nin that case, so it was deemed okay to run `find` for special targets\nrequiring this macro.\n\nHowever, as of ebb7baf02f (Makefile: add a hdr-check target,\n2018-09-19), $(LIB_H) is part of a global rule and therefore must be\nexpanded. Meaning: this `find` command has to be run upon every\n`make` invocation. In the presence of many a worktree, this can tax the\ndevelopers' patience quite a bit.\n\nEven in the absence of worktrees or other untracked files and\ndirectories, the cost of I/O to generate that list of header files is\nsimply a lot larger than a simple `git ls-files` call.\n\nTherefore, just like in 335339758c (Makefile: ask \"ls-files\" to list\nsource files if available, 2011-10-18), we now prefer to use `git\nls-files` to enumerate the header files to enumerating them via `find`,\nfalling back to the latter if the former failed (which would be the case\ne.g. in a worktree that was extracted from a source .tar file rather\nthan from a clone of Git's sources).\n\nThis has one notable consequence: we no longer include `command-list.h`\nin `LIB_H`, as it is a generated file, not a tracked one.\n\nLikewise, we no longer include not-yet-tracked header files in `LIB_H`.\n\nGiven the speed improvements, these consequences seem a comparably small\nprice to pay.\n\nSigned-off-by: Johannes Schindelin \n---\n Makefile | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex c5240942f2..2644324915 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -841,7 +841,8 @@ VCSSVN_LIB = vcs-svn/lib.a\n \n GENERATED_H += command-list.h\n \n-LIB_H = $(shell $(FIND) . \\\n+LIB_H = $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \\\n+\t$(FIND) . \\\n \t-name .git -prune -o \\\n \t-name t -prune -o \\\n \t-name Documentation -prune -o \\\n@@ -2363,7 +2364,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)\n+$(OBJECTS): $(LIB_H) $(GENERATED_H)\n endif\n \n exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX\n-- \ngitgitgadget\n"},{"id":"370450","messageId":"20190301213619.GA1518@sigill.intra.peff.net","threadId":"50623","inReplyTo":"0b5529406b9458d37f3f5cdf38baa2d6a0a70a65.1551470265.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-01T21:36:19Z","receivedAt":"2019-03-01T21:36:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2019 at 11:57:46AM -0800, Johannes Schindelin via GitGitGadget wrote:\n\n> In d85b0dff72 (Makefile: use `find` to determine static header\n> dependencies, 2014-08-25), we switched from a static list of header\n> files to a dynamically-generated one, asking `find` to enumerate them.\n> \n> Back in those days, we did not use `$(LIB_H)` by default, and many a\n> `make` implementation seems smart enough not to run that `find` command\n> in that case, so it was deemed okay to run `find` for special targets\n> requiring this macro.\n> \n> However, as of ebb7baf02f (Makefile: add a hdr-check target,\n> 2018-09-19), $(LIB_H) is part of a global rule and therefore must be\n> expanded. Meaning: this `find` command has to be run upon every\n> `make` invocation. In the presence of many a worktree, this can tax the\n> developers' patience quite a bit.\n\nI'm confused about this part. We don't run hdr-check by default. Why\nwould make need the value of $(LIB_H)? Yet empirically it does run find.\n\nWorse, it seems to actually run it _three times_. Once for the $(HCO)\ntarget, once for the .PHONY, and once for the hdr-check target. I think\nthe .PHONY one is unavoidable (it doesn't know which names we might\notherwise be building should be marked), but the other two seem like\nbugs in make (or at least pessimisations).\n\nIt makes me wonder if we'd be better off pushing hdr-check into a\nseparate script. It doesn't actually use make's dependency tree in any\nmeaningful way. And then regular invocations wouldn't even have to pay\nthe `ls-files` price.\n\nIf we are going to keep it in the Makefile, we should probably use a\n\":=\" rule to avoid running it three times.\n\n> Even in the absence of worktrees or other untracked files and\n> directories, the cost of I/O to generate that list of header files is\n> simply a lot larger than a simple `git ls-files` call.\n> \n> Therefore, just like in 335339758c (Makefile: ask \"ls-files\" to list\n> source files if available, 2011-10-18), we now prefer to use `git\n> ls-files` to enumerate the header files to enumerating them via `find`,\n> falling back to the latter if the former failed (which would be the case\n> e.g. in a worktree that was extracted from a source .tar file rather\n> than from a clone of Git's sources).\n\nThat seems reasonable (regardless of whether it is in a script or in the\nMakefile). Another option is to use -maxdepth, but that involves\nguessing how deep people might actually put header files.\n\n> This has one notable consequence: we no longer include `command-list.h`\n> in `LIB_H`, as it is a generated file, not a tracked one.\n\nWe should be able to add back $(GENERATED_H) as appropriate. I see you\ndid it for the non-computed-dependencies case. Couldn't we do the same\nfor $(LOCALIZED_C) and $(CHK_HDRS)?\n\n> Likewise, we no longer include not-yet-tracked header files in `LIB_H`.\n\nI think that's probably OK.\n\n-Peff\n"},{"id":"370451","messageId":"20190301215414.GB1518@sigill.intra.peff.net","threadId":"50623","inReplyTo":"20190301213619.GA1518@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-01T21:54:15Z","receivedAt":"2019-03-01T21:54:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2019 at 04:36:19PM -0500, Jeff King wrote:\n\n> > However, as of ebb7baf02f (Makefile: add a hdr-check target,\n> > 2018-09-19), $(LIB_H) is part of a global rule and therefore must be\n> > expanded. Meaning: this `find` command has to be run upon every\n> > `make` invocation. In the presence of many a worktree, this can tax the\n> > developers' patience quite a bit.\n> \n> I'm confused about this part. We don't run hdr-check by default. Why\n> would make need the value of $(LIB_H)? Yet empirically it does run find.\n> \n> Worse, it seems to actually run it _three times_. Once for the $(HCO)\n> target, once for the .PHONY, and once for the hdr-check target. I think\n> the .PHONY one is unavoidable (it doesn't know which names we might\n> otherwise be building should be marked), but the other two seem like\n> bugs in make (or at least pessimisations).\n> \n> It makes me wonder if we'd be better off pushing hdr-check into a\n> separate script. It doesn't actually use make's dependency tree in any\n> meaningful way. And then regular invocations wouldn't even have to pay\n> the `ls-files` price.\n> \n> If we are going to keep it in the Makefile, we should probably use a\n> \":=\" rule to avoid running it three times.\n\nLooping in Ramsay as the author of hdr-check. I think we could even do\nthis without using a separate script, like so:\n\ndiff --git a/Makefile b/Makefile\nindex c5240942f2..a65c4599e9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1852,7 +1852,6 @@ 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@@ -2740,11 +2739,13 @@ 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+.PHONY: hdr-check\n+hdr-check:\n+\t@for i in $(LIB_H); do \\\n+\t\techo \" HDR $$i\"; \\\n+\t\t$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $$i || \\\n+\t\t\texit 1; \\\n+\tdone\n \n .PHONY: style\n style:\n\nThe one thing we do lose, though, is make's parallelization. It would\nprobably be possible to actually shove this into a sub-make which\ndefined the hdr-check rules, but I don't know how complicated that would\nbecome.\n\n> > This has one notable consequence: we no longer include `command-list.h`\n> > in `LIB_H`, as it is a generated file, not a tracked one.\n> \n> We should be able to add back $(GENERATED_H) as appropriate. I see you\n> did it for the non-computed-dependencies case. Couldn't we do the same\n> for $(LOCALIZED_C) and $(CHK_HDRS)?\n\nAnswering my own question somewhat: $(CHK_HDRS) explicitly ignores the\ngenerated headers, so we wouldn't want to bother re-adding it there\nanyway. Possibly $(LOCALIZED_C) would care, though. The generated file\ndoes have translatable strings in it.\n\n-Peff\n"},{"id":"370453","messageId":"20190301220112.GA14966@sigill.intra.peff.net","threadId":"50623","inReplyTo":"20190301215414.GB1518@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-01T22:01:12Z","receivedAt":"2019-03-01T22:01:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2019 at 04:54:15PM -0500, Jeff King wrote:\n\n> The one thing we do lose, though, is make's parallelization. It would\n> probably be possible to actually shove this into a sub-make which\n> defined the hdr-check rules, but I don't know how complicated that would\n> become.\n\nThis seems to work, though it's kind of horrid.\n\nIt costs at least one extra process to run \"make hdr-check\", and\nprobably more for things like $(GIT_VERSION) that the Makefile include\nlikely triggers. But when you're not running hdr-check (which is the\nnorm), it's zero-cost.\n\n-Peff\n\ndiff --git a/Makefile b/Makefile\nindex c5240942f2..ab6ecf450e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2735,16 +2735,8 @@ $(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+hdr-check:\n+\t$(MAKE) -f hdr-check.mak hdr-check\n \n .PHONY: style\n style:\ndiff --git a/hdr-check.mak b/hdr-check.mak\nnew file mode 100644\nindex 0000000000..b8924afa90\n--- /dev/null\n+++ b/hdr-check.mak\n@@ -0,0 +1,12 @@\n+include Makefile\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"},{"id":"370470","messageId":"nycvar.QRO.7.76.6.1903022053290.45@tvgsbejvaqbjf.bet","threadId":"50623","inReplyTo":"20190301220112.GA14966@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-02T19:54:55Z","receivedAt":"2019-03-02T19:55:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 1 Mar 2019, Jeff King wrote:\n\n> On Fri, Mar 01, 2019 at 04:54:15PM -0500, Jeff King wrote:\n> \n> > The one thing we do lose, though, is make's parallelization. It would\n> > probably be possible to actually shove this into a sub-make which\n> > defined the hdr-check rules, but I don't know how complicated that would\n> > become.\n> \n> This seems to work, though it's kind of horrid.\n> \n> It costs at least one extra process to run \"make hdr-check\", and\n> probably more for things like $(GIT_VERSION) that the Makefile include\n> likely triggers. But when you're not running hdr-check (which is the\n> norm), it's zero-cost.\n\nIf we want to go that route (and I am not saying we should), we could\neasily just add another target (say, `check-headers`) that requires a list\nof headers to check to be passed in via a Makefile variable that is\ndefined via the command-line.\n\nCiao,\nDscho\n\n> \n> -Peff\n> \n> diff --git a/Makefile b/Makefile\n> index c5240942f2..ab6ecf450e 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2735,16 +2735,8 @@ $(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> +hdr-check:\n> +\t$(MAKE) -f hdr-check.mak hdr-check\n> \n> .PHONY: style\n> style:\n> diff --git a/hdr-check.mak b/hdr-check.mak\n> new file mode 100644\n> index 0000000000..b8924afa90\n> --- /dev/null\n> +++ b/hdr-check.mak\n> @@ -0,0 +1,12 @@\n> +include Makefile\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"},{"id":"370471","messageId":"nycvar.QRO.7.76.6.1903022056500.45@tvgsbejvaqbjf.bet","threadId":"50623","inReplyTo":"20190301215414.GB1518@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-02T19:57:57Z","receivedAt":"2019-03-02T19:58:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 1 Mar 2019, Jeff King wrote:\n\n> On Fri, Mar 01, 2019 at 04:36:19PM -0500, Jeff King wrote:\n> \n> > > This has one notable consequence: we no longer include\n> > > `command-list.h` in `LIB_H`, as it is a generated file, not a\n> > > tracked one.\n> > \n> > We should be able to add back $(GENERATED_H) as appropriate. I see you\n> > did it for the non-computed-dependencies case. Couldn't we do the same\n> > for $(LOCALIZED_C) and $(CHK_HDRS)?\n> \n> Answering my own question somewhat: $(CHK_HDRS) explicitly ignores the\n> generated headers, so we wouldn't want to bother re-adding it there\n> anyway. Possibly $(LOCALIZED_C) would care, though. The generated file\n> does have translatable strings in it.\n\nYes, and it is defined thusly:\n\n\tLOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)\n\nSo it does include the generated headers explictly anyway, with or without\nmy patch.\n\nCiao,\nDscho\n"},{"id":"370472","messageId":"nycvar.QRO.7.76.6.1903022058230.45@tvgsbejvaqbjf.bet","threadId":"50623","inReplyTo":"20190301213619.GA1518@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-02T20:05:24Z","receivedAt":"2019-03-02T20:05:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 1 Mar 2019, Jeff King wrote:\n\n> On Fri, Mar 01, 2019 at 11:57:46AM -0800, Johannes Schindelin via GitGitGadget wrote:\n> \n> > In d85b0dff72 (Makefile: use `find` to determine static header\n> > dependencies, 2014-08-25), we switched from a static list of header\n> > files to a dynamically-generated one, asking `find` to enumerate them.\n> > \n> > Back in those days, we did not use `$(LIB_H)` by default, and many a\n> > `make` implementation seems smart enough not to run that `find` command\n> > in that case, so it was deemed okay to run `find` for special targets\n> > requiring this macro.\n> > \n> > However, as of ebb7baf02f (Makefile: add a hdr-check target,\n> > 2018-09-19), $(LIB_H) is part of a global rule and therefore must be\n> > expanded. Meaning: this `find` command has to be run upon every\n> > `make` invocation. In the presence of many a worktree, this can tax the\n> > developers' patience quite a bit.\n> \n> I'm confused about this part. We don't run hdr-check by default. Why\n> would make need the value of $(LIB_H)? Yet empirically it does run find.\n> \n> Worse, it seems to actually run it _three times_. Once for the $(HCO)\n> target, once for the .PHONY, and once for the hdr-check target. I think\n> the .PHONY one is unavoidable (it doesn't know which names we might\n> otherwise be building should be marked), but the other two seem like\n> bugs in make (or at least pessimisations).\n> \n> It makes me wonder if we'd be better off pushing hdr-check into a\n> separate script. It doesn't actually use make's dependency tree in any\n> meaningful way. And then regular invocations wouldn't even have to pay\n> the `ls-files` price.\n> \n> If we are going to keep it in the Makefile, we should probably use a\n> \":=\" rule to avoid running it three times.\n\nGood point.\n\n> > Even in the absence of worktrees or other untracked files and\n> > directories, the cost of I/O to generate that list of header files is\n> > simply a lot larger than a simple `git ls-files` call.\n> > \n> > Therefore, just like in 335339758c (Makefile: ask \"ls-files\" to list\n> > source files if available, 2011-10-18), we now prefer to use `git\n> > ls-files` to enumerate the header files to enumerating them via `find`,\n> > falling back to the latter if the former failed (which would be the case\n> > e.g. in a worktree that was extracted from a source .tar file rather\n> > than from a clone of Git's sources).\n> \n> That seems reasonable (regardless of whether it is in a script or in the\n> Makefile). Another option is to use -maxdepth, but that involves\n> guessing how deep people might actually put header files.\n\nIt would also fail to work when somebody clones an unrelated repository\nthat contains header files in the top-level directory into the Git\nworktree. I know somebody like that: me.\n\n> > This has one notable consequence: we no longer include `command-list.h`\n> > in `LIB_H`, as it is a generated file, not a tracked one.\n> \n> We should be able to add back $(GENERATED_H) as appropriate. I see you\n> did it for the non-computed-dependencies case. Couldn't we do the same\n> for $(LOCALIZED_C) and $(CHK_HDRS)?\n\nAs you figured out, CHK_HDRS *specifically* excludes the generated\nheaders, and as I pointed out: LOCALIZED_C includes $(GENERATED_H)\nexplicitly.\n\nSo I think we're good on that front.\n\n> > Likewise, we no longer include not-yet-tracked header files in `LIB_H`.\n> \n> I think that's probably OK.\n\nIt does potentially make developing new patches more challenging. I, for\none, do not immediately stage every new file I add, especially not header\nfiles. But then, I rarely recompile after only editing such a new header\nfile (I would more likely modify also the source file that includes that\nheader).\n\nSo while I think this issue could potentially cause problems only *very*\nrarely, I think that it would be a really terribly confusing thing if it\nhappened.\n\nBut I probably worry too much about it?\n\nCiao,\nDscho\n"},{"id":"370539","messageId":"20190303170824.GB23811@sigill.intra.peff.net","threadId":"50623","inReplyTo":"nycvar.QRO.7.76.6.1903022053290.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-03T17:08:25Z","receivedAt":"2019-03-03T17:08:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 02, 2019 at 08:54:55PM +0100, Johannes Schindelin wrote:\n\n> On Fri, 1 Mar 2019, Jeff King wrote:\n> \n> > On Fri, Mar 01, 2019 at 04:54:15PM -0500, Jeff King wrote:\n> > \n> > > The one thing we do lose, though, is make's parallelization. It would\n> > > probably be possible to actually shove this into a sub-make which\n> > > defined the hdr-check rules, but I don't know how complicated that would\n> > > become.\n> > \n> > This seems to work, though it's kind of horrid.\n> > \n> > It costs at least one extra process to run \"make hdr-check\", and\n> > probably more for things like $(GIT_VERSION) that the Makefile include\n> > likely triggers. But when you're not running hdr-check (which is the\n> > norm), it's zero-cost.\n> \n> If we want to go that route (and I am not saying we should), we could\n> easily just add another target (say, `check-headers`) that requires a list\n> of headers to check to be passed in via a Makefile variable that is\n> defined via the command-line.\n\nYeah, that was actually going to be my initial attempt before I tried\nthe \"include\" shortcut. :) I'd worry slightly about things like\ncommand-line limits, but maybe our list isn't big enough to justify\nthat.\n\n-Peff\n"},{"id":"370541","messageId":"20190303171141.GC23811@sigill.intra.peff.net","threadId":"50623","inReplyTo":"nycvar.QRO.7.76.6.1903022056500.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-03T17:11:42Z","receivedAt":"2019-03-03T17:11:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 02, 2019 at 08:57:57PM +0100, Johannes Schindelin wrote:\n\n> Hi Peff,\n> \n> On Fri, 1 Mar 2019, Jeff King wrote:\n> \n> > On Fri, Mar 01, 2019 at 04:36:19PM -0500, Jeff King wrote:\n> > \n> > > > This has one notable consequence: we no longer include\n> > > > `command-list.h` in `LIB_H`, as it is a generated file, not a\n> > > > tracked one.\n> > > \n> > > We should be able to add back $(GENERATED_H) as appropriate. I see you\n> > > did it for the non-computed-dependencies case. Couldn't we do the same\n> > > for $(LOCALIZED_C) and $(CHK_HDRS)?\n> > \n> > Answering my own question somewhat: $(CHK_HDRS) explicitly ignores the\n> > generated headers, so we wouldn't want to bother re-adding it there\n> > anyway. Possibly $(LOCALIZED_C) would care, though. The generated file\n> > does have translatable strings in it.\n> \n> Yes, and it is defined thusly:\n> \n> \tLOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)\n> \n> So it does include the generated headers explictly anyway, with or without\n> my patch.\n\nOops, I should have read the source more carefully before responding.\n\nIs it worth mentioning in the commit message? I.e., something like:\n\n This has one notable consequence: we no longer include\n `command-list.h` in `LIB_H`, as it is a generated file, not a tracked\n one, but that is easily worked around. Of the three sites that use\n `LIB_H`, two (`LOCALIZED_C` and `CHK_HDRS`) already handle generated\n headers separately. In the third, the computed-dependency fallback,\n we can just add in a reference to $(GENERATED_H).\n\n-Peff\n"},{"id":"370547","messageId":"20190303171951.GD23811@sigill.intra.peff.net","threadId":"50623","inReplyTo":"nycvar.QRO.7.76.6.1903022058230.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-03T17:19:51Z","receivedAt":"2019-03-03T17:19:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 02, 2019 at 09:05:24PM +0100, Johannes Schindelin wrote:\n\n> > That seems reasonable (regardless of whether it is in a script or in the\n> > Makefile). Another option is to use -maxdepth, but that involves\n> > guessing how deep people might actually put header files.\n> \n> It would also fail to work when somebody clones an unrelated repository\n> that contains header files in the top-level directory into the Git\n> worktree. I know somebody like that: me.\n\nGood point.\n\nBy the way, \"make hdr-check\" already fails for me on master, as I do not have\nlibgcrypt installed, and it unconditionally checks sha256/gcrypt.h.\n\nI wonder if this actually does point to hdr-check needing to be smarter\nabout checking the headers that are actually used in compilation on your\nplatform. Or if that file should just be added to the set of excluded\nheaders.\n\n> > We should be able to add back $(GENERATED_H) as appropriate. I see you\n> > did it for the non-computed-dependencies case. Couldn't we do the same\n> > for $(LOCALIZED_C) and $(CHK_HDRS)?\n> \n> As you figured out, CHK_HDRS *specifically* excludes the generated\n> headers, and as I pointed out: LOCALIZED_C includes $(GENERATED_H)\n> explicitly.\n> \n> So I think we're good on that front.\n\nYeah, agreed.\n\n> > > Likewise, we no longer include not-yet-tracked header files in `LIB_H`.\n> > \n> > I think that's probably OK.\n> \n> It does potentially make developing new patches more challenging. I, for\n> one, do not immediately stage every new file I add, especially not header\n> files. But then, I rarely recompile after only editing such a new header\n> file (I would more likely modify also the source file that includes that\n> header).\n> \n> So while I think this issue could potentially cause problems only *very*\n> rarely, I think that it would be a really terribly confusing thing if it\n> happened.\n> \n> But I probably worry too much about it?\n\nI think it's not ideal, but it's probably an acceptable tradeoff. The\nLIB_H list is used for three things:\n\n - hdr-check, which I'd think would generally be run periodically on a\n full tree to catch any new header breakages. But I dunno, maybe\n people want to run it as soon as they've written new code.\n\n - the .po generation, which generally is a separate workflow from\n writing new header files\n\n - the header-dependency fallback code. This is definitely the place\n where somebody just adding a new header file and running \"make\"\n might get bitten. But it only kicks in for ancient, crappy compilers\n that don't do dependency computation; so I think most developers\n would not be using it. (this is your cue to explain to me how\n some workflow involving MSVC does not compute dependencies, and\n I'm unknowingly dismissing a large portion developers ;) ).\n\n-Peff\n"},{"id":"370560","messageId":"8e4b7966-7a3e-a081-bfc1-9e60303b8eab@ramsayjones.plus.com","threadId":"50623","inReplyTo":"20190303171951.GD23811@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-03T21:30:17Z","receivedAt":"2019-03-03T21:30:26Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 03/03/2019 17:19, Jeff King wrote:\n> On Sat, Mar 02, 2019 at 09:05:24PM +0100, Johannes Schindelin wrote:\n> \n>>> That seems reasonable (regardless of whether it is in a script or in the\n>>> Makefile). Another option is to use -maxdepth, but that involves\n>>> guessing how deep people might actually put header files.\n>>\n>> It would also fail to work when somebody clones an unrelated repository\n>> that contains header files in the top-level directory into the Git\n>> worktree. I know somebody like that: me.\n> \n> Good point.\n\n[Sorry for the late reply - I have been AWOL this weekend and\nI am only just catching up with email! :-D ]\n\nSo, tl;dr: soon, I will be submitting a patch to remove the\n'hdr-check' target completely, for now anyway.\n\n> By the way, \"make hdr-check\" already fails for me on master, as I do not have\n> libgcrypt installed, and it unconditionally checks sha256/gcrypt.h.\n\nYep, for me too. There is a similar problem if you build with\nNO_CURL and don't have the 'curl/curl.h' header file, etc ...\n\nThe first version of the 'hdr-check' target re-introduced a static\nlist of header files, but I didn't think people would appreciate\nhaving to maintain the list manually, so I gave up on that!\n\nThe next version was essentially the same patch that started this\nthread from Johannes (ie. the 'git ls-files' patch), given that\nthe 'tags' targets had found it necessary. However, when I did some\n'informal' timing tests (ie 5x 'time make ...' and average), this\ndid not make any noticeable difference for me (_even_ on cygwin!). ;-)\n\nOf course, I had already made the mistake of trying to re-use\nsomething that was 'handy' (ie. LIB_H) and already there.\nHowever, LIB_H wasn't quite what I wanted - I needed a sub-set\nof it.\n\nSo, I already have a 'hdr-check-fixup' branch (I think I have\nalready mentioned it), in which the first commit looks like:\n\n diff --git a/Makefile b/Makefile\n index c5240942f2..3401d1b9fb 100644\n --- a/Makefile\n +++ b/Makefile\n @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n .PHONY: sparse $(SP_OBJ)\n sparse: $(SP_OBJ)\n \n +HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \\\n + sha1dc/*.h refs/*.h vcs-svn/*.h)\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 +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS))\n HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n \n $(HCO): %.hco: %.h FORCE\n \n\n... which drops the use of LIB_H entirely.\n\nNow, I have timed this and, for me, it no faster ... (I suspect\nthat it would be faster for Johannes, but it would still cause\na problem if you have 'top-level header files from some other\nrepo ...').\n\nSo, if we really need to solve the problem, allowing for some\nunrelated headers in your worktree, then we can only use a\nstatic list, or a 'git ls-files' approach.\n\nAnyway, for now, since I seem to be the only person using this\ntarget, I think we should just remove it while I think again.\n(I can put it in my config.mak, so there will be no loss for me).\n\nATB,\nRamsay Jones\n"},{"id":"370589","messageId":"nycvar.QRO.7.76.6.1903041206300.45@tvgsbejvaqbjf.bet","threadId":"50623","inReplyTo":"20190303171951.GD23811@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-04T11:08:41Z","receivedAt":"2019-03-04T11:08:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sun, 3 Mar 2019, Jeff King wrote:\n\n> By the way, \"make hdr-check\" already fails for me on master, as I do not\n> have libgcrypt installed, and it unconditionally checks sha256/gcrypt.h.\n\nOh? I thought it'd fail only on Windows, so I have not submitted\nhttps://github.com/dscho/git/commit/fix-hdr-check yet:\n\n-- snipsnap --\nFrom c13d9985284d4b452db0d95b6949e02c533db634 Mon Sep 17 00:00:00 2001\nFrom: Johannes Schindelin \nDate: Sat, 23 Feb 2019 20:38:40 +0100\nSubject: [PATCH] hdr-check: make it work on Windows\n\nOn Windows, we define a specific set of pre-processor macros, among\nother reasons: to avoid including syslog.h (which is not available on\nWindows).\n\nThe hdr-check target did not use those definitions, resulting in a\nfailure to include said syslog.h.\n\nTo fix that, let's let the hdr-check target make use of ALL_CFLAGS,\nwhich does include the pre-processor macros that would e.g. skip the\nconditional block including syslog.h.\n\nSigned-off-by: Johannes Schindelin \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex c5240942f2..b9f00f0ea0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2741,7 +2741,7 @@ 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+\t$(QUIET_HDR)$(CC) $(ALL_CFLAGS) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n \n .PHONY: hdr-check $(HCO)\n hdr-check: $(HCO)\n-- \n2.20.1.windows.1.1576.g37ac248661.dirty\n\n\n"},{"id":"370597","messageId":"nycvar.QRO.7.76.6.1903041210410.45@tvgsbejvaqbjf.bet","threadId":"50623","inReplyTo":"8e4b7966-7a3e-a081-bfc1-9e60303b8eab@ramsayjones.plus.com","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-04T12:38:13Z","receivedAt":"2019-03-04T12:38:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ramsay,\n\nOn Sun, 3 Mar 2019, Ramsay Jones wrote:\n\n> On 03/03/2019 17:19, Jeff King wrote:\n> > On Sat, Mar 02, 2019 at 09:05:24PM +0100, Johannes Schindelin wrote:\n> > \n> >>> That seems reasonable (regardless of whether it is in a script or in the\n> >>> Makefile). Another option is to use -maxdepth, but that involves\n> >>> guessing how deep people might actually put header files.\n> >>\n> >> It would also fail to work when somebody clones an unrelated repository\n> >> that contains header files in the top-level directory into the Git\n> >> worktree. I know somebody like that: me.\n> > \n> > Good point.\n> \n> [Sorry for the late reply - I have been AWOL this weekend and\n> I am only just catching up with email! :-D ]\n\nYou should never feel the need to apologize for spending a weekend away\nfrom the keyboard (AKA \"having a life\").\n\n> So, tl;dr: soon, I will be submitting a patch to remove the\n> 'hdr-check' target completely, for now anyway.\n\nYou mentioned later that you might be the only person using that target,\nand if that were so, I would agree.\n\nHowever, I started looking into adding `hdr-check` to the Azure Pipeline\nwe have (which would make a *ton* of sense, if you ask me), along with the\n`sparse` thing we talked about.\n\nSo I am a bit opposed to removing that target.\n\n> > By the way, \"make hdr-check\" already fails for me on master, as I do\n> > not have libgcrypt installed, and it unconditionally checks\n> > sha256/gcrypt.h.\n> \n> Yep, for me too. There is a similar problem if you build with\n> NO_CURL and don't have the 'curl/curl.h' header file, etc ...\n\nI think all it needs to do is to pass `$(ALL_CFLAGS)` to gcc (although we\nmight need to add something like `#if defined(SHA256_GCRYPT) ... #endif`\nguards to `sha256/gcrypt.h` to make it work in Peff's case).\n\nBut given that this target really is meant to catch all kinds of errors,\nI'd be in favor of declaring that target requiring things like libgcrypt's\nheader files to be installed. We can easily make that happen in our CI\nbuilds.\n\n> The first version of the 'hdr-check' target re-introduced a static list\n> of header files, but I didn't think people would appreciate having to\n> maintain the list manually, so I gave up on that!\n> \n> The next version was essentially the same patch that started this thread\n> from Johannes (ie. the 'git ls-files' patch), given that the 'tags'\n> targets had found it necessary. However, when I did some 'informal'\n> timing tests (ie 5x 'time make ...' and average), this did not make any\n> noticeable difference for me (_even_ on cygwin!). ;-)\n\nMy complaint is not about the speed in a regular clone, but about *my*\nspecial clone, where I have some 50+ worktrees (that I really like to keep\nin the same directory, thankyouverymuch, I ignore them via\n`.git/info/exclude`, and I really like to have them all tucked away neatly\nunder the `/usr/src/git/` directory) and also some 30+ cloned repositories\nin `3rdparty/` for random testing (including a bare clone of the Linux\nkernel, of course also ignored via `.git/info/exclude`).\n\nIn that scenario, my version of `find` takes ages, only to finally even\nthrow a segmentation fault (!).\n\n> So, I already have a 'hdr-check-fixup' branch (I think I have\n> already mentioned it), in which the first commit looks like:\n> \n> diff --git a/Makefile b/Makefile\n> index c5240942f2..3401d1b9fb 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n> .PHONY: sparse $(SP_OBJ)\n> sparse: $(SP_OBJ)\n> \n> +HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \\\n> + sha1dc/*.h refs/*.h vcs-svn/*.h)\n\nBut of course that would also share at least to a lesser extend the\nshortcomings of a static list.\n\nI still like the `ls-files` method the best.\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> +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS))\n> HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n> \n> $(HCO): %.hco: %.h FORCE\n> \n> \n> ... which drops the use of LIB_H entirely.\n> \n> Now, I have timed this and, for me, it no faster ... (I suspect\n> that it would be faster for Johannes, but it would still cause\n> a problem if you have 'top-level header files from some other\n> repo ...').\n> \n> So, if we really need to solve the problem, allowing for some\n> unrelated headers in your worktree, then we can only use a\n> static list, or a 'git ls-files' approach.\n\nOr we can use `ls-files`, and fall back to your wildcard idea if\n`ls-files` fails for some reason (typically because `.git/` is missing,\ne.g. in case of an unpacked source .tar).\n\n> Anyway, for now, since I seem to be the only person using this\n> target, I think we should just remove it while I think again.\n> (I can put it in my config.mak, so there will be no loss for me).\n\nAs I said above, I would rather keep it, with the `ls-files` and `:=`\nfixup.\n\nIf others *really* feel strongly about lazy-evaluating `LIB_H`, then I\nwould *additionally* use Peff's sub-make hack. But only if others feel\nstrongly about it, because I, for one, certainly don't.\n\nCiao,\nDscho\n"},{"id":"370601","messageId":"pull.130.v2.git.gitgitgadget@gmail.com","threadId":"50623","inReplyTo":"pull.130.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] Avoid calling find in the Makefile, if possible","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-04T13:47:05Z","receivedAt":"2019-03-04T13:47:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I noticed this quite a bit of time ago, but did not get a chance to look\ninto it in detail: all of a sudden, make started really slowly over here.\n\nThe culprit turned out to be a find call, which was in the Makefile for\nages, so I was puzzled why it only caused problems recently.\n\nAfter some digging, I found out that the hdr-check target is the culprit:\nbefore it was introduced, $(LIB_H) did not need to be expanded, and as a\nconsequence the find call was not executed. Once hdr-check made it into \nmaster, though, $(LIB_H) must be expanded to define that rule.\n\nSince I have tons of worktrees as subdirectories of my principal Git clone,\nand since I also have a 3rdparty/ directory with tons of repositories I use\nfor various testing/contributing purposes, this find is quite a little slow\nover here.\n\nSo here is my suggested fix. It is based on similar code we already had in\nthe Makefile, obviously also intended to avoid an expensive find invocation.\n\nChanges since v1:\n\n * Since LIB_H's lazy evaluation kicks in all the time anyway, changed the = \n to := to avoid evaluating it three times.\n * Clarified in the commit message that the existing sites using $(LIB_H) \n are not affected by this change (or at least not affected as long as no\n untracked header files are included in .c files).\n\nJohannes Schindelin (1):\n Makefile: use `git ls-files` to list header files, if possible\n\n Makefile | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\n\nbase-commit: c65a2884eae159bad540135479bc8afe20ff62d1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-130%2Fdscho%2Favoid-find-in-makefile-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-130/dscho/avoid-find-in-makefile-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/130\n\nRange-diff vs v1:\n\n 1: 0b5529406b ! 1: cb253bd0cf Makefile: use `git ls-files` to list header files, if possible\n @@ -29,7 +29,11 @@\n than from a clone of Git's sources).\n \n This has one notable consequence: we no longer include `command-list.h`\n - in `LIB_H`, as it is a generated file, not a tracked one.\n + in `LIB_H`, as it is a generated file, not a tracked one, but that is\n + easily worked around. Of the three sites that use `LIB_H`, two\n + (`LOCALIZED_C` and `CHK_HDRS`) already handle generated headers\n + separately. In the third, the computed-dependency fallback, we can just\n + add in a reference to $(GENERATED_H).\n \n Likewise, we no longer include not-yet-tracked header files in `LIB_H`.\n \n @@ -46,7 +50,7 @@\n GENERATED_H += command-list.h\n \n -LIB_H = $(shell $(FIND) . \\\n -+LIB_H = $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \\\n ++LIB_H := $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \\\n +\t$(FIND) . \\\n \t-name .git -prune -o \\\n \t-name t -prune -o \\\n\n-- \ngitgitgadget\n"},{"id":"370602","messageId":"cb253bd0cf2896cf31516079a89ec2dab21032cf.1551707225.git.gitgitgadget@gmail.com","threadId":"50623","inReplyTo":"pull.130.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-04T13:47:06Z","receivedAt":"2019-03-04T13:47:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn d85b0dff72 (Makefile: use `find` to determine static header\ndependencies, 2014-08-25), we switched from a static list of header\nfiles to a dynamically-generated one, asking `find` to enumerate them.\n\nBack in those days, we did not use `$(LIB_H)` by default, and many a\n`make` implementation seems smart enough not to run that `find` command\nin that case, so it was deemed okay to run `find` for special targets\nrequiring this macro.\n\nHowever, as of ebb7baf02f (Makefile: add a hdr-check target,\n2018-09-19), $(LIB_H) is part of a global rule and therefore must be\nexpanded. Meaning: this `find` command has to be run upon every\n`make` invocation. In the presence of many a worktree, this can tax the\ndevelopers' patience quite a bit.\n\nEven in the absence of worktrees or other untracked files and\ndirectories, the cost of I/O to generate that list of header files is\nsimply a lot larger than a simple `git ls-files` call.\n\nTherefore, just like in 335339758c (Makefile: ask \"ls-files\" to list\nsource files if available, 2011-10-18), we now prefer to use `git\nls-files` to enumerate the header files to enumerating them via `find`,\nfalling back to the latter if the former failed (which would be the case\ne.g. in a worktree that was extracted from a source .tar file rather\nthan from a clone of Git's sources).\n\nThis has one notable consequence: we no longer include `command-list.h`\nin `LIB_H`, as it is a generated file, not a tracked one, but that is\neasily worked around. Of the three sites that use `LIB_H`, two\n(`LOCALIZED_C` and `CHK_HDRS`) already handle generated headers\nseparately. In the third, the computed-dependency fallback, we can just\nadd in a reference to $(GENERATED_H).\n\nLikewise, we no longer include not-yet-tracked header files in `LIB_H`.\n\nGiven the speed improvements, these consequences seem a comparably small\nprice to pay.\n\nSigned-off-by: Johannes Schindelin \n---\n Makefile | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex c5240942f2..0c4712cb48 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -841,7 +841,8 @@ VCSSVN_LIB = vcs-svn/lib.a\n \n GENERATED_H += command-list.h\n \n-LIB_H = $(shell $(FIND) . \\\n+LIB_H := $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \\\n+\t$(FIND) . \\\n \t-name .git -prune -o \\\n \t-name t -prune -o \\\n \t-name Documentation -prune -o \\\n@@ -2363,7 +2364,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)\n+$(OBJECTS): $(LIB_H) $(GENERATED_H)\n endif\n \n exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX\n-- \ngitgitgadget\n"},{"id":"370635","messageId":"b4ea52cb-bbd1-5b55-d3d8-b09285bb2e6f@ramsayjones.plus.com","threadId":"50623","inReplyTo":"nycvar.QRO.7.76.6.1903041210410.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-04T20:31:35Z","receivedAt":"2019-03-04T20:31:41Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/03/2019 12:38, Johannes Schindelin wrote:\n\n>> So, tl;dr: soon, I will be submitting a patch to remove the\n>> 'hdr-check' target completely, for now anyway.\n> \n> You mentioned later that you might be the only person using that target,\n> and if that were so, I would agree.\n\nIt is obviously difficult to judge such a thing, but I suspect\nthat the number is very low, if not exactly one. ;-)\n\n> However, I started looking into adding `hdr-check` to the Azure Pipeline\n> we have (which would make a *ton* of sense, if you ask me), along with the\n\nHmm, I am in two minds about this - I feel duty bound to warn you\nthat, if you go ahead with this idea, you will likely create a\nrod for your own back! :-D\n\nIf the false positive rate can be kept reasonably low, then this\nwould be a good idea. However, some projects are just not a good\nfit for this kind of thing (and Git may be one of them).\n\n> `sparse` thing we talked about.\n> \n> So I am a bit opposed to removing that target.\n\nOK, I took the 'nuclear' route because this affects people (to a\ngreater or lesser degree) even when they don't know/care about\nthis target. If it is just (or mostly) me, then it didn't seem\nworth causing issues for you (or anyone else working on GTW?).\n\n>>> By the way, \"make hdr-check\" already fails for me on master, as I do\n>>> not have libgcrypt installed, and it unconditionally checks\n>>> sha256/gcrypt.h.\n>>\n>> Yep, for me too. There is a similar problem if you build with\n>> NO_CURL and don't have the 'curl/curl.h' header file, etc ...\n> \n> I think all it needs to do is to pass `$(ALL_CFLAGS)` to gcc (although we\n> might need to add something like `#if defined(SHA256_GCRYPT) ... #endif`\n> guards to `sha256/gcrypt.h` to make it work in Peff's case).\n\nJust adding $(ALL_CFLAGS) to the command-line is not a cure-all,\nunfortunately! :( Also, the 'sha256/gcrypt.h' should be excluded\nfrom the list of headers to check, conditionally, based on the\nbuild variable SHA256_GCRYPT. \n\n> But given that this target really is meant to catch all kinds of errors,\n> I'd be in favor of declaring that target requiring things like libgcrypt's\n> header files to be installed. We can easily make that happen in our CI\n> builds.\n\nThat would help, but you may not be able to cover all the platform\nspecific build options available. dunno. (I haven't given it enough\nthought).\n\n>> The next version was essentially the same patch that started this thread\n>> from Johannes (ie. the 'git ls-files' patch), given that the 'tags'\n>> targets had found it necessary. However, when I did some 'informal'\n>> timing tests (ie 5x 'time make ...' and average), this did not make any\n>> noticeable difference for me (_even_ on cygwin!). ;-)\n> \n> My complaint is not about the speed in a regular clone, but about *my*\n> special clone, where I have some 50+ worktrees (that I really like to keep\n> in the same directory, thankyouverymuch, I ignore them via\n> `.git/info/exclude`, and I really like to have them all tucked away neatly\n> under the `/usr/src/git/` directory) and also some 30+ cloned repositories\n> in `3rdparty/` for random testing (including a bare clone of the Linux\n> kernel, of course also ignored via `.git/info/exclude`).\n\nOh, wow! I can't imagine ever doing anything like that. However,\nit is a perfectly valid setup and the build system should not\nmake it hard for you to work the way you want.\n\n> In that scenario, my version of `find` takes ages, only to finally even\n> throw a segmentation fault (!).\nAt least the segfault curtailed your misery! :-D\n\n>> So, I already have a 'hdr-check-fixup' branch (I think I have\n>> already mentioned it), in which the first commit looks like:\n>>\n>> diff --git a/Makefile b/Makefile\n>> index c5240942f2..3401d1b9fb 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n>> .PHONY: sparse $(SP_OBJ)\n>> sparse: $(SP_OBJ)\n>> \n>> +HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \\\n>> + sha1dc/*.h refs/*.h vcs-svn/*.h)\n> \n> But of course that would also share at least to a lesser extend the\n> shortcomings of a static list.\n\nYes, but it is no worse than ...\n\n> I still like the `ls-files` method the best.\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\n... the original, manually maintained, exception list!\n\n(This patch was written before the 'sha256/' directory existed.\nSo the fact that it 'fixes' the GCRYPT error is simply an accident\nof timing! Today, it would be conditionally added to the exception\nlist. Yes, I watched that error go from the 'pu' branch down to\nthe 'master' branch).\n\n>> +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS))\n>> HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n>> \n>> $(HCO): %.hco: %.h FORCE\n>> \n>>\n>> ... which drops the use of LIB_H entirely.\n>>\n>> So, if we really need to solve the problem, allowing for some\n>> unrelated headers in your worktree, then we can only use a\n>> static list, or a 'git ls-files' approach.\n> \n> Or we can use `ls-files`, and fall back to your wildcard idea if\n> `ls-files` fails for some reason (typically because `.git/` is missing,\n> e.g. in case of an unpacked source .tar).\n\nYes, I think an 'git ls-files' approach is the way to go. (I am not\nsure that the 'hdr-check' target would be of any use 'offline' at\nall, but I suppose we could use a generated file in that case).\n\n>> Anyway, for now, since I seem to be the only person using this\n>> target, I think we should just remove it while I think again.\n>> (I can put it in my config.mak, so there will be no loss for me).\n> \n> As I said above, I would rather keep it, with the `ls-files` and `:=`\n> fixup.\n\nI would be happy with that, if you are. :-D\n\nATB,\nRamsay Jones\n\n"},{"id":"370636","messageId":"ae65dbe2-cdcb-175b-af58-ab8482270ab9@ramsayjones.plus.com","threadId":"50623","inReplyTo":"cb253bd0cf2896cf31516079a89ec2dab21032cf.1551707225.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-04T20:38:42Z","receivedAt":"2019-03-04T20:38:46Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/03/2019 13:47, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> In d85b0dff72 (Makefile: use `find` to determine static header\n> dependencies, 2014-08-25), we switched from a static list of header\n> files to a dynamically-generated one, asking `find` to enumerate them.\n> \n> Back in those days, we did not use `$(LIB_H)` by default, and many a\n> `make` implementation seems smart enough not to run that `find` command\n> in that case, so it was deemed okay to run `find` for special targets\n> requiring this macro.\n> \n> However, as of ebb7baf02f (Makefile: add a hdr-check target,\n> 2018-09-19), $(LIB_H) is part of a global rule and therefore must be\n> expanded. Meaning: this `find` command has to be run upon every\n> `make` invocation. In the presence of many a worktree, this can tax the\n> developers' patience quite a bit.\n> \n> Even in the absence of worktrees or other untracked files and\n> directories, the cost of I/O to generate that list of header files is\n> simply a lot larger than a simple `git ls-files` call.\n> \n> Therefore, just like in 335339758c (Makefile: ask \"ls-files\" to list\n> source files if available, 2011-10-18), we now prefer to use `git\n> ls-files` to enumerate the header files to enumerating them via `find`,\n> falling back to the latter if the former failed (which would be the case\n> e.g. in a worktree that was extracted from a source .tar file rather\n> than from a clone of Git's sources).\n> \n> This has one notable consequence: we no longer include `command-list.h`\n> in `LIB_H`, as it is a generated file, not a tracked one, but that is\n\nHeh, just to be _unnecessarily_ picky, but this is not always true.\nThe 'command-list.h' header is _sometimes_ not included in the LIB_H\nvariable - it simply depends on whether it has been generated by the\ntime the $(FIND) is called.\n\nObviously, not worth a re-roll. Otherwise, this LGTM.\n\nThanks!\n\nATB,\nRamsay Jones\n\n> easily worked around. Of the three sites that use `LIB_H`, two\n> (`LOCALIZED_C` and `CHK_HDRS`) already handle generated headers\n> separately. In the third, the computed-dependency fallback, we can just\n> add in a reference to $(GENERATED_H).\n> \n> Likewise, we no longer include not-yet-tracked header files in `LIB_H`.\n> \n> Given the speed improvements, these consequences seem a comparably small\n> price to pay.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> Makefile | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/Makefile b/Makefile\n> index c5240942f2..0c4712cb48 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -841,7 +841,8 @@ VCSSVN_LIB = vcs-svn/lib.a\n> \n> GENERATED_H += command-list.h\n> \n> -LIB_H = $(shell $(FIND) . \\\n> +LIB_H := $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \\\n> +\t$(FIND) . \\\n> \t-name .git -prune -o \\\n> \t-name t -prune -o \\\n> \t-name Documentation -prune -o \\\n> @@ -2363,7 +2364,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)\n> +$(OBJECTS): $(LIB_H) $(GENERATED_H)\n> endif\n> \n> exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX\n> \n"},{"id":"370639","messageId":"99a38032-1182-7777-ccb2-1de97c284a84@ramsayjones.plus.com","threadId":"50623","inReplyTo":"ae65dbe2-cdcb-175b-af58-ab8482270ab9@ramsayjones.plus.com","subject":"Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-04T21:01:54Z","receivedAt":"2019-03-04T21:01:58Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/03/2019 20:38, Ramsay Jones wrote:\n> \n> \n> On 04/03/2019 13:47, Johannes Schindelin via GitGitGadget wrote:\n>> From: Johannes Schindelin \n>>\n>> In d85b0dff72 (Makefile: use `find` to determine static header\n>> dependencies, 2014-08-25), we switched from a static list of header\n>> files to a dynamically-generated one, asking `find` to enumerate them.\n>>\n>> Back in those days, we did not use `$(LIB_H)` by default, and many a\n>> `make` implementation seems smart enough not to run that `find` command\n>> in that case, so it was deemed okay to run `find` for special targets\n>> requiring this macro.\n>>\n>> However, as of ebb7baf02f (Makefile: add a hdr-check target,\n>> 2018-09-19), $(LIB_H) is part of a global rule and therefore must be\n>> expanded. Meaning: this `find` command has to be run upon every\n>> `make` invocation. In the presence of many a worktree, this can tax the\n>> developers' patience quite a bit.\n>>\n>> Even in the absence of worktrees or other untracked files and\n>> directories, the cost of I/O to generate that list of header files is\n>> simply a lot larger than a simple `git ls-files` call.\n>>\n>> Therefore, just like in 335339758c (Makefile: ask \"ls-files\" to list\n>> source files if available, 2011-10-18), we now prefer to use `git\n>> ls-files` to enumerate the header files to enumerating them via `find`,\n>> falling back to the latter if the former failed (which would be the case\n>> e.g. in a worktree that was extracted from a source .tar file rather\n>> than from a clone of Git's sources).\n>>\n>> This has one notable consequence: we no longer include `command-list.h`\n>> in `LIB_H`, as it is a generated file, not a tracked one, but that is\n> \n> Heh, just to be _unnecessarily_ picky, but this is not always true.\n> The 'command-list.h' header is _sometimes_ not included in the LIB_H\n> variable - it simply depends on whether it has been generated by the\n> time the $(FIND) is called.\n> \n> Obviously, not worth a re-roll. Otherwise, this LGTM.\n\nAhem! Obviously, I didn't read the commit message closely enough!\n\nHowever, _before_ this change, then 'command-list.h' was sometimes\nnot included in $(LIB_H) ...\n\nSorry for the noise!\n\nATB,\nRamsay Jones\n"},{"id":"370641","messageId":"20190304213733.GA3347@sigill.intra.peff.net","threadId":"50623","inReplyTo":"nycvar.QRO.7.76.6.1903041210410.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-04T21:37:33Z","receivedAt":"2019-03-04T21:37:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2019 at 01:38:13PM +0100, Johannes Schindelin wrote:\n\n> > So, tl;dr: soon, I will be submitting a patch to remove the\n> > 'hdr-check' target completely, for now anyway.\n> \n> You mentioned later that you might be the only person using that target,\n> and if that were so, I would agree.\n> \n> However, I started looking into adding `hdr-check` to the Azure Pipeline\n> we have (which would make a *ton* of sense, if you ask me), along with the\n> `sparse` thing we talked about.\n> \n> So I am a bit opposed to removing that target.\n\nYeah, I agree. If it's a linting check we expect developers to follow,\nthen we need to make it easy for developers to run it.\n\n> > Yep, for me too. There is a similar problem if you build with\n> > NO_CURL and don't have the 'curl/curl.h' header file, etc ...\n> \n> I think all it needs to do is to pass `$(ALL_CFLAGS)` to gcc (although we\n> might need to add something like `#if defined(SHA256_GCRYPT) ... #endif`\n> guards to `sha256/gcrypt.h` to make it work in Peff's case).\n> \n> But given that this target really is meant to catch all kinds of errors,\n> I'd be in favor of declaring that target requiring things like libgcrypt's\n> header files to be installed. We can easily make that happen in our CI\n> builds.\n\nI don't think that really scales, though. For CI, perhaps it's not too\nbad, but IMHO there's real value making it possible for everyday\ndevelopers to run linting and tests locally, because it keeps the\nfeedback cycle tight. In other words, imagine my changes go through a\nsequence like this:\n\n 1. I run \"make\" locally\n\n 2. I run \"make test\" locally\n\n 3. I push, and CI runs \"make && make test\" on more platforms, or with\n more knobs tweaked.\n\nit's preferable to find out about problems in the early steps, because\nthe cost to run each of the subsequent steps is much higher.\n\nSo if we're adding more linting, it would ideally be part of the build.\nThe obvious reasons not to are that it's expensive, or that it's hard\nfor the local user to set up. But I think at least a hdr-check that\nchecks _your_ platform would be valuable to run as part of step 1 or 2.\n\nI also think it points to a weakness in the current hdr-check scheme. It\nomits compat/* entirely, so we can't detect any problems there. But if\nwe are going to add those back in, then making it run in CI is not as\nsimple as \"just make sure we have all libraries installed\". It's\ninherently going to be a different set per platform (and we'd want to\nrun \"make hdr-check\" on each of the platforms where we already run \"make\ntest\").\n\n> > Anyway, for now, since I seem to be the only person using this\n> > target, I think we should just remove it while I think again.\n> > (I can put it in my config.mak, so there will be no loss for me).\n> \n> As I said above, I would rather keep it, with the `ls-files` and `:=`\n> fixup.\n> \n> If others *really* feel strongly about lazy-evaluating `LIB_H`, then I\n> would *additionally* use Peff's sub-make hack. But only if others feel\n> strongly about it, because I, for one, certainly don't.\n\nMy measurements show it's not a big expense at least on my system (even\nwith the 3x runs). So I can live with just moving it to ls-files to\nbound the filesystem access, and calling that good enough.\n\n-Peff\n"},{"id":"370642","messageId":"20190304214155.GB3347@sigill.intra.peff.net","threadId":"50623","inReplyTo":"nycvar.QRO.7.76.6.1903041206300.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-04T21:41:55Z","receivedAt":"2019-03-04T21:41:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2019 at 12:08:41PM +0100, Johannes Schindelin wrote:\n\n> -- snipsnap --\n> From c13d9985284d4b452db0d95b6949e02c533db634 Mon Sep 17 00:00:00 2001\n> From: Johannes Schindelin \n> Date: Sat, 23 Feb 2019 20:38:40 +0100\n> Subject: [PATCH] hdr-check: make it work on Windows\n> \n> On Windows, we define a specific set of pre-processor macros, among\n> other reasons: to avoid including syslog.h (which is not available on\n> Windows).\n> \n> The hdr-check target did not use those definitions, resulting in a\n> failure to include said syslog.h.\n> \n> To fix that, let's let the hdr-check target make use of ALL_CFLAGS,\n> which does include the pre-processor macros that would e.g. skip the\n> conditional block including syslog.h.\n\nThis makes sense to me, though as you noted elsewhere, it doesn't fix\nthe gcrypt problem, since that file unconditionally wants to look at the\nsystem gcrypt.h (and we control at the Makefile level whether we\nactually look at sha256/gcrypt.h).\n\n-Peff\n"},{"id":"370643","messageId":"20190304214346.GC3347@sigill.intra.peff.net","threadId":"50623","inReplyTo":"cb253bd0cf2896cf31516079a89ec2dab21032cf.1551707225.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-04T21:43:46Z","receivedAt":"2019-03-04T21:43:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2019 at 05:47:06AM -0800, Johannes Schindelin via GitGitGadget wrote:\n\n> Therefore, just like in 335339758c (Makefile: ask \"ls-files\" to list\n> source files if available, 2011-10-18), we now prefer to use `git\n> ls-files` to enumerate the header files to enumerating them via `find`,\n> falling back to the latter if the former failed (which would be the case\n> e.g. in a worktree that was extracted from a source .tar file rather\n> than from a clone of Git's sources).\n\nThanks, this version looks good to me, and seems like an obvious first\nstep regardless of whether we want to later push it into a sub-make.\n\n-Peff\n"},{"id":"370669","messageId":"xmqqzhq9vpik.fsf@gitster-ct.c.googlers.com","threadId":"50623","inReplyTo":"20190304214155.GB3347@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-05T05:50:11Z","receivedAt":"2019-03-05T05:50:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This makes sense to me, though as you noted elsewhere, it doesn't fix\n> the gcrypt problem, since that file unconditionally wants to look at the\n> system gcrypt.h (and we control at the Makefile level whether we\n> actually look at sha256/gcrypt.h).\n\nHmm, is that because the header check target does not know which *.h\nfiles we ship are actually used in a particular build?\n\nAfter a normal build, with dynamic dependency checking on, we would\nhave sufficient information to figure it out. Would that help?\n"},{"id":"370721","messageId":"nycvar.QRO.7.76.6.1903051627050.41@tvgsbejvaqbjf.bet","threadId":"50623","inReplyTo":"xmqqzhq9vpik.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-05T15:28:45Z","receivedAt":"2019-03-05T15:29:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 5 Mar 2019, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > This makes sense to me, though as you noted elsewhere, it doesn't fix\n> > the gcrypt problem, since that file unconditionally wants to look at the\n> > system gcrypt.h (and we control at the Makefile level whether we\n> > actually look at sha256/gcrypt.h).\n> \n> Hmm, is that because the header check target does not know which *.h\n> files we ship are actually used in a particular build?\n> \n> After a normal build, with dynamic dependency checking on, we would\n> have sufficient information to figure it out. Would that help?\n\nYes, *if* the dynamic dependency checking is in effect (read: if we are\ncompiling with GCC).\n\nHowever, I think that one of the benefits of the current approach is that\nhdr-check finds issues also in headers that are *not* part of the current\nbuild. Read: once hdr-check is run as part of the CI build, it is harder\nfor some random contributor to break hdr-check for somebody else with\ndifferent compile time options.\n\nCiao,\nDscho\n"},{"id":"370751","messageId":"xmqqlg1tt0uf.fsf@gitster-ct.c.googlers.com","threadId":"50623","inReplyTo":"nycvar.QRO.7.76.6.1903051627050.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-05T22:26:00Z","receivedAt":"2019-03-05T22:26:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> After a normal build, with dynamic dependency checking on, we would\n>> have sufficient information to figure it out. Would that help?\n>\n> Yes, *if* the dynamic dependency checking is in effect (read: if we are\n> compiling with GCC).\n>\n> However, I think that one of the benefits of the current approach is that\n> hdr-check finds issues also in headers that are *not* part of the current\n> build. Read: once hdr-check is run as part of the CI build, it is harder\n> for some random contributor to break hdr-check for somebody else with\n> different compile time options.\n\nSure. That is exactly why we have multiple configurations at the\nCI, hopefully more than each individual contributor has at hand so\nthat CI can give more coverage than any single individual can test.\n\nWhat I was assuming was that at least one of these multiple\nconfigurations would cover the case where sha256/gcrypt.h truly is\nused, because that particular build is configured to use the system\nheader and library. And it would be a far better approach to make\nthat build check also for the header, if we can cover all the\nproblematic (in the same sense as the sha256/gcrypt.h) header files,\nthan unconditionally \"checking\" without knowing the prerequisite and\ncausing a false positive.\n\nHaving said that, the optional use of ls-files introduced by the\npatch in question is a valid incremental improvement over the\ncurrent Makefile, so it is a pretty much independent/orthogonal\nissue.\n"},{"id":"370761","messageId":"20190305230723.GB22901@sigill.intra.peff.net","threadId":"50623","inReplyTo":"xmqqzhq9vpik.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-05T23:07:23Z","receivedAt":"2019-03-05T23:07:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2019 at 02:50:11PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > This makes sense to me, though as you noted elsewhere, it doesn't fix\n> > the gcrypt problem, since that file unconditionally wants to look at the\n> > system gcrypt.h (and we control at the Makefile level whether we\n> > actually look at sha256/gcrypt.h).\n> \n> Hmm, is that because the header check target does not know which *.h\n> files we ship are actually used in a particular build?\n\nYes, exactly.\n\n> After a normal build, with dynamic dependency checking on, we would\n> have sufficient information to figure it out. Would that help?\n\nYeah, that's what I was hinting at earlier in the thread. Here it is\nsketched out to an actual working patch. The sub-make bits could\nactually be a shell script instead of a Makefile; the only point in\nusing make is to use the parent \"-j\" for parallelism.\n\nNote also that by including compat/ headers, I noticed that bswap.h\nfails its hdr-check. :)\n\nSo do command-list.h (it needs gettext.h to handle the _() markings, but\nthat only comes in via cache.h), and all of xdiff (which is probably\nfixable, but I didn't bother here).\n\n---\n Makefile | 23 ++++++++++++-----------\n compat/bswap.h | 5 +++++\n 2 files changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex c5240942f2..283e934d7b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1852,7 +1852,6 @@ 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@@ -2735,16 +2734,18 @@ $(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+.PHONY: hdr-check\n+hdr-check: all\n+ifdef USE_COMPUTED_HEADER_DEPENDENCIES\n+\t@$(MAKE) -f hdr-check.mak CC=\"$(CC)\" V=$(V) \\\n+\t\t$$(sed -n 's/^\\([^ ]*\\)\\.h:/\\1.hco/p' .depend/* | \\\n+\t\t sort -u | \\\n+\t\t egrep -v '^(xdiff/|unicode-width.h|command-list.h)' \\\n+\t\t)\n+else\n+\t@echo >&2 \"error: hdr-check supported only on platforms with computed dependencies\"\n+\t@false\n+endif\n \n .PHONY: style\n style:\ndiff --git a/compat/bswap.h b/compat/bswap.h\nindex 5078ce5ecc..e4e25735ce 100644\n--- a/compat/bswap.h\n+++ b/compat/bswap.h\n@@ -1,3 +1,6 @@\n+#ifndef COMPAT_BSWAP_H\n+#define COMPAT_BSWAP_H\n+\n /*\n * Let's make sure we always have a sane definition for ntohl()/htonl().\n * Some libraries define those as a function call, just to perform byte\n@@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value)\n }\n \n #endif\n+\n+#endif /* COMPAT_BSWAP_H */\n-- \n2.21.0.684.gc9dc8b89c9\n\n"},{"id":"370775","messageId":"42d125d4-76bf-afc3-8f12-a9fa1296c85c@ramsayjones.plus.com","threadId":"50623","inReplyTo":"20190305230723.GB22901@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-06T00:23:20Z","receivedAt":"2019-03-06T00:23:24Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/03/2019 23:07, Jeff King wrote:\n> On Tue, Mar 05, 2019 at 02:50:11PM +0900, Junio C Hamano wrote:\n> \n>> Jeff King writes:\n>>\n>>> This makes sense to me, though as you noted elsewhere, it doesn't fix\n>>> the gcrypt problem, since that file unconditionally wants to look at the\n>>> system gcrypt.h (and we control at the Makefile level whether we\n>>> actually look at sha256/gcrypt.h).\n>>\n>> Hmm, is that because the header check target does not know which *.h\n>> files we ship are actually used in a particular build?\n> \n> Yes, exactly.\n> \n>> After a normal build, with dynamic dependency checking on, we would\n>> have sufficient information to figure it out. Would that help?\n> \n> Yeah, that's what I was hinting at earlier in the thread. Here it is\n> sketched out to an actual working patch. The sub-make bits could\n> actually be a shell script instead of a Makefile; the only point in\n> using make is to use the parent \"-j\" for parallelism.\n\nsigh. :(\n\nI wish my patch removing this target had been picked up now!\n\nEarlier this evening I spent an hour or so writing an email which\ntried to discourage spending any time on this, because of the\npotential for this to be a huge time-suck. An unrewarding one at\nthat! :-D\n\nThe email was actually prompted by someone mentioning 'dynamic\ncompiler dependencies', because that's how it always starts ...\n\nI deleted that email (it's not in my drafts folder anyway)\nbecause, in the end, it is not up to me to say how people should\nspend their time.\n\nSo I won't! :-D\n\nATB,\nRamsay Jones\n"},{"id":"370779","messageId":"20190306044006.GA6664@sigill.intra.peff.net","threadId":"50623","inReplyTo":"42d125d4-76bf-afc3-8f12-a9fa1296c85c@ramsayjones.plus.com","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-06T04:40:06Z","receivedAt":"2019-03-06T04:40:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 06, 2019 at 12:23:20AM +0000, Ramsay Jones wrote:\n\n> > Yeah, that's what I was hinting at earlier in the thread. Here it is\n> > sketched out to an actual working patch. The sub-make bits could\n> > actually be a shell script instead of a Makefile; the only point in\n> > using make is to use the parent \"-j\" for parallelism.\n> \n> sigh. :(\n> \n> I wish my patch removing this target had been picked up now!\n> \n> Earlier this evening I spent an hour or so writing an email which\n> tried to discourage spending any time on this, because of the\n> potential for this to be a huge time-suck. An unrewarding one at\n> that! :-D\n\nHeh. I am OK with removing it, too.\n\nMy thinking earlier in the thread was that it should go in our bag of\nlinting tools that people should generally run. But actually, it is kind\nof expensive to run, and it does not actually help anything immediately\nin practice. I.e., what we really care about is that the C source files\ncompile, and running \"make\" does that (and especially running it on\nvarious platforms). This is just checking for a _potential_ problem if\nsomebody were to include a particular header file at the start of\nanother C file.\n\nSo it's really about 2 steps removed from stopping an actual problem.\n\n> I deleted that email (it's not in my drafts folder anyway)\n> because, in the end, it is not up to me to say how people should\n> spend their time.\n\nFWIW, it was a fun exercise to build on the compiler dependencies. ;)\nAnd I think I'm done playing with it for now. \"make hdr-check\" does not\nrun for me, but I think I've convinced myself that it's not all that\nimportant that I run it.\n\nI did forget to include one file in my earlier patch (the newly added\nhdr-check.mak). I imagine one could guess at its contents, but here is\nthe complete patch, for the sake of reference.\n\n-Peff\n\n---\n Makefile | 23 ++++++++++++-----------\n compat/bswap.h | 5 +++++\n hdr-check.mak | 8 ++++++++\n 3 files changed, 25 insertions(+), 11 deletions(-)\n create mode 100644 hdr-check.mak\n\ndiff --git a/Makefile b/Makefile\nindex c5240942f2..283e934d7b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1852,7 +1852,6 @@ 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@@ -2735,16 +2734,18 @@ $(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+.PHONY: hdr-check\n+hdr-check: all\n+ifdef USE_COMPUTED_HEADER_DEPENDENCIES\n+\t@$(MAKE) -f hdr-check.mak CC=\"$(CC)\" V=$(V) \\\n+\t\t$$(sed -n 's/^\\([^ ]*\\)\\.h:/\\1.hco/p' .depend/* | \\\n+\t\t sort -u | \\\n+\t\t egrep -v '^(xdiff/|unicode-width.h|command-list.h)' \\\n+\t\t)\n+else\n+\t@echo >&2 \"error: hdr-check supported only on platforms with computed dependencies\"\n+\t@false\n+endif\n \n .PHONY: style\n style:\ndiff --git a/compat/bswap.h b/compat/bswap.h\nindex 5078ce5ecc..e4e25735ce 100644\n--- a/compat/bswap.h\n+++ b/compat/bswap.h\n@@ -1,3 +1,6 @@\n+#ifndef COMPAT_BSWAP_H\n+#define COMPAT_BSWAP_H\n+\n /*\n * Let's make sure we always have a sane definition for ntohl()/htonl().\n * Some libraries define those as a function call, just to perform byte\n@@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value)\n }\n \n #endif\n+\n+#endif /* COMPAT_BSWAP_H */\ndiff --git a/hdr-check.mak b/hdr-check.mak\nnew file mode 100644\nindex 0000000000..00a3110fda\n--- /dev/null\n+++ b/hdr-check.mak\n@@ -0,0 +1,8 @@\n+.PHONY: FORCE\n+\n+ifndef V\n+QUIET_HDR = @echo ' ' HDR $<;\n+endif\n+\n+%.hco: %.h FORCE\n+\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n-- \n2.21.0.699.ge1748d4d73\n\n"},{"id":"370785","messageId":"xmqqd0n4r2qm.fsf@gitster-ct.c.googlers.com","threadId":"50623","inReplyTo":"20190306044006.GA6664@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-06T05:28:01Z","receivedAt":"2019-03-06T05:28:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 06, 2019 at 12:23:20AM +0000, Ramsay Jones wrote:\n>\n>> > Yeah, that's what I was hinting at earlier in the thread. Here it is\n>> > sketched out to an actual working patch. The sub-make bits could\n>> > actually be a shell script instead of a Makefile; the only point in\n>> > using make is to use the parent \"-j\" for parallelism.\n>> \n>> sigh. :(\n>> \n>> I wish my patch removing this target had been picked up now!\n>> \n>> Earlier this evening I spent an hour or so writing an email which\n>> tried to discourage spending any time on this, because of the\n>> potential for this to be a huge time-suck. An unrewarding one at\n>> that! :-D\n>\n> Heh. I am OK with removing it, too.\n\nFWIW, I am fine with that plan as well.\n\n> My thinking earlier in the thread was that it should go in our bag of\n> linting tools that people should generally run. But actually, it is kind\n> of expensive to run, and it does not actually help anything immediately\n> in practice. I.e., what we really care about is that the C source files\n> compile, and running \"make\" does that (and especially running it on\n> various platforms). This is just checking for a _potential_ problem if\n> somebody were to include a particular header file at the start of\n> another C file.\n>\n> So it's really about 2 steps removed from stopping an actual problem.\n\nYeah, exactly.\n\n> diff --git a/compat/bswap.h b/compat/bswap.h\n> index 5078ce5ecc..e4e25735ce 100644\n> --- a/compat/bswap.h\n> +++ b/compat/bswap.h\n> @@ -1,3 +1,6 @@\n> +#ifndef COMPAT_BSWAP_H\n> +#define COMPAT_BSWAP_H\n> +\n> /*\n> * Let's make sure we always have a sane definition for ntohl()/htonl().\n> * Some libraries define those as a function call, just to perform byte\n> @@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value)\n> }\n> \n> #endif\n> +\n> +#endif /* COMPAT_BSWAP_H */\n\nThis probably is worth having as an independent clean-up.\n"},{"id":"370819","messageId":"20190306190509.GA18239@sigill.intra.peff.net","threadId":"50623","inReplyTo":"xmqqd0n4r2qm.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] compat/bswap: add include header guards","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-06T19:05:10Z","receivedAt":"2019-03-06T19:05:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 06, 2019 at 02:28:01PM +0900, Junio C Hamano wrote:\n\n> > +#ifndef COMPAT_BSWAP_H\n> > +#define COMPAT_BSWAP_H\n> [...]\n> \n> This probably is worth having as an independent clean-up.\n\nYeah, let's do that now before we forget.\n\n-- >8 --\nSubject: [PATCH] compat/bswap: add include header guards\n\nOur compat/bswap.h lacks the usual preprocessor guards against multiple\ninclusion. This usually isn't an issue since it only gets included from\ngit-compat-util.h, which has its own guards. But it would produce\nredeclaration errors if any file included it separately.\n\nOur hdr-check target would complain about this, except that it currently\nskips items in compat/ entirely.\n\nSigned-off-by: Jeff King \n---\n compat/bswap.h | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/compat/bswap.h b/compat/bswap.h\nindex 5078ce5ecc..e4e25735ce 100644\n--- a/compat/bswap.h\n+++ b/compat/bswap.h\n@@ -1,3 +1,6 @@\n+#ifndef COMPAT_BSWAP_H\n+#define COMPAT_BSWAP_H\n+\n /*\n * Let's make sure we always have a sane definition for ntohl()/htonl().\n * Some libraries define those as a function call, just to perform byte\n@@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value)\n }\n \n #endif\n+\n+#endif /* COMPAT_BSWAP_H */\n-- \n2.21.0.699.ge1748d4d73\n\n"},{"id":"370835","messageId":"xmqq8sxrr5e9.fsf@gitster-ct.c.googlers.com","threadId":"50623","inReplyTo":"20190306190509.GA18239@sigill.intra.peff.net","subject":"Re: [PATCH] compat/bswap: add include header guards","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-06T22:42:54Z","receivedAt":"2019-03-06T22:43:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 06, 2019 at 02:28:01PM +0900, Junio C Hamano wrote:\n>\n>> > +#ifndef COMPAT_BSWAP_H\n>> > +#define COMPAT_BSWAP_H\n>> [...]\n>> \n>> This probably is worth having as an independent clean-up.\n>\n> Yeah, let's do that now before we forget.\n\nThanks\n\n>\n> -- >8 --\n> Subject: [PATCH] compat/bswap: add include header guards\n> ...\n"}]}