On Mon, Dec 13 2021, Eric Sunshine wrote: > On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer <fs@xxxxxxxxxxxx> 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".
