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:36 UTC
Message-ID: <20190301213619.GA1518@sigill.intra.peff.net>
In-Reply-To: <0b5529406b9458d37f3f5cdf38baa2d6a0a70a65.1551470265.git.gitgitgadget@gmail.com>

On Fri, Mar 01, 2019 at 11:57:46AM -0800, Johannes Schindelin via GitGitGadget wrote:

Show 14 quoted lines

> 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.

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.

Show 10 quoted 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-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).

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.

> 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)?

> Likewise, we no longer include not-yet-tracked header files in `LIB_H`.

I think that's probably OK.

-Peff