From: Jeff King <peff@peff.net>
To: Ramsay Jones <ramsay@ramsayjones.plus.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
"Denton Liu" <liu.denton@gmail.com>
Subject: Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY
Date: Wed, 22 Sep 2021 13:53:42 -0400 [thread overview]
Message-ID: <YUttpgkU6eCOxMj/@coredump.intra.peff.net> (raw)
In-Reply-To: <c13b050c-abb9-c669-b92c-930f2b43ce10@ramsayjones.plus.com>
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
next prev parent reply other threads:[~2021-09-22 17:53 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-21 22:55 [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY Ævar Arnfjörð Bjarmason
2021-09-21 22:55 ` [PATCH 1/3] Makefile: make the "sparse" target non-.PHONY Ævar Arnfjörð Bjarmason
2021-09-22 2:24 ` Jeff King
2021-09-21 22:55 ` [PATCH 2/3] Makefile: do one append in %.hcc rule Ævar Arnfjörð Bjarmason
2021-09-21 22:55 ` [PATCH 3/3] Makefile: make the "hdr-check" target non-.PHONY Ævar Arnfjörð Bjarmason
2021-09-22 2:11 ` [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY Jeff King
2021-09-22 16:58 ` Ramsay Jones
2021-09-22 17:53 ` Jeff King [this message]
2021-09-22 19:17 ` Ramsay Jones
2021-09-22 23:28 ` Junio C Hamano
2021-09-23 1:07 ` Ævar Arnfjörð Bjarmason
2021-09-23 1:23 ` Junio C Hamano
2021-09-23 2:17 ` Ævar Arnfjörð Bjarmason
2021-09-22 19:24 ` Junio C Hamano
2021-09-23 0:07 ` [PATCH v2] Makefile: make the "sparse" target non-.PHONY Ævar Arnfjörð Bjarmason
2021-09-23 16:24 ` Jeff King
2021-09-23 17:06 ` Ævar Arnfjörð Bjarmason
2021-09-23 17:17 ` Jeff King
2021-09-23 17:39 ` Junio C Hamano
2021-09-23 23:28 ` Ramsay Jones
2021-09-24 1:16 ` Ævar Arnfjörð Bjarmason
2021-09-24 16:38 ` Ramsay Jones
2021-09-24 1:30 ` Ævar Arnfjörð Bjarmason
2021-09-24 19:37 ` Junio C Hamano
2021-09-28 1:15 ` [PATCH v3] Makefile: add a non-.PHONY "sparse-incr" target Ævar Arnfjörð Bjarmason
2021-09-28 1:43 ` [PATCH v4] " Ævar Arnfjörð Bjarmason
2021-09-28 17:44 ` Junio C Hamano
2021-09-28 19:45 ` Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YUttpgkU6eCOxMj/@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=liu.denton@gmail.com \
--cc=ramsay@ramsayjones.plus.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).