On 04/03/2019 12:38, Johannes Schindelin wrote: >> 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. It is obviously difficult to judge such a thing, but I suspect that the number is very low, if not exactly one. ;-) > 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 Hmm, I am in two minds about this - I feel duty bound to warn you that, if you go ahead with this idea, you will likely create a rod for your own back! :-D If the false positive rate can be kept reasonably low, then this would be a good idea. However, some projects are just not a good fit for this kind of thing (and Git may be one of them). > `sparse` thing we talked about. > > So I am a bit opposed to removing that target. OK, I took the 'nuclear' route because this affects people (to a greater or lesser degree) even when they don't know/care about this target. If it is just (or mostly) me, then it didn't seem worth causing issues for you (or anyone else working on GTW?). >>> 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). Just adding $(ALL_CFLAGS) to the command-line is not a cure-all, unfortunately! :( Also, the 'sha256/gcrypt.h' should be excluded from the list of headers to check, conditionally, based on the build variable SHA256_GCRYPT. > 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. That would help, but you may not be able to cover all the platform specific build options available. dunno. (I haven't given it enough thought). >> 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`). Oh, wow! I can't imagine ever doing anything like that. However, it is a perfectly valid setup and the build system should not make it hard for you to work the way you want. > In that scenario, my version of `find` takes ages, only to finally even > throw a segmentation fault (!). At least the segfault curtailed your misery! :-D >> 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. Yes, but it is no worse than ... > 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))) ... the original, manually maintained, exception list! (This patch was written before the 'sha256/' directory existed. So the fact that it 'fixes' the GCRYPT error is simply an accident of timing! Today, it would be conditionally added to the exception list. Yes, I watched that error go from the 'pu' branch down to the 'master' branch). >> +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. >> >> 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). Yes, I think an 'git ls-files' approach is the way to go. (I am not sure that the 'hdr-check' target would be of any use 'offline' at all, but I suppose we could use a generated file in that case). >> 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. I would be happy with that, if you are. :-D ATB, Ramsay Jones
