Avoid calling find in the Makefile, if possible
cover Avoid calling find in the Makefile, if possible
I noticed this quite a bit of time ago, but did not get a chance to look into it in detail: all of a sudden, make started really slowly over here.
The culprit turned out to be a find call, which was in the Makefile for ages, so I was puzzled why it only caused problems recently.
After some digging, I found out that the hdr-check target is the culprit:
before it was introduced, $(LIB_H) did not need to be expanded, and as a consequence the find call was not executed. Once hdr-check made it into master, though, $(LIB_H) must be expanded to define that rule.
Since I have tons of worktrees as subdirectories of my principal Git clone, and since I also have a 3rdparty/ directory with tons of repositories I use for various testing/contributing purposes, this find is quite a little slow over here.
So here is my suggested fix. It is based on similar code we already had in the Makefile, obviously also intended to avoid an expensive find invocation.
Johannes Schindelin (1):
Makefile: use git ls-files to list header files, if possible
Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
base-commit: c65a2884eae159bad540135479bc8afe20ff62d1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-130%2Fdscho%2Favoid-find-in-makefile-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-130/dscho/avoid-find-in-makefile-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/130
Signature · 1 line
gitgitgadget
PATCH 1/1 Makefile: use `git ls-files` to list header files, if possible
From: Johannes Schindelin <[email protected]>
In d85b0dff72 (Makefile: use find to determine static header dependencies, 2014-08-25), we switched from a static list of header files to a dynamically-generated one, asking find to enumerate them.
Back in those days, we did not use $(LIB_H) by default, and many a make implementation seems smart enough not to run that find command in that case, so it was deemed okay to run find for special targets requiring this macro.
However, as of ebb7baf02f (Makefile: add a hdr-check target, 2018-09-19), $(LIB_H) is part of a global rule and therefore must be expanded. Meaning: this find command has to be run upon every make invocation. In the presence of many a worktree, this can tax the developers' patience quite a bit.
Even in the absence of worktrees or other untracked files and directories, the cost of I/O to generate that list of header files is simply a lot larger than a simple git ls-files call.
Therefore, just like in 335339758c (Makefile: ask "ls-files" to list source files if available, 2011-10-18), we now prefer to use git ls-files to enumerate the header files to enumerating them via find, falling back to the latter if the former failed (which would be the case e.g. in a worktree that was extracted from a source .tar file rather than from a clone of Git's sources).
This has one notable consequence: we no longer include command-list.h in LIB_H, as it is a generated file, not a tracked one.
Likewise, we no longer include not-yet-tracked header files in LIB_H.
Given the speed improvements, these consequences seem a comparably small price to pay.
--- Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Makefile +3 -2
@@ -841,7 +841,8 @@ VCSSVN_LIB = vcs-svn/lib.a GENERATED_H += command-list.h -LIB_H = $(shell $(FIND) . \ +LIB_H = $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \ + $(FIND) . \ -name .git -prune -o \ -name t -prune -o \ -name Documentation -prune -o \
@@ -2363,7 +2364,7 @@ else # should _not_ be included here, since they are necessary even when # building an object for the first time. -$(OBJECTS): $(LIB_H) +$(OBJECTS): $(LIB_H) $(GENERATED_H) endif exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX
Signature · 1 line
gitgitgadget
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 9 lines · Johannes Schindelin via GitGitGadget
In
d85b0dff72(Makefile: usefindto determine static header dependencies, 2014-08-25), we switched from a static list of header files to a dynamically-generated one, askingfindto enumerate them.Back in those days, we did not use
$(LIB_H)by default, and many amakeimplementation seems smart enough not to run thatfindcommand in that case, so it was deemed okay to runfindfor special targets requiring this macro.
Quoted 5 lines
However, as of
ebb7baf02f(Makefile: add a hdr-check target, 2018-09-19), $(LIB_H) is part of a global rule and therefore must be expanded. Meaning: thisfindcommand has to be run upon everymakeinvocation. In the presence of many a worktree, this can tax the developers' patience quite a bit.
I'm confused about this part. We don't run hdr-check by default. Why would make need the value of $(LIB_H)? Yet empirically it does run find.
Worse, it seems to actually run it _three times_. Once for the $(HCO) target, once for the .PHONY, and once for the hdr-check target. I think the .PHONY one is unavoidable (it doesn't know which names we might otherwise be building should be marked), but the other two seem like bugs in make (or at least pessimisations).
It makes me wonder if we'd be better off pushing hdr-check into a separate script. It doesn't actually use make's dependency tree in any meaningful way. And then regular invocations wouldn't even have to pay the ls-files price.
If we are going to keep it in the Makefile, we should probably use a ":=" rule to avoid running it three times.
Quoted 10 lines
Even in the absence of worktrees or other untracked files and directories, the cost of I/O to generate that list of header files is simply a lot larger than a simple
git ls-filescall.Therefore, just like in
335339758c(Makefile: ask "ls-files" to list source files if available, 2011-10-18), we now prefer to usegit ls-filesto enumerate the header files to enumerating them viafind, falling back to the latter if the former failed (which would be the case e.g. in a worktree that was extracted from a source .tar file rather than from a clone of Git's sources).
That seems reasonable (regardless of whether it is in a script or in the Makefile). Another option is to use -maxdepth, but that involves guessing how deep people might actually put header files.
Quoted 2 lines
This has one notable consequence: we no longer include
command-list.hinLIB_H, as it is a generated file, not a tracked one.
We should be able to add back $(GENERATED_H) as appropriate. I see you did it for the non-computed-dependencies case. Couldn't we do the same for $(LOCALIZED_C) and $(CHK_HDRS)?
Quoted 1 line
Likewise, we no longer include not-yet-tracked header files in
LIB_H.
I think that's probably OK.
-Peff
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 15 lines · Jeff King
Quoted 5 lines
However, as of
ebb7baf02f(Makefile: add a hdr-check target, 2018-09-19), $(LIB_H) is part of a global rule and therefore must be expanded. Meaning: thisfindcommand has to be run upon everymakeinvocation. In the presence of many a worktree, this can tax the developers' patience quite a bit.I'm confused about this part. We don't run hdr-check by default. Why would make need the value of $(LIB_H)? Yet empirically it does run find.
Worse, it seems to actually run it
_threetimes_. Once for the $(HCO) target, once for the .PHONY, and once for the hdr-check target. I think the .PHONY one is unavoidable (it doesn't know which names we might otherwise be building should be marked), but the other two seem like bugs in make (or at least pessimisations).
Quoted 7 lines
It makes me wonder if we'd be better off pushing hdr-check into a separate script. It doesn't actually use make's dependency tree in any meaningful way. And then regular invocations wouldn't even have to pay the
ls-filesprice.If we are going to keep it in the
Makefile, we should probably use a ":=" rule to avoid running it three times.
Looping in Ramsay as the author of hdr-check. I think we could even do this without using a separate script, like so:
Makefile +7 -6
@@ -1852,7 +1852,6 @@ ifndef V QUIET_MSGFMT = @echo ' ' MSGFMT $@; QUIET_GCOV = @echo ' ' GCOV $@; QUIET_SP = @echo ' ' SP $<; - QUIET_HDR = @echo ' ' HDR $<; QUIET_RC = @echo ' ' RC $@; QUIET_SUBDIR0 = +@subdir= QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \
@@ -2740,11 +2739,13 @@ EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) -$(HCO): %.hco: %.h FORCE - $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< - -.PHONY: hdr-check $(HCO) -hdr-check: $(HCO) +.PHONY: hdr-check +hdr-check: + @for i in $(LIB_H); do \ + echo " HDR $$i"; \ + $(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $$i || \ + exit 1; \ + done .PHONY: style style:
The one thing we do lose, though, is make's parallelization. It would probably be possible to actually shove this into a sub-make which defined the hdr-check rules, but I don't know how complicated that would become.
Quoted 6 lines
Quoted 2 lines
This has one notable consequence: we no longer include
command-list.hinLIB_H, as it is a generated file, not a tracked one.We should be able to add back $(GENERATED_H) as appropriate. I see you did it for the non-computed-dependencies case. Couldn't we do the same for $(LOCALIZED_C) and $(CHK_HDRS)?
Answering my own question somewhat: $(CHK_HDRS) explicitly ignores the generated headers, so we wouldn't want to bother re-adding it there anyway. Possibly $(LOCALIZED_C) would care, though. The generated file does have translatable strings in it.
-Peff
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 4 lines · Jeff King
The one thing we do lose, though, is make's parallelization. It would probably be possible to actually shove this into a sub-make which defined the hdr-check rules, but I don't know how complicated that would become.
This seems to work, though it's kind of horrid.
It costs at least one extra process to run "make hdr-check", and probably more for things like $(GIT_VERSION) that the Makefile include likely triggers. But when you're not running hdr-check (which is the norm), it's zero-cost.
-Peff
Makefile +2 -10
@@ -2735,16 +2735,8 @@ $(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% -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) -HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) - -$(HCO): %.hco: %.h FORCE - $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< - -.PHONY: hdr-check $(HCO) -hdr-check: $(HCO) +hdr-check: + $(MAKE) -f hdr-check.mak hdr-check .PHONY: style style:
hdr-check.mak new file +12 -0
@@ -0,0 +1,12 @@ +include Makefile + +GEN_HDRS := command-list.h unicode-width.h +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) + +$(HCO): %.hco: %.h FORCE + $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< + +.PHONY: hdr-check $(HCO) +hdr-check: $(HCO)
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Hi Peff,
Quoted 7 lines · Jeff King
Quoted 4 lines · Jeff King
The one thing we do lose, though, is make's parallelization. It would probably be possible to actually shove this into a sub-make which defined the hdr-check rules, but I don't know how complicated that would become.
Quoted 6 lines
This seems to work, though it's kind of horrid.
It costs at least one extra process to run "make hdr-check", and probably more for things like $(GIT_VERSION) that the
Makefileinclude likely triggers. But when you're not running hdr-check (which is the norm), it's zero-cost.
If we want to go that route (and I am not saying we should), we could easily just add another target (say, check-headers) that requires a list of headers to check to be passed in via a Makefile variable that is defined via the command-line.
Ciao,
Dscho
Quoted 45 lines
(quoted diff) +0
-1-PeffMakefile +2
-10@@ -2735,16 +2735,8 @@ $(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% -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) -HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) - -$(HCO): %.hco: %.h FORCE - $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< - -.PHONY: hdr-check $(HCO) -hdr-check: $(HCO) +hdr-check: + $(MAKE) -f hdr-check.mak hdr-check .PHONY: style style:hdr-check.mak new file +12
-0@@ -0,0 +1,12 @@ +include Makefile + +GEN_HDRS := command-list.h unicode-width.h +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) + +$(HCO): %.hco: %.h FORCE + $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< + +.PHONY: hdr-check $(HCO) +hdr-check: $(HCO)
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 16 lines · Johannes Schindelin
Quoted 7 lines · Jeff King
Quoted 4 lines · Jeff King
The one thing we do lose, though, is make's parallelization. It would probably be possible to actually shove this into a sub-make which defined the hdr-check rules, but I don't know how complicated that would become.
Quoted 6 lines
This seems to work, though it's kind of horrid.
It costs at least one extra process to run "make hdr-check", and probably more for things like $(GIT_VERSION) that the
Makefileinclude likely triggers. But when you're not running hdr-check (which is the norm), it's zero-cost.
Quoted 4 lines
If we want to go that route (and I am not saying we should), we could easily just add another target (say,
check-headers) that requires a list of headers to check to be passed in via aMakefilevariable that is defined via the command-line.
Yeah, that was actually going to be my initial attempt before I tried the "include" shortcut. :) I'd worry slightly about things like command-line limits, but maybe our list isn't big enough to justify that.
-Peff
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
In reply to Jeff KingHi Peff,
Quoted 6 lines · Jeff King
Quoted 4 lines · Jeff King
Quoted 3 lines
This has one notable consequence: we no longer include
command-list.hinLIB_H, as it is a generated file, not a tracked one.
Quoted 8 lines
Quoted 3 lines
We should be able to add back $(GENERATED_H) as appropriate. I see you did it for the non-computed-dependencies case. Couldn't we do the same for $(LOCALIZED_C) and $(CHK_HDRS)?
Answering my own question somewhat: $(CHK_HDRS) explicitly ignores the generated headers, so we wouldn't want to bother re-adding it there anyway. Possibly $(LOCALIZED_C) would care, though. The generated file does have translatable strings in it.
Yes, and it is defined thusly:
LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
So it does include the generated headers explictly anyway, with or without my patch.
Ciao,
Dscho
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 19 lines · Johannes Schindelin
Hi Peff,
Quoted 6 lines · Jeff King
Quoted 4 lines · Jeff King
Quoted 3 lines
This has one notable consequence: we no longer include
command-list.hinLIB_H, as it is a generated file, not a tracked one.Quoted 8 lines
Quoted 3 lines
We should be able to add back $(GENERATED_H) as appropriate. I see you did it for the non-computed-dependencies case. Couldn't we do the same for $(LOCALIZED_C) and $(CHK_HDRS)?
Answering my own question somewhat: $(CHK_HDRS) explicitly ignores the generated headers, so we wouldn't want to bother re-adding it there anyway. Possibly $(LOCALIZED_C) would care, though. The generated file does have translatable strings in it.
Quoted 6 lines
Yes, and it is defined thusly:
LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)So it does include the generated headers explictly anyway, with or without my patch.
Oops, I should have read the source more carefully before responding.
Is it worth mentioning in the commit message? I.e., something like:
This has one notable consequence: we no longer include command-list.h in LIB_H, as it is a generated file, not a tracked one, but that is easily worked around. Of the three sites that use LIB_H, two (LOCALIZED_C and CHK_HDRS) already handle generated headers separately. In the third, the computed-dependency fallback, we can just add in a reference to $(GENERATED_H).
-Peff
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
In reply to Jeff KingHi Peff,
Quoted 26 lines · Jeff King
Quoted 9 lines · Johannes Schindelin via GitGitGadget
In
d85b0dff72(Makefile: usefindto determine static header dependencies, 2014-08-25), we switched from a static list of header files to a dynamically-generated one, askingfindto enumerate them.Back in those days, we did not use
$(LIB_H)by default, and many amakeimplementation seems smart enough not to run thatfindcommand in that case, so it was deemed okay to runfindfor special targets requiring this macro.Quoted 5 lines
However, as of
ebb7baf02f(Makefile: add a hdr-check target, 2018-09-19), $(LIB_H) is part of a global rule and therefore must be expanded. Meaning: thisfindcommand has to be run upon everymakeinvocation. In the presence of many a worktree, this can tax the developers' patience quite a bit.I'm confused about this part. We don't run hdr-check by default. Why would make need the value of $(LIB_H)? Yet empirically it does run find.
Worse, it seems to actually run it
_threetimes_. Once for the $(HCO) target, once for the .PHONY, and once for the hdr-check target. I think the .PHONY one is unavoidable (it doesn't know which names we might otherwise be building should be marked), but the other two seem like bugs in make (or at least pessimisations).
Quoted 7 lines
It makes me wonder if we'd be better off pushing hdr-check into a separate script. It doesn't actually use make's dependency tree in any meaningful way. And then regular invocations wouldn't even have to pay the
ls-filesprice.If we are going to keep it in the
Makefile, we should probably use a ":=" rule to avoid running it three times.
Good point.
Quoted 11 lines
Quoted 10 lines
Even in the absence of worktrees or other untracked files and directories, the cost of I/O to generate that list of header files is simply a lot larger than a simple
git ls-filescall.Therefore, just like in
335339758c(Makefile: ask "ls-files" to list source files if available, 2011-10-18), we now prefer to usegit ls-filesto enumerate the header files to enumerating them viafind, falling back to the latter if the former failed (which would be the case e.g. in a worktree that was extracted from a source .tar file rather than from a clone of Git's sources).
Quoted 3 lines
That seems reasonable (regardless of whether it is in a script or in the
Makefile). Another option is to use -maxdepth, but that involves guessing how deep people might actually put header files.
It would also fail to work when somebody clones an unrelated repository that contains header files in the top-level directory into the Git worktree. I know somebody like that: me.
Quoted 6 lines
Quoted 2 lines
This has one notable consequence: we no longer include
command-list.hinLIB_H, as it is a generated file, not a tracked one.We should be able to add back $(GENERATED_H) as appropriate. I see you did it for the non-computed-dependencies case. Couldn't we do the same for $(LOCALIZED_C) and $(CHK_HDRS)?
As you figured out, CHK_HDRS *specifically* excludes the generated headers, and as I pointed out: LOCALIZED_C includes $(GENERATED_H) explicitly.
So I think we're good on that front.
Quoted 3 lines
Quoted 1 line
Likewise, we no longer include not-yet-tracked header files in
LIB_H.I think that's probably OK.
It does potentially make developing new patches more challenging. I, for one, do not immediately stage every new file I add, especially not header files. But then, I rarely recompile after only editing such a new header file (I would more likely modify also the source file that includes that header).
So while I think this issue could potentially cause problems only *very* rarely, I think that it would be a really terribly confusing thing if it happened.
But I probably worry too much about it?
Ciao,
Dscho
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 7 lines · Johannes Schindelin
Quoted 3 lines
That seems reasonable (regardless of whether it is in a script or in the
Makefile). Another option is to use -maxdepth, but that involves guessing how deep people might actually put header files.It would also fail to work when somebody clones an unrelated repository that contains header files in the top-level directory into the Git worktree. I know somebody like that: me.
Good point.
By the way, "make hdr-check" already fails for me on master, as I do not have libgcrypt installed, and it unconditionally checks sha256/gcrypt.h.
I wonder if this actually does point to hdr-check needing to be smarter about checking the headers that are actually used in compilation on your platform. Or if that file should just be added to the set of excluded headers.
Quoted 9 lines
Quoted 3 lines
We should be able to add back $(GENERATED_H) as appropriate. I see you did it for the non-computed-dependencies case. Couldn't we do the same for $(LOCALIZED_C) and $(CHK_HDRS)?
As you figured out,
CHK_HDRS*specifically* excludes the generated headers, and as I pointed out:LOCALIZED_Cincludes $(GENERATED_H) explicitly.So I think we're good on that front.
Yeah, agreed.
Quoted 10 lines
Quoted 3 lines
Quoted 1 line
Likewise, we no longer include not-yet-tracked header files in
LIB_H.I think that's probably OK.
It does potentially make developing new patches more challenging. I, for one, do not immediately stage every new file I add, especially not header files. But then, I rarely recompile after only editing such a new header file (I would more likely modify also the source file that includes that header).
Quoted 5 lines
So while I think this issue could potentially cause problems only *very* rarely, I think that it would be a really terribly confusing thing if it happened.
But I probably worry too much about it?
I think it's not ideal, but it's probably an acceptable tradeoff. The LIB_H list is used for three things:
- hdr-check, which I'd think would generally be run periodically on a full tree to catch any new header breakages. But I dunno, maybe people want to run it as soon as they've written new code.
- the .po generation, which generally is a separate workflow from writing new header files
- the header-dependency fallback code. This is definitely the place where somebody just adding a new header file and running "make" might get bitten. But it only kicks in for ancient, crappy compilers that don't do dependency computation; so I think most developers would not be using it. (this is your cue to explain to me how some workflow involving MSVC does not compute dependencies, and I'm unknowingly dismissing a large portion developers ;) ).
-Peff
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 11 lines · Jeff King
Quoted 7 lines · Johannes Schindelin
Quoted 3 lines
That seems reasonable (regardless of whether it is in a script or in the
Makefile). Another option is to use -maxdepth, but that involves guessing how deep people might actually put header files.It would also fail to work when somebody clones an unrelated repository that contains header files in the top-level directory into the Git worktree. I know somebody like that: me.
Good point.
[Sorry for the late reply - I have been AWOL this weekend and I am only just catching up with email! :-D ]
So, tl;dr: soon, I will be submitting a patch to remove the 'hdr-check' target completely, for now anyway.
Quoted 2 lines
By the way, "make hdr-check" already fails for me on master, as I do not have libgcrypt installed, and it unconditionally checks
sha256/gcrypt.h.
Yep, for me too. There is a similar problem if you build with NO_CURL and don't have the 'curl/curl.h' header file, etc ...
The first version of the 'hdr-check' target re-introduced a static list of header files, but I didn't think people would appreciate having to maintain the list manually, so I gave up on that!
The next version was essentially the same patch that started this thread from Johannes (ie. the 'git ls-files' patch), given that the 'tags' targets had found it necessary. However, when I did some 'informal' timing tests (ie 5x 'time make ...' and average), this did not make any noticeable difference for me (_even_ on cygwin!). ;-)
Of course, I had already made the mistake of trying to re-use something that was 'handy' (ie. LIB_H) and already there. However, LIB_H wasn't quite what I wanted - I needed a sub-set of it.
So, I already have a 'hdr-check-fixup' branch (I think I have already mentioned it), in which the first commit looks like:
diff --git a/Makefile b/Makefile index c5240942f2..3401d1b9fb 100644 --- a/Makefile +++ b/Makefile @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE .PHONY: sparse $(SP_OBJ) sparse: $(SP_OBJ) +HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \ + sha1dc/*.h refs/*.h vcs-svn/*.h) GEN_HDRS := command-list.h unicode-width.h -EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS)) HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
$(HCO): %.hco: %.h FORCE
... which drops the use of LIB_H entirely.
Now, I have timed this and, for me, it no faster ... (I suspect that it would be faster for Johannes, but it would still cause a problem if you have 'top-level header files from some other repo ...').
So, if we really need to solve the problem, allowing for some unrelated headers in your worktree, then we can only use a static list, or a 'git ls-files' approach.
Anyway, for now, since I seem to be the only person using this target, I think we should just remove it while I think again. (I can put it in my config.mak, so there will be no loss for me).
ATB,
Ramsay Jones
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Hi Ramsay,
Quoted 7 lines · Ramsay Jones
Quoted 6 lines · Jeff King
Quoted 4 lines · Johannes Schindelin
Quoted 3 lines
That seems reasonable (regardless of whether it is in a script or in the
Makefile). Another option is to use -maxdepth, but that involves guessing how deep people might actually put header files.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
It would also fail to work when somebody clones an unrelated repository that contains header files in the top-level directory into the Git worktree. I know somebody like that: me.
Good point.
[Sorry for the late reply - I have been AWOL this weekend and I am only just catching up with email! :-D ]
You should never feel the need to apologize for spending a weekend away from the keyboard (AKA "having a life").
Quoted 2 lines
So, tl;dr: soon, I will be submitting a patch to remove the 'hdr-check' target completely, for now anyway.
You mentioned later that you might be the only person using that target, and if that were so, I would agree.
However, I started looking into adding hdr-check to the Azure Pipeline we have (which would make a *ton* of sense, if you ask me), along with the sparse thing we talked about.
So I am a bit opposed to removing that target.
Quoted 6 lines
Quoted 3 lines
By the way, "make hdr-check" already fails for me on master, as I do not have libgcrypt installed, and it unconditionally checks
sha256/gcrypt.h.Yep, for me too. There is a similar problem if you build with
NO_CURLand don't have the 'curl/curl.h' header file, etc ...
I think all it needs to do is to pass $(ALL_CFLAGS) to gcc (although we might need to add something like #if defined(SHA256_GCRYPT) ... #endif guards to sha256/gcrypt.h to make it work in Peff's case).
But given that this target really is meant to catch all kinds of errors, I'd be in favor of declaring that target requiring things like libgcrypt's header files to be installed. We can easily make that happen in our CI builds.
Quoted 9 lines
The first version of the 'hdr-check' target re-introduced a static list of header files, but I didn't think people would appreciate having to maintain the list manually, so I gave up on that!
The next version was essentially the same patch that started this thread from Johannes (ie. the 'git ls-files' patch), given that the 'tags' targets had found it necessary. However, when I did some 'informal' timing tests (ie 5x 'time make ...' and average), this did not make any noticeable difference for me (_even_ on cygwin!). ;-)
My complaint is not about the speed in a regular clone, but about *my* special clone, where I have some 50+ worktrees (that I really like to keep in the same directory, thankyouverymuch, I ignore them via .git/info/exclude, and I really like to have them all tucked away neatly under the /usr/src/git/ directory) and also some 30+ cloned repositories in 3rdparty/ for random testing (including a bare clone of the Linux kernel, of course also ignored via .git/info/exclude).
In that scenario, my version of find takes ages, only to finally even throw a segmentation fault (!).
Quoted 11 lines
So, I already have a 'hdr-check-fixup' branch (I think I have already mentioned it), in which the first commit looks like:
diff --git a/Makefile b/Makefile index c5240942f2..3401d1b9fb 100644 --- a/Makefile +++ b/Makefile @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE .PHONY: sparse $(SP_OBJ) sparse: $(SP_OBJ)
Quoted 2 lines
+HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \
+ sha1dc/*.h refs/*.h vcs-svn/*.h)
But of course that would also share at least to a lesser extend the shortcomings of a static list.
I still like the ls-files method the best.
Quoted 11 lines
GEN_HDRS := command-list.h unicode-width.h -EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS)) HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) $(HCO): %.hco: %.h FORCE... which drops the use of
LIB_Hentirely.
Quoted 8 lines
Now, I have timed this and, for me, it no faster ... (I suspect that it would be faster for Johannes, but it would still cause a problem if you have 'top-level header files from some other repo ...').
So, if we really need to solve the problem, allowing for some unrelated headers in your worktree, then we can only use a static list, or a 'git ls-files' approach.
Or we can use ls-files, and fall back to your wildcard idea if ls-files fails for some reason (typically because .git/ is missing, e.g. in case of an unpacked source .tar).
Quoted 3 lines
Anyway, for now, since I seem to be the only person using this target, I think we should just remove it while I think again. (I can put it in my config.mak, so there will be no loss for me).
As I said above, I would rather keep it, with the ls-files and := fixup.
If others *really* feel strongly about lazy-evaluating LIB_H, then I would *additionally* use Peff's sub-make hack. But only if others feel strongly about it, because I, for one, certainly don't.
Ciao,
Dscho
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 5 lines · Johannes Schindelin
Quoted 2 lines
So, tl;dr: soon, I will be submitting a patch to remove the 'hdr-check' target completely, for now anyway.
You mentioned later that you might be the only person using that target, and if that were so, I would agree.
It is obviously difficult to judge such a thing, but I suspect that the number is very low, if not exactly one. ;-)
Quoted 2 lines
However, I started looking into adding
hdr-checkto the Azure Pipeline we have (which would make a *ton* of sense, if you ask me), along with the
Hmm, I am in two minds about this - I feel duty bound to warn you that, if you go ahead with this idea, you will likely create a rod for your own back! :-D
If the false positive rate can be kept reasonably low, then this would be a good idea. However, some projects are just not a good fit for this kind of thing (and Git may be one of them).
Quoted 3 lines
sparsething we talked about.So I am a bit opposed to removing that target.
OK, I took the 'nuclear' route because this affects people (to a greater or lesser degree) even when they don't know/care about this target. If it is just (or mostly) me, then it didn't seem worth causing issues for you (or anyone else working on GTW?).
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
By the way, "make hdr-check" already fails for me on master, as I do not have libgcrypt installed, and it unconditionally checks
sha256/gcrypt.h.Yep, for me too. There is a similar problem if you build with
NO_CURLand don't have the 'curl/curl.h' header file, etc ...I think all it needs to do is to pass
$(ALL_CFLAGS)to gcc (although we might need to add something like#if defined(SHA256_GCRYPT) ... #endifguards tosha256/gcrypt.hto make it work in Peff's case).
Just adding $(ALL_CFLAGS) to the command-line is not a cure-all, unfortunately! :( Also, the 'sha256/gcrypt.h' should be excluded from the list of headers to check, conditionally, based on the build variable SHA256_GCRYPT.
Quoted 4 lines
But given that this target really is meant to catch all kinds of errors, I'd be in favor of declaring that target requiring things like libgcrypt's header files to be installed. We can easily make that happen in our CI builds.
That would help, but you may not be able to cover all the platform specific build options available. dunno. (I haven't given it enough thought).
Quoted 6 lines
Quoted 5 lines
The next version was essentially the same patch that started this thread from Johannes (ie. the 'git ls-files' patch), given that the 'tags' targets had found it necessary. However, when I did some 'informal' timing tests (ie 5x 'time make ...' and average), this did not make any noticeable difference for me (_even_ on cygwin!). ;-)
Quoted 7 lines
My complaint is not about the speed in a regular clone, but about *my* special clone, where I have some 50+ worktrees (that I really like to keep in the same directory, thankyouverymuch, I ignore them via
.git/info/exclude, and I really like to have them all tucked away neatly under the/usr/src/git/directory) and also some 30+ cloned repositories in3rdparty/for random testing (including a bare clone of the Linux kernel, of course also ignored via.git/info/exclude).
Oh, wow! I can't imagine ever doing anything like that. However, it is a perfectly valid setup and the build system should not make it hard for you to work the way you want.
Quoted 2 lines
In that scenario, my version of
findtakes ages, only to finally even throw a segmentation fault (!).
At least the segfault curtailed your misery! :-D
Quoted 11 lines
Quoted 11 lines
So, I already have a 'hdr-check-fixup' branch (I think I have already mentioned it), in which the first commit looks like:
diff --git a/Makefile b/Makefile index c5240942f2..3401d1b9fb 100644 --- a/Makefile +++ b/Makefile @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE .PHONY: sparse $(SP_OBJ) sparse: $(SP_OBJ)
Quoted 5 lines
Quoted 2 lines
+HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \
+ sha1dc/*.h refs/*.h vcs-svn/*.h)But of course that would also share at least to a lesser extend the shortcomings of a static list.
Yes, but it is no worse than ...
Quoted 5 lines
I still like the
ls-filesmethod the best.Quoted 3 lines
GEN_HDRS := command-list.h unicode-width.h -EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
... the original, manually maintained, exception list!
(This patch was written before the 'sha256/' directory existed. So the fact that it 'fixes' the GCRYPT error is simply an accident of timing! Today, it would be conditionally added to the exception list. Yes, I watched that error go from the 'pu' branch down to the 'master' branch).
Quoted 8 lines
Quoted 8 lines
+CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS)) HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))$(HCO): %.hco: %.h FORCE... which drops the use of
LIB_Hentirely.
Quoted 7 lines
Quoted 3 lines
So, if we really need to solve the problem, allowing for some unrelated headers in your worktree, then we can only use a static list, or a 'git ls-files' approach.
Or we can use
ls-files, and fall back to your wildcard idea ifls-filesfails for some reason (typically because.git/is missing, e.g. in case of an unpacked source .tar).
Yes, I think an 'git ls-files' approach is the way to go. (I am not sure that the 'hdr-check' target would be of any use 'offline' at all, but I suppose we could use a generated file in that case).
Quoted 6 lines
Quoted 3 lines
Anyway, for now, since I seem to be the only person using this target, I think we should just remove it while I think again. (I can put it in my config.mak, so there will be no loss for me).
As I said above, I would rather keep it, with the
ls-filesand:=fixup.
I would be happy with that, if you are. :-D
ATB,
Ramsay Jones
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
In reply to Johannes SchindelinQuoted 11 lines · Johannes Schindelin
Quoted 2 lines
So, tl;dr: soon, I will be submitting a patch to remove the 'hdr-check' target completely, for now anyway.
You mentioned later that you might be the only person using that target, and if that were so, I would agree.
However, I started looking into adding
hdr-checkto the Azure Pipeline we have (which would make a *ton* of sense, if you ask me), along with thesparsething we talked about.So I am a bit opposed to removing that target.
Yeah, I agree. If it's a linting check we expect developers to follow, then we need to make it easy for developers to run it.
Quoted 11 lines
Quoted 2 lines
Yep, for me too. There is a similar problem if you build with
NO_CURLand don't have the 'curl/curl.h' header file, etc ...I think all it needs to do is to pass
$(ALL_CFLAGS)to gcc (although we might need to add something like#if defined(SHA256_GCRYPT) ... #endifguards tosha256/gcrypt.hto make it work in Peff's case).But given that this target really is meant to catch all kinds of errors, I'd be in favor of declaring that target requiring things like libgcrypt's header files to be installed. We can easily make that happen in our CI builds.
I don't think that really scales, though. For CI, perhaps it's not too bad, but IMHO there's real value making it possible for everyday developers to run linting and tests locally, because it keeps the feedback cycle tight. In other words, imagine my changes go through a sequence like this:
1. I run "make" locally
2. I run "make test" locally
3. I push, and CI runs "make && make test" on more platforms, or with more knobs tweaked.
it's preferable to find out about problems in the early steps, because the cost to run each of the subsequent steps is much higher.
So if we're adding more linting, it would ideally be part of the build. The obvious reasons not to are that it's expensive, or that it's hard for the local user to set up. But I think at least a hdr-check that checks _your_ platform would be valuable to run as part of step 1 or 2.
I also think it points to a weakness in the current hdr-check scheme. It omits compat/* entirely, so we can't detect any problems there. But if we are going to add those back in, then making it run in CI is not as simple as "just make sure we have all libraries installed". It's inherently going to be a different set per platform (and we'd want to run "make hdr-check" on each of the platforms where we already run "make test").
Quoted 10 lines
Quoted 3 lines
Anyway, for now, since I seem to be the only person using this target, I think we should just remove it while I think again. (I can put it in my config.mak, so there will be no loss for me).
As I said above, I would rather keep it, with the
ls-filesand:=fixup.If others *really* feel strongly about lazy-evaluating
LIB_H, then I would *additionally* use Peff's sub-make hack. But only if others feel strongly about it, because I, for one, certainly don't.
My measurements show it's not a big expense at least on my system (even with the 3x runs). So I can live with just moving it to ls-files to bound the filesystem access, and calling that good enough.
-Peff
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
In reply to Jeff KingHi Peff,
Quoted 2 lines · Jeff King
By the way, "make hdr-check" already fails for me on master, as I do not have libgcrypt installed, and it unconditionally checks
sha256/gcrypt.h.
Oh? I thought it'd fail only on Windows, so I have not submitted
https://github.com/dscho/git/commit/fix-hdr-check yet:
-- snipsnap --
From c13d9985284d4b452db0d95b6949e02c533db634 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <[email protected]>
Date: Sat, 23 Feb 2019 20:38:40 +0100
Subject: [PATCH] hdr-check: make it work on Windows
On Windows, we define a specific set of pre-processor macros, among other reasons: to avoid including syslog.h (which is not available on Windows).
The hdr-check target did not use those definitions, resulting in a failure to include said syslog.h.
To fix that, let's let the hdr-check target make use of ALL_CFLAGS, which does include the pre-processor macros that would e.g. skip the conditional block including syslog.h.
--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
@@ -2741,7 +2741,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) $(HCO): %.hco: %.h FORCE - $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< + $(QUIET_HDR)$(CC) $(ALL_CFLAGS) -include git-compat-util.h -I. -o /dev/null -c -xc $< .PHONY: hdr-check $(HCO) hdr-check: $(HCO)
Signature · 1 line
2.20.1.windows.1.1576.g37ac248661.dirty
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 10 lines · Johannes Schindelin
-- snipsnap --
Fromc13d9985284d4b452db0d95b6949e02c533db634Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <[email protected]>
Date: Sat, 23 Feb 2019 20:38:40 +0100
Subject: [PATCH] hdr-check: make it work on WindowsOn Windows, we define a specific set of pre-processor macros, among other reasons: to avoid including
syslog.h(which is not available on Windows).
Quoted 6 lines
The hdr-check target did not use those definitions, resulting in a failure to include said
syslog.h.To fix that, let's let the hdr-check target make use of
ALL_CFLAGS, which does include the pre-processor macros that would e.g. skip the conditional block includingsyslog.h.
This makes sense to me, though as you noted elsewhere, it doesn't fix the gcrypt problem, since that file unconditionally wants to look at the system gcrypt.h (and we control at the Makefile level whether we actually look at sha256/gcrypt.h).
-Peff
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 4 lines · Jeff King
This makes sense to me, though as you noted elsewhere, it doesn't fix the gcrypt problem, since that file unconditionally wants to look at the system
gcrypt.h(and we control at theMakefilelevel whether we actually look atsha256/gcrypt.h).
Hmm, is that because the header check target does not know which *.h files we ship are actually used in a particular build?
After a normal build, with dynamic dependency checking on, we would have sufficient information to figure it out. Would that help?
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Hi Junio,
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Jeff King
This makes sense to me, though as you noted elsewhere, it doesn't fix the gcrypt problem, since that file unconditionally wants to look at the system
gcrypt.h(and we control at theMakefilelevel whether we actually look atsha256/gcrypt.h).
Quoted 5 lines
Hmm, is that because the header check target does not know which *.h files we ship are actually used in a particular build?
After a normal build, with dynamic dependency checking on, we would have sufficient information to figure it out. Would that help?
Yes, *if* the dynamic dependency checking is in effect (read: if we are compiling with GCC).
However, I think that one of the benefits of the current approach is that hdr-check finds issues also in headers that are *not* part of the current build. Read: once hdr-check is run as part of the CI build, it is harder for some random contributor to break hdr-check for somebody else with different compile time options.
Ciao,
Dscho
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 11 lines · Johannes Schindelin
Quoted 2 lines
After a normal build, with dynamic dependency checking on, we would have sufficient information to figure it out. Would that help?
Yes, *if* the dynamic dependency checking is in effect (read: if we are compiling with GCC).
However, I think that one of the benefits of the current approach is that hdr-check finds issues also in headers that are *not* part of the current build. Read: once hdr-check is run as part of the CI build, it is harder for some random contributor to break hdr-check for somebody else with different compile time options.
Sure. That is exactly why we have multiple configurations at the CI, hopefully more than each individual contributor has at hand so that CI can give more coverage than any single individual can test.
What I was assuming was that at least one of these multiple configurations would cover the case where sha256/gcrypt.h truly is used, because that particular build is configured to use the system header and library. And it would be a far better approach to make that build check also for the header, if we can cover all the problematic (in the same sense as the sha256/gcrypt.h) header files, than unconditionally "checking" without knowing the prerequisite and causing a false positive.
Having said that, the optional use of ls-files introduced by the patch in question is a valid incremental improvement over the current Makefile, so it is a pretty much independent/orthogonal issue.
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
In reply to Junio C HamanoQuoted 9 lines · Junio C Hamano
Quoted 4 lines · Jeff King
This makes sense to me, though as you noted elsewhere, it doesn't fix the gcrypt problem, since that file unconditionally wants to look at the system
gcrypt.h(and we control at theMakefilelevel whether we actually look atsha256/gcrypt.h).Hmm, is that because the header check target does not know which *.h files we ship are actually used in a particular build?
Yes, exactly.
Quoted 2 lines
After a normal build, with dynamic dependency checking on, we would have sufficient information to figure it out. Would that help?
Yeah, that's what I was hinting at earlier in the thread. Here it is sketched out to an actual working patch. The sub-make bits could actually be a shell script instead of a Makefile; the only point in using make is to use the parent "-j" for parallelism.
Note also that by including compat/ headers, I noticed that bswap.h fails its hdr-check. :)
So do command-list.h (it needs gettext.h to handle the _() markings, but that only comes in via cache.h), and all of xdiff (which is probably fixable, but I didn't bother here).
--- Makefile | 23 ++++++++++++----------- compat/bswap.h | 5 +++++ 2 files changed, 17 insertions(+), 11 deletions(-)
Makefile +12 -11
@@ -1852,7 +1852,6 @@ ifndef V QUIET_MSGFMT = @echo ' ' MSGFMT $@; QUIET_GCOV = @echo ' ' GCOV $@; QUIET_SP = @echo ' ' SP $<; - QUIET_HDR = @echo ' ' HDR $<; QUIET_RC = @echo ' ' RC $@; QUIET_SUBDIR0 = +@subdir= QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \
@@ -2735,16 +2734,18 @@ $(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% -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) -HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) - -$(HCO): %.hco: %.h FORCE - $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< - -.PHONY: hdr-check $(HCO) -hdr-check: $(HCO) +.PHONY: hdr-check +hdr-check: all +ifdef USE_COMPUTED_HEADER_DEPENDENCIES + @$(MAKE) -f hdr-check.mak CC="$(CC)" V=$(V) \ + $$(sed -n 's/^\([^ ]*\)\.h:/\1.hco/p' .depend/* | \ + sort -u | \ + egrep -v '^(xdiff/|unicode-width.h|command-list.h)' \ + ) +else + @echo >&2 "error: hdr-check supported only on platforms with computed dependencies" + @false +endif .PHONY: style style:
compat/bswap.h +5 -0
@@ -1,3 +1,6 @@ +#ifndef COMPAT_BSWAP_H +#define COMPAT_BSWAP_H +/* * Let's make sure we always have a sane definition for ntohl()/htonl(). * Some libraries define those as a function call, just to perform byte
@@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value) }#endif + +#endif /* COMPAT_BSWAP_H */
Signature · 1 line
2.21.0.684.gc9dc8b89c9
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 14 lines · Jeff King
Quoted 9 lines · Junio C Hamano
Quoted 4 lines · Jeff King
This makes sense to me, though as you noted elsewhere, it doesn't fix the gcrypt problem, since that file unconditionally wants to look at the system
gcrypt.h(and we control at theMakefilelevel whether we actually look atsha256/gcrypt.h).Hmm, is that because the header check target does not know which *.h files we ship are actually used in a particular build?
Yes, exactly.
Quoted 7 lines
Quoted 2 lines
After a normal build, with dynamic dependency checking on, we would have sufficient information to figure it out. Would that help?
Yeah, that's what I was hinting at earlier in the thread. Here it is sketched out to an actual working patch. The sub-make bits could actually be a shell script instead of a
Makefile; the only point in using make is to use the parent "-j" for parallelism.
sigh. :(
I wish my patch removing this target had been picked up now!
Earlier this evening I spent an hour or so writing an email which tried to discourage spending any time on this, because of the potential for this to be a huge time-suck. An unrewarding one at that! :-D
The email was actually prompted by someone mentioning 'dynamic compiler dependencies', because that's how it always starts ...
I deleted that email (it's not in my drafts folder anyway) because, in the end, it is not up to me to say how people should spend their time.
So I won't! :-D
ATB,
Ramsay Jones
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 5 lines · Ramsay Jones
Quoted 4 lines
Yeah, that's what I was hinting at earlier in the thread. Here it is sketched out to an actual working patch. The sub-make bits could actually be a shell script instead of a
Makefile; the only point in using make is to use the parent "-j" for parallelism.
Quoted 8 lines
sigh. :(
I wish my patch removing this target had been picked up now!
Earlier this evening I spent an hour or so writing an email which tried to discourage spending any time on this, because of the potential for this to be a huge time-suck. An unrewarding one at that! :-D
Heh. I am OK with removing it, too.
My thinking earlier in the thread was that it should go in our bag of linting tools that people should generally run. But actually, it is kind of expensive to run, and it does not actually help anything immediately in practice. I.e., what we really care about is that the C source files compile, and running "make" does that (and especially running it on various platforms). This is just checking for a _potential_ problem if somebody were to include a particular header file at the start of another C file.
So it's really about 2 steps removed from stopping an actual problem.
Quoted 3 lines
I deleted that email (it's not in my drafts folder anyway) because, in the end, it is not up to me to say how people should spend their time.
FWIW, it was a fun exercise to build on the compiler dependencies. ;) And I think I'm done playing with it for now. "make hdr-check" does not run for me, but I think I've convinced myself that it's not all that important that I run it.
I did forget to include one file in my earlier patch (the newly added hdr-check.mak). I imagine one could guess at its contents, but here is the complete patch, for the sake of reference.
-Peff
--- Makefile | 23 ++++++++++++----------- compat/bswap.h | 5 +++++ hdr-check.mak | 8 ++++++++ 3 files changed, 25 insertions(+), 11 deletions(-) create mode 100644 hdr-check.mak
Makefile +12 -11
@@ -1852,7 +1852,6 @@ ifndef V QUIET_MSGFMT = @echo ' ' MSGFMT $@; QUIET_GCOV = @echo ' ' GCOV $@; QUIET_SP = @echo ' ' SP $<; - QUIET_HDR = @echo ' ' HDR $<; QUIET_RC = @echo ' ' RC $@; QUIET_SUBDIR0 = +@subdir= QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \
@@ -2735,16 +2734,18 @@ $(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% -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) -HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) - -$(HCO): %.hco: %.h FORCE - $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< - -.PHONY: hdr-check $(HCO) -hdr-check: $(HCO) +.PHONY: hdr-check +hdr-check: all +ifdef USE_COMPUTED_HEADER_DEPENDENCIES + @$(MAKE) -f hdr-check.mak CC="$(CC)" V=$(V) \ + $$(sed -n 's/^\([^ ]*\)\.h:/\1.hco/p' .depend/* | \ + sort -u | \ + egrep -v '^(xdiff/|unicode-width.h|command-list.h)' \ + ) +else + @echo >&2 "error: hdr-check supported only on platforms with computed dependencies" + @false +endif .PHONY: style style:
compat/bswap.h +5 -0
@@ -1,3 +1,6 @@ +#ifndef COMPAT_BSWAP_H +#define COMPAT_BSWAP_H +/* * Let's make sure we always have a sane definition for ntohl()/htonl(). * Some libraries define those as a function call, just to perform byte
@@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value) }#endif + +#endif /* COMPAT_BSWAP_H */
hdr-check.mak new file +8 -0
@@ -0,0 +1,8 @@ +.PHONY: FORCE + +ifndef V +QUIET_HDR = @echo ' ' HDR $<; +endif + +%.hco: %.h FORCE + $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
Signature · 1 line
2.21.0.699.ge1748d4d73
Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 9 lines · Jeff King
Quoted 7 lines · Ramsay Jones
Quoted 4 lines
Yeah, that's what I was hinting at earlier in the thread. Here it is sketched out to an actual working patch. The sub-make bits could actually be a shell script instead of a
Makefile; the only point in using make is to use the parent "-j" for parallelism.sigh. :(
Quoted 8 lines
Quoted 6 lines
I wish my patch removing this target had been picked up now!
Earlier this evening I spent an hour or so writing an email which tried to discourage spending any time on this, because of the potential for this to be a huge time-suck. An unrewarding one at that! :-D
Heh. I am OK with removing it, too.
FWIW, I am fine with that plan as well.
Quoted 10 lines
My thinking earlier in the thread was that it should go in our bag of linting tools that people should generally run. But actually, it is kind of expensive to run, and it does not actually help anything immediately in practice. I.e., what we really care about is that the C source files compile, and running "make" does that (and especially running it on various platforms). This is just checking for a _potential_ problem if somebody were to include a particular header file at the start of another C file.
So it's really about 2 steps removed from stopping an actual problem.
Yeah, exactly.
Quoted 12 lines
compat/bswap.h +3
-0@@ -1,3 +1,6 @@ +#ifndef COMPAT_BSWAP_H +#define COMPAT_BSWAP_H +/* * Let's make sure we always have a sane definition for ntohl()/htonl(). * Some libraries define those as a function call, just to perform byte
compat/bswap.h static inline void put_be64(void *ptr, uint64_t value) · line 217
Quoted 5 lines
compat/bswap.h +2
-0@@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value) }#endif + +#endif /* COMPAT_BSWAP_H */
This probably is worth having as an independent clean-up.
PATCH 1/1 compat/bswap: add include header guards
Quoted 5 lines · Junio C Hamano
Review
Quoted 2 lines
(quoted diff) +2
-0+#ifndef COMPAT_BSWAP_H +#define COMPAT_BSWAP_H[...]
This probably is worth having as an independent clean-up.
Yeah, let's do that now before we forget.
-- >8 --
Subject: [PATCH] compat/bswap: add include header guards
Our compat/bswap.h lacks the usual preprocessor guards against multiple inclusion. This usually isn't an issue since it only gets included from git-compat-util.h, which has its own guards. But it would produce redeclaration errors if any file included it separately.
Our hdr-check target would complain about this, except that it currently skips items in compat/ entirely.
--- compat/bswap.h | 5 +++++ 1 file changed, 5 insertions(+)
compat/bswap.h +5 -0
@@ -1,3 +1,6 @@ +#ifndef COMPAT_BSWAP_H +#define COMPAT_BSWAP_H +/* * Let's make sure we always have a sane definition for ntohl()/htonl(). * Some libraries define those as a function call, just to perform byte
@@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value) }#endif + +#endif /* COMPAT_BSWAP_H */
Signature · 1 line
2.21.0.699.ge1748d4d73
Re: [PATCH] compat/bswap: add include header guards
Quoted 9 lines · Jeff King
Quoted 5 lines · Junio C Hamano
compat/bswap.h line 2
Quoted 2 lines
compat/bswap.h +2
-0+#ifndef COMPAT_BSWAP_H +#define COMPAT_BSWAP_H [...]
This probably is worth having as an independent clean-up.
Yeah, let's do that now before we forget.
Thanks
Quoted 4 lines
-- >8 --
Subject: [PATCH] compat/bswap: add include header guards
...
v2 cover Avoid calling find in the Makefile, if possible
In reply to Johannes Schindelin via GitGitGadgetI noticed this quite a bit of time ago, but did not get a chance to look into it in detail: all of a sudden, make started really slowly over here.
The culprit turned out to be a find call, which was in the Makefile for ages, so I was puzzled why it only caused problems recently.
After some digging, I found out that the hdr-check target is the culprit:
before it was introduced, $(LIB_H) did not need to be expanded, and as a consequence the find call was not executed. Once hdr-check made it into master, though, $(LIB_H) must be expanded to define that rule.
Since I have tons of worktrees as subdirectories of my principal Git clone, and since I also have a 3rdparty/ directory with tons of repositories I use for various testing/contributing purposes, this find is quite a little slow over here.
So here is my suggested fix. It is based on similar code we already had in the Makefile, obviously also intended to avoid an expensive find invocation.
Changes since v1:
* Since LIB_H's lazy evaluation kicks in all the time anyway, changed the = to := to avoid evaluating it three times.
* Clarified in the commit message that the existing sites using $(LIB_H) are not affected by this change (or at least not affected as long as no untracked header files are included in .c files).
Johannes Schindelin (1):
Makefile: use git ls-files to list header files, if possible
Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
base-commit: c65a2884eae159bad540135479bc8afe20ff62d1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-130%2Fdscho%2Favoid-find-in-makefile-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-130/dscho/avoid-find-in-makefile-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/130
Range-diff vs v1:
1: 0b5529406b ! 1: cb253bd0cf Makefile: use git ls-files to list header files, if possible
@@ -29,7 +29,11 @@
than from a clone of Git's sources).
This has one notable consequence: we no longer include command-list.h
- in LIB_H, as it is a generated file, not a tracked one.
+ in LIB_H, as it is a generated file, not a tracked one, but that is
+ easily worked around. Of the three sites that use LIB_H, two
+ (LOCALIZED_C and CHK_HDRS) already handle generated headers
+ separately. In the third, the computed-dependency fallback, we can just
+ add in a reference to $(GENERATED_H).
Likewise, we no longer include not-yet-tracked header files in LIB_H.
@@ -46,7 +50,7 @@
GENERATED_H += command-list.h
-LIB_H = $(shell $(FIND) . \
-+LIB_H = $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
++LIB_H := $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
+ $(FIND) . \
-name .git -prune -o \
-name t -prune -o \
Signature · 1 line
gitgitgadget
PATCH v2 1/1 Makefile: use `git ls-files` to list header files, if possible
From: Johannes Schindelin <[email protected]>
In d85b0dff72 (Makefile: use find to determine static header dependencies, 2014-08-25), we switched from a static list of header files to a dynamically-generated one, asking find to enumerate them.
Back in those days, we did not use $(LIB_H) by default, and many a make implementation seems smart enough not to run that find command in that case, so it was deemed okay to run find for special targets requiring this macro.
However, as of ebb7baf02f (Makefile: add a hdr-check target, 2018-09-19), $(LIB_H) is part of a global rule and therefore must be expanded. Meaning: this find command has to be run upon every make invocation. In the presence of many a worktree, this can tax the developers' patience quite a bit.
Even in the absence of worktrees or other untracked files and directories, the cost of I/O to generate that list of header files is simply a lot larger than a simple git ls-files call.
Therefore, just like in 335339758c (Makefile: ask "ls-files" to list source files if available, 2011-10-18), we now prefer to use git ls-files to enumerate the header files to enumerating them via find, falling back to the latter if the former failed (which would be the case e.g. in a worktree that was extracted from a source .tar file rather than from a clone of Git's sources).
This has one notable consequence: we no longer include command-list.h in LIB_H, as it is a generated file, not a tracked one, but that is easily worked around. Of the three sites that use LIB_H, two (LOCALIZED_C and CHK_HDRS) already handle generated headers separately. In the third, the computed-dependency fallback, we can just add in a reference to $(GENERATED_H).
Likewise, we no longer include not-yet-tracked header files in LIB_H.
Given the speed improvements, these consequences seem a comparably small price to pay.
--- Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Makefile +3 -2
@@ -841,7 +841,8 @@ VCSSVN_LIB = vcs-svn/lib.a GENERATED_H += command-list.h -LIB_H = $(shell $(FIND) . \ +LIB_H := $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \ + $(FIND) . \ -name .git -prune -o \ -name t -prune -o \ -name Documentation -prune -o \
@@ -2363,7 +2364,7 @@ else # should _not_ be included here, since they are necessary even when # building an object for the first time. -$(OBJECTS): $(LIB_H) +$(OBJECTS): $(LIB_H) $(GENERATED_H) endif exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 28 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
In
d85b0dff72(Makefile: usefindto determine static header dependencies, 2014-08-25), we switched from a static list of header files to a dynamically-generated one, askingfindto enumerate them.Back in those days, we did not use
$(LIB_H)by default, and many amakeimplementation seems smart enough not to run thatfindcommand in that case, so it was deemed okay to runfindfor special targets requiring this macro.However, as of
ebb7baf02f(Makefile: add a hdr-check target, 2018-09-19), $(LIB_H) is part of a global rule and therefore must be expanded. Meaning: thisfindcommand has to be run upon everymakeinvocation. In the presence of many a worktree, this can tax the developers' patience quite a bit.Even in the absence of worktrees or other untracked files and directories, the cost of I/O to generate that list of header files is simply a lot larger than a simple
git ls-filescall.Therefore, just like in
335339758c(Makefile: ask "ls-files" to list source files if available, 2011-10-18), we now prefer to usegit ls-filesto enumerate the header files to enumerating them viafind, falling back to the latter if the former failed (which would be the case e.g. in a worktree that was extracted from a source .tar file rather than from a clone of Git's sources).
Quoted 2 lines
This has one notable consequence: we no longer include
command-list.hinLIB_H, as it is a generated file, not a tracked one, but that is
Heh, just to be _unnecessarily_ picky, but this is not always true. The 'command-list.h' header is _sometimes_ not included in the LIB_H variable - it simply depends on whether it has been generated by the time the $(FIND) is called.
Obviously, not worth a re-roll. Otherwise, this LGTM.
Thanks!
ATB,
Ramsay Jones
Quoted 39 lines
easily worked around. Of the three sites that use
LIB_H, two (LOCALIZED_CandCHK_HDRS) already handle generated headers separately. In the third, the computed-dependency fallback, we can just add in a reference to $(GENERATED_H).Likewise, we no longer include not-yet-tracked header files in
LIB_H.Given the speed improvements, these consequences seem a comparably small price to pay.
Signed-off-by Johannes Schindelin <[email protected]>--- Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)Makefile +3
-2@@ -841,7 +841,8 @@ VCSSVN_LIB = vcs-svn/lib.a GENERATED_H += command-list.h -LIB_H = $(shell $(FIND) . \ +LIB_H := $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \ + $(FIND) . \ -name .git -prune -o \ -name t -prune -o \ -name Documentation -prune -o \@@ -2363,7 +2364,7 @@ else # should _not_ be included here, since they are necessary even when # building an object for the first time. -$(OBJECTS): $(LIB_H) +$(OBJECTS): $(LIB_H) $(GENERATED_H) endif exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX
Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible
Quoted 34 lines · Ramsay Jones
Quoted 28 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
In
d85b0dff72(Makefile: usefindto determine static header dependencies, 2014-08-25), we switched from a static list of header files to a dynamically-generated one, askingfindto enumerate them.Back in those days, we did not use
$(LIB_H)by default, and many amakeimplementation seems smart enough not to run thatfindcommand in that case, so it was deemed okay to runfindfor special targets requiring this macro.However, as of
ebb7baf02f(Makefile: add a hdr-check target, 2018-09-19), $(LIB_H) is part of a global rule and therefore must be expanded. Meaning: thisfindcommand has to be run upon everymakeinvocation. In the presence of many a worktree, this can tax the developers' patience quite a bit.Even in the absence of worktrees or other untracked files and directories, the cost of I/O to generate that list of header files is simply a lot larger than a simple
git ls-filescall.Therefore, just like in
335339758c(Makefile: ask "ls-files" to list source files if available, 2011-10-18), we now prefer to usegit ls-filesto enumerate the header files to enumerating them viafind, falling back to the latter if the former failed (which would be the case e.g. in a worktree that was extracted from a source .tar file rather than from a clone of Git's sources).Quoted 2 lines
This has one notable consequence: we no longer include
command-list.hinLIB_H, as it is a generated file, not a tracked one, but that is
Quoted 6 lines
Heh, just to be _unnecessarily_ picky, but this is not always true. The '
command-list.h' header is _sometimes_ not included in theLIB_Hvariable - it simply depends on whether it has been generated by the time the $(FIND) is called.Obviously, not worth a re-roll. Otherwise, this LGTM.
Ahem! Obviously, I didn't read the commit message closely enough!
However, _before_ this change, then 'command-list.h' was sometimes not included in $(LIB_H) ...
Sorry for the noise!
ATB,
Ramsay Jones
Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible
In reply to Johannes Schindelin via GitGitGadgetQuoted 6 lines · Johannes Schindelin via GitGitGadget
Therefore, just like in
335339758c(Makefile: ask "ls-files" to list source files if available, 2011-10-18), we now prefer to usegit ls-filesto enumerate the header files to enumerating them viafind, falling back to the latter if the former failed (which would be the case e.g. in a worktree that was extracted from a source .tar file rather than from a clone of Git's sources).
Thanks, this version looks good to me, and seems like an obvious first step regardless of whether we want to later push it into a sub-make.
-Peff