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

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Mar 3, 2019, 21:30 UTC
Message-ID: <8e4b7966-7a3e-a081-bfc1-9e60303b8eab@ramsayjones.plus.com>
In-Reply-To: <20190303171951.GD23811@sigill.intra.peff.net>

On 03/03/2019 17:19, Jeff King wrote:

Show 11 quoted lines

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

So, tl;dr: soon, I will be submitting a patch to remove the 'hdr-check' target completely, for now anyway.

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

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!). ;-)

Of course, I had already made the mistake of trying to re-use something that was 'handy' (ie. LIB_H) and already there. However, LIB_H wasn't quite what I wanted - I needed a sub-set of it.

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

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

ATB, Ramsay Jones