Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

From: Jeff King <peff@peff.net>
Date: Sep 22, 2021, 17:53 UTC
Message-ID: <YUttpgkU6eCOxMj/@coredump.intra.peff.net>
In-Reply-To: <c13b050c-abb9-c669-b92c-930f2b43ce10@ramsayjones.plus.com>

On Wed, Sep 22, 2021 at 05:58:16PM +0100, Ramsay Jones wrote:

Show 10 quoted lines

> > All three seem pretty reasonable to me.
> 
> Heh, interesting. My initial reaction was completely negative! ;-P
> (and not just mildly negative either, but 'you must be kidding').
> 
> However, I then thought 'I must be missing something, I'm being
> stupid and about to embarrass myself in public!'. So, I have
> been trying hard to understand what these patches are trying to
> accomplish and just what it is I'm missing. But, I'm coming up
> blank ...

I think the point is just avoiding repeated work. If you just manually run "make sparse" once in a while, then caching the result probably isn't of much value. But if you plan to run, say:

  git rebase -x 'make sparse'

then it would be nice for it to avoid checking the same files over and over.

Show 5 quoted lines

> At the heart of my unease is dependencies (or rather the lack) for
> the 'synthetic object files' *.hco and *.sp. (Also, the addition
> of even more 'shrapnel' to the build directories - I wrote a patch
> to remove the useless *.hcc files just after commit b503a2d515e was
> included, but didn't get around to submitting it).

I don't consider them shrapnel if they're holding useful results. :) But I agree that this does make "ls" in the top-level increasingly cluttered (curiously, I find that's something I rarely do, but probably because I'm so used to knowing where everything is in this project).

Perhaps writing them to build-cruft/%.hco instead would help there. I guess there may be some complications around directory creation.

But overall, I do agree that if we can't make the dependencies solid here, this is not worth doing. Sacrificing correctness of the checks for reduced computation is not a good idea.

Show 20 quoted lines

> Hmm, odd:
>   
>   $ find . -iname '*.hcc' | sed s/.hcc// | sort >zz
>   $ find . -iname '*.hco' | sed s/.hco// | sort >xx
>   $ diff zz xx
>   90d89
>   < ./merge-strategies
>   137d135
>   < ./reftable/slice
>   152d149
>   < ./sha1-lookup
>   198,202d194
>   < ./vcs-svn/fast_export
>   < ./vcs-svn/line_buffer
>   < ./vcs-svn/sliding_window
>   < ./vcs-svn/svndiff
>   < ./vcs-svn/svndump
>   $ 
> 
> ... just noticed in passing, I didn't investigate.

I think some of that is cruft from old runs. We no longer have vcs-svn/*.c at all, for example.

Show 7 quoted lines

> Now, by definition, every '*.hcc' file depends on git-compat-util.h, so
> after changing that header an 'hdr-check' should check every header:
> 
>   $ touch git-compat-util.h
>   $ make hdr-check
>       HDR git-compat-util.h
>   $

Yeah, I think this is similar to the header-dependency thing I brought up for the sparse files. My thinking was that it wouldn't matter for the hdr-check, because by definition if a header you include changes, then we'd check it independently. But it's possible for foo.h to be fine itself, but bar.h which includes it to fail because of a change in foo.h (e.g., removing a type declaration).

Show 10 quoted lines

> Hmm, not quite. So, the sparse target should have similar problems:
>   
>   $ make sparse
>       * new build flags
>       SP abspath.c
>   ...
>       SP remote-curl.c
>   $ 
> 
> OK, that works.

Sort of. Your "new build flags" is what saved you there, which was triggered by something outside of your example commands. :)

As you saw below (and I showed in my earlier email), it doesn't work in the general case.

Show 9 quoted lines

>   $ make clean
>   ...
>   rm -f GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PERL-HEADER GIT-PYTHON-VARS
>   $ find . -iname '*.sp' | wc -l
>   452
>   $ 
>  
> Ah, yes, you may want to add the removal of the 'synthetic objects' to the
> make clean target!

Agreed.

> As I said, I don't quite understand what these patches want to do, so I can't
> offer any solutions. :( Well, you could *add* the necessary dependencies,
> of course, but that could lead to a rabbit hole which I would not want to
> go down!

I think depending on the ".o", as I mentioned earlier, is a good way of quickly getting those dependencies without having to specify any logic. Even though we don't use the ".o" file itself, it's a proxy for "all the things the .c file depends on".

-Peff