On Wed, Sep 22, 2021 at 05:58:16PM +0100, Ramsay Jones wrote: > > 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. > 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. > 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. > 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). > 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. > $ 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
