Make justfile clearer and fix some edge cases in the checks #1994
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`! ...` is simpler, as well as easier to read in `just` output. The `if ...; then false; else true; fi` syntax seems to have been introduced in 72cf940, in a command that, at that time, was in the `Makefile`. It's unclear if there was a reason to prefer it at the time it was introduced. Both a `Makefile` and a `justfile` work fine with `!` as it is used here, in both cases it is preserved literally and passed to the shell, and in both cases the shell is by default `sh` on all systems. It looks like `!` may have been avoided to accommodate behavior of `bsdmake` (which was at one time the default `make` on macOS, and which remains the default `make` on various other systems), where at least in some versions an optimization to run commands without a shell is triggered even in the presence of a leading `!` and even though `!` is a shell keyword. This does happen today if `bsdmake` is installed on macOS via `brew`, but only with *simple* commands, as in a rule like this (if one fixes the indentation, to use tabs): hello: ! false Then: $ bsdmake hello ! false !:No such file or directory *** Error code 1 Whereas, with... hello: if false; then false; else true; fi ...the shell is used and it succeeds: $ bsdmake hello if false; then false; else true; fi However, this does not establish that the practice was needed even when the command was in a `Makefile`, because with... hello: ! false >/dev/null ...the presence of the redirection operator `>` is sufficient to cause a shell to be used, and it succeeds: $ bsdmake hello ! false >/dev/null All commands negated via `if ...; then false; else true; fi` in the history of `gitoxide` in the `Makefile` (as well as after they were moved to the `justfile`) seem to have used `>` redirection, because the point is to verify that a build command that should fail does fail, and it is distracting to show the full compiler output of the intended failure. So it seems `!` negation could have worked even with versions of `bsdmake` where it does not always work, and even when the commands were in the `Makefile`. (Though it may have been avoided in order to make immediately clear to human readers that no `!` misinterpretation would occur.) In any case, this is not in a `Makefile` at all anymore. Using `!` works locally and on CI, is easier to read, and the output is easier to understand, both in general and specifically in how it lines up with the explanatory comment. The output now looks like: # assure compile error occurs ! cargo check --features lean-async 2>/dev/null ! cargo check -p gitoxide-core --all-features 2>/dev/null ! cargo check -p gix-packetline --all-features 2>/dev/null ! cargo check -p gix-transport --all-features 2>/dev/null ! cargo check -p gix-protocol --all-features 2>/dev/null
In this justfile, for the `cargo tree ...| grep warning` commands: - Put the explanatory comment before the first of these commands, rather than at the end of the first one, since this is easier to read in the `just` output, as well as clearer because it applies to a cluster of commands rather than only the first one. This is also more consistent stylistically with other comments in the `justfile`. - Pass `-F` to `grep`, as this is a literal search for the text `warning`. (This is a minor refactoring for clarity, and it may be changed soon if the check is made more precise.)
The `check` recipe in the `justfile` contains several commands to check that, when particular packages are built without default features, this avoids having a particular other crate appear anywhere in the dependency tree (i.e., without default features, we do not depend on a particular named package). These are done by using `cargo tree` with the appropriate arguments to compute the inverse dependency tree from the crate whose absence we wish to assert. But because `cargo tree` treats an inverse tree with no nodes (not even its root) as a mere warning, we cannot check the exit code. So we parse the output for a warning. As implemented before this commit, those checks already had a high probability of giving the correct result. But they could have falsely passed. The technique being used was to redirect stderr to stdout, then search for the text `warning` anywhere in the combined output. This would falsely pass in either of two edge cases: 1. If the inverse dependency tree was produced, but contained a crate with `warning` in its name (or in associated details). 2. If a different warning than the anticipated one was produced, and the anticipated warning was not produced. This commit hardens the check against both those edge cases, by: 1. Discarding what `cargo tree` sends to stdout, rather than including it. (This has the minor disadvantage that the syntax may look wrong, or be less obvious in its effects, to readers less familiar with POSIX shell redirection syntax. The right side of a redirection operator, i.e. the filename to open or the file descriptor to duplicate, is dereferenced. So `2>&1` duplicates the original stdout and sends stderr to it, which works even though the immediately subsequent redirection `>/dev/null`, short for `1>/dev/null`, sends stdout to the null device. Although this is a common source of confusion, the actual semantics are exactly what we want here.) 2. Parsing more narrowly for the warning, ensuring it starts at the beginning of the line and that the warning message begins with the expected phrase. This, in particular, makes the commands significantly longer. But by showing the message, it also helps clarify what they are for. So even aside from keeping the checks from wrongly passing, this part of the change seems to improve clarity overall.
Without this, the `check` recipe in the `justfile` was failing on CI due to the change in the previous commit. On CI, we set `CARGO_TERM_COLOR` to `always`. The `--color` option takes precedence over the `CARGO_TERM_COLOR` environment variable. The suppresion is only done on those specific command whose stderr is being parsed. Other colorization remains allowed, including on CI. (An alternative to using `--color` here could be to make the `grep` pattern tolerate color codes where they may be present. But this would make the pattern considerably longer, more complicated, less less readable, and easier to get wrong.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes some improvements to the
justfile. It might be regarded as a sequel to #1904. See the commit messages for details, particularly aebc335 and b8147f4.