Hi Ramsay, On Sun, 3 Mar 2019, Ramsay Jones wrote: > On 03/03/2019 17:19, Jeff King wrote: > > On Sat, Mar 02, 2019 at 09:05:24PM +0100, Johannes Schindelin wrote: > > > >>> 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. > > > > Good point. > > [Sorry for the late reply - I have been AWOL this weekend and > I am only just catching up with email! :-D ] You should never feel the need to apologize for spending a weekend away from the keyboard (AKA "having a life"). > So, tl;dr: soon, I will be submitting a patch to remove the > 'hdr-check' target completely, for now anyway. You mentioned later that you might be the only person using that target, and if that were so, I would agree. However, I started looking into adding `hdr-check` to the Azure Pipeline we have (which would make a *ton* of sense, if you ask me), along with the `sparse` thing we talked about. So I am a bit opposed to removing that target. > > By the way, "make hdr-check" already fails for me on master, as I do > > not have libgcrypt installed, and it unconditionally checks > > sha256/gcrypt.h. > > Yep, for me too. There is a similar problem if you build with > NO_CURL and don't have the 'curl/curl.h' header file, etc ... I think all it needs to do is to pass `$(ALL_CFLAGS)` to gcc (although we might need to add something like `#if defined(SHA256_GCRYPT) ... #endif` guards to `sha256/gcrypt.h` to make it work in Peff's case). But given that this target really is meant to catch all kinds of errors, I'd be in favor of declaring that target requiring things like libgcrypt's header files to be installed. We can easily make that happen in our CI builds. > The first version of the 'hdr-check' target re-introduced a static list > of header files, but I didn't think people would appreciate having to > maintain the list manually, so I gave up on that! > > The next version was essentially the same patch that started this thread > from Johannes (ie. the 'git ls-files' patch), given that the 'tags' > targets had found it necessary. However, when I did some 'informal' > timing tests (ie 5x 'time make ...' and average), this did not make any > noticeable difference for me (_even_ on cygwin!). ;-) My complaint is not about the speed in a regular clone, but about *my* special clone, where I have some 50+ worktrees (that I really like to keep in the same directory, thankyouverymuch, I ignore them via `.git/info/exclude`, and I really like to have them all tucked away neatly under the `/usr/src/git/` directory) and also some 30+ cloned repositories in `3rdparty/` for random testing (including a bare clone of the Linux kernel, of course also ignored via `.git/info/exclude`). In that scenario, my version of `find` takes ages, only to finally even throw a segmentation fault (!). > So, I already have a 'hdr-check-fixup' branch (I think I have > already mentioned it), in which the first commit looks like: > > diff --git a/Makefile b/Makefile > index c5240942f2..3401d1b9fb 100644 > --- a/Makefile > +++ b/Makefile > @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE > .PHONY: sparse $(SP_OBJ) > sparse: $(SP_OBJ) > > +HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \ > + sha1dc/*.h refs/*.h vcs-svn/*.h) But of course that would also share at least to a lesser extend the shortcomings of a static list. I still like the `ls-files` method the best. > GEN_HDRS := command-list.h unicode-width.h > -EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% > -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) > +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS)) > HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) > > $(HCO): %.hco: %.h FORCE > > > ... which drops the use of LIB_H entirely. > > Now, I have timed this and, for me, it no faster ... (I suspect > that it would be faster for Johannes, but it would still cause > a problem if you have 'top-level header files from some other > repo ...'). > > So, if we really need to solve the problem, allowing for some > unrelated headers in your worktree, then we can only use a > static list, or a 'git ls-files' approach. Or we can use `ls-files`, and fall back to your wildcard idea if `ls-files` fails for some reason (typically because `.git/` is missing, e.g. in case of an unpacked source .tar). > Anyway, for now, since I seem to be the only person using this > target, I think we should just remove it while I think again. > (I can put it in my config.mak, so there will be no loss for me). As I said above, I would rather keep it, with the `ls-files` and `:=` fixup. If others *really* feel strongly about lazy-evaluating `LIB_H`, then I would *additionally* use Peff's sub-make hack. But only if others feel strongly about it, because I, for one, certainly don't. Ciao, Dscho
