Web lists-archives.com

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




Hi Peff,

On Fri, 1 Mar 2019, Jeff King wrote:

> On Fri, Mar 01, 2019 at 11:57:46AM -0800, Johannes Schindelin via GitGitGadget wrote:
> 
> > 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.

Good point.

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

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.

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

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.

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