Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

From

Jeff King <peff@peff.net>

Date

Mar 1, 2019, 21:54 UTC

Message-ID

<20190301215414.GB1518@sigill.intra.peff.net>

In-Reply-To

<20190301213619.GA1518@sigill.intra.peff.net>

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.

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