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
