Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Dec 16 2021, Eric Sunshine wrote:

> On Thu, Dec 16, 2021 at 8:22 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> > On 13.12.2021 09:27, Eric Sunshine wrote:
>> >>It's not seen in the patch context, but earlier in the file we have:
>> >>
>> >>    CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))
>> >>
>> >>which provides stability via `sort`, thus ensures that the order of
>> >>the ".test" and ".expect" match.
>> >>
>> >>I think that addresses your concern (unless I misunderstand your observation).
>>
>> But just FWIW I think both of you are wrong about the potenital for a
>> ".test" and ".expect" bug here.
>>
>> I.e. yes the CHAINLINTTESTS variable is sorted:
>>
>> But in Eric's patch we just have this relevant to this concern of
>> (paraphrased) "would it not be sorted break it?":
>>
>>         +       sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
>>         +       cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
>>
>> So it doesn't matter if it's sorted our not.
>>
>> I.e. we've got {A,B,C}.{test,expect} files in a directory, and we're
>> constructing a "A.test" and "A.expect" via "$(patsubst)".
>>
>> So if it's "A B C", "C B A", "A C B" etc. won't matter. We'll always get
>> ".test" files corresponding to ".expect".
>
> Yes, sorry, I meant to say something along these lines in my reply, in
> addition to mentioning `sort`, but forgot. Taking a look at this
> again, though, makes me wonder if the CHAINLINTTESTS assignment should
> be done with `:=` rather than `=` (unless GNU make is smart enough to
> only invoke the `wildcard` operation only once, in which case it
> wouldn't particularly matter).

It appears to be smart enough to do that, i.e. it'll invoke a $(shell)
assignment N times for -jN unless you use simply-expanded variables, but
not for a simple wildcard.

But I really don't think that'll matter, doing that I/O is trivial
compared to other things involved there.

If for some weird reson (e.g. hundreds of thousands of dependency files)
you find yourself trying to optimize make runs on this basis, this is
the wrong way to go about it.

Instead you'd have a rule depending on the contain directory, whose
mtime will be updated should anything there change. You shouldn't need
that trickery for anything the size of the git codebase, but that
approach will beat trying to optimize the O(n) lstat() calls you'll
otherwise be doing on the contents of the directory.