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
