On Fri, Mar 01, 2019 at 04:36:19PM -0500, Jeff King wrote: > > 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. > > 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. Looping in Ramsay as the author of hdr-check. I think we could even do this without using a separate script, like so: diff --git a/Makefile b/Makefile index c5240942f2..a65c4599e9 100644 --- a/Makefile +++ b/Makefile @@ -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. > > 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. > > 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
