git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Fabian Stelzer <fs@gigacodes.de>, Git List <git@vger.kernel.org>,
	Jeff King <peff@peff.net>, Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test
Date: Mon, 13 Dec 2021 17:11:18 +0100	[thread overview]
Message-ID: <211213.86tufc8oop.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <CAPig+cSXHBMgOUycL0cXuVCb_PJ2=x2w4wUkc7eQQueyk=0Uzw@mail.gmail.com>


On Mon, Dec 13 2021, Eric Sunshine wrote:

> On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> Personally i find the initial for loop variant to be the most readable.
>> Ævars makefile targets could be very nice too, but especially:
>>
>> +$(BUILT_CHAINLINTTESTS): | .build/chainlint
>> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %
>> +       $(CHAINLINT) <$< | \
>> +        sed -e '/^# LINT: /d' >$@ && \
>> +       diff -u $(basename $<).expect $@
>>
>> i find very hard to grasp :/
>> I have no idea what is going on here: `<$< |` ?
>
> Ya, that line-noise is an unfortunate combination of shell and
> Makefile gobbledygook. The `$<` is effectively the source file (the
> file being passed into chainlint.sed), and the rest of it is just
> normal shell. `<` is redirection (using the source file `$<` as
> stdin), and `|` is the pipe operator (sending the output of
> chainlint.sed to another `sed`), and capturing it all via shell `>`
> redirection in `$@` which is the Makefile variable for the target
> file.

To add to that;
https://www.gnu.org/software/make/manual/html_node/Rules.html#Rules and
other relevant parts of the GNU make manual are very helpful here.

> Anyhow, although the commit message tries to sell this change as some
> sort of optimization, it's really in preparation for the new chainlint
> which wants to check all tests in all files with a single invocation
> rather than being invoked over and over and over. The self-test files
> also require more preprocessing to work with the new chainlint, so the
> implementation of `check-chainlint` gets rather more complex once the
> end state is reached. I'll think about it a bit, but at the moment,
> I'm still leaning toward this intermediate step as being beneficial to
> reaching the end state. However, my opinion could change since the way
> this is done here was probably influenced by an earlier iteration of
> the new chainlint, but now that the implementation of the new
> chainlint is concrete, it may not be especially important to do it
> this way.

I don't really care about the details of whether it's invoked once or N
times, although I think the N times with proper dependencies tends to
give you better error messages, but maybe you'll be changing it
significantly enough that the current map between chainlint files and
approximately what sort of thing they check won't be there anymore.

In any case, I'd think that a rule that used $< now (i.e. 1=1 file->out
prereq) would be better for the current state, and could just be changed
to use one of $^ or $+ later.

I.e. you can declare a "check.done" that depends on {1..10}.todo, and
get a list of all of those {1..10}.todo files if one changes, or just
the ones whose mtime is newer than a "check.done".

The reason I looked at this to begin with is that it takes it ~100-150ms
to run now, which adds up if you're e.g. using "make test T=<glob>" in
"git rebase -i --exec".

  reply	other threads:[~2021-12-13 16:17 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-13  6:30 [PATCH 00/15] generalize chainlint self-tests Eric Sunshine
2021-12-13  6:30 ` [PATCH 01/15] t/chainlint/*.test: don't use invalid shell syntax Eric Sunshine
2021-12-13  6:30 ` [PATCH 02/15] t/chainlint/*.test: fix invalid test cases due to mixing quote types Eric Sunshine
2021-12-13  6:30 ` [PATCH 03/15] t/chainlint/*.test: generalize self-test commentary Eric Sunshine
2021-12-13  6:30 ` [PATCH 04/15] t/chainlint/one-liner: avoid overly intimate chainlint.sed knowledge Eric Sunshine
2021-12-13  6:30 ` [PATCH 05/15] t/Makefile: optimize chainlint self-test Eric Sunshine
2021-12-13 10:09   ` [RFC PATCH] t/Makefile: use dependency graph for "check-chainlint" Ævar Arnfjörð Bjarmason
2021-12-14  7:44     ` Eric Sunshine
2021-12-14 12:34       ` Ævar Arnfjörð Bjarmason
2021-12-13 10:22   ` [PATCH 05/15] t/Makefile: optimize chainlint self-test Fabian Stelzer
2021-12-13 14:27     ` Eric Sunshine
2021-12-13 15:43       ` Fabian Stelzer
2021-12-13 16:02         ` Eric Sunshine
2021-12-13 16:11           ` Ævar Arnfjörð Bjarmason [this message]
2021-12-13 17:05             ` Eric Sunshine
2021-12-13 17:25               ` Eric Sunshine
2021-12-13 19:33                 ` Ævar Arnfjörð Bjarmason
2021-12-13 21:37                   ` Eric Sunshine
2021-12-13 16:14           ` Fabian Stelzer
2021-12-16 13:17         ` Ævar Arnfjörð Bjarmason
2021-12-16 15:47           ` Eric Sunshine
2021-12-16 19:26             ` Ævar Arnfjörð Bjarmason
2021-12-13  6:30 ` [PATCH 06/15] chainlint.sed: improve ?!AMP?! placement accuracy Eric Sunshine
2021-12-13  6:30 ` [PATCH 07/15] chainlint.sed: improve ?!SEMI?! " Eric Sunshine
2021-12-13  6:30 ` [PATCH 08/15] chainlint.sed: tolerate harmless ";" at end of last line in block Eric Sunshine
2021-12-13  6:30 ` [PATCH 09/15] chainlint.sed: drop unnecessary distinction between ?!AMP?! and ?!SEMI?! Eric Sunshine
2021-12-13  6:30 ` [PATCH 10/15] chainlint.sed: drop subshell-closing ">" annotation Eric Sunshine
2021-12-13  6:30 ` [PATCH 11/15] chainlint.sed: make here-doc "<<-" operator recognition more POSIX-like Eric Sunshine
2021-12-13  6:30 ` [PATCH 12/15] chainlint.sed: don't mistake `<< word` in string as here-doc operator Eric Sunshine
2021-12-13  6:30 ` [PATCH 13/15] chainlint.sed: stop throwing away here-doc tags Eric Sunshine
2021-12-13  6:30 ` [PATCH 14/15] chainlint.sed: swallow comments consistently Eric Sunshine
2021-12-13  6:30 ` [PATCH 15/15] chainlint.sed: stop splitting "(..." into separate lines "(" and "..." Eric Sunshine
2021-12-15  0:00 ` [PATCH 00/15] generalize chainlint self-tests Elijah Newren
2021-12-15  3:15   ` Eric Sunshine

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=211213.86tufc8oop.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=fs@gigacodes.de \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.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.
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 NNTP newsgroup(s).