From aebc33559c3260d6b1b31ea68d295a8f24e20659 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 5 May 2025 18:57:36 +0000 Subject: [PATCH 1/4] Change `if ...; then false; else true; fi` to `! ...` `! ...` 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 --- justfile | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/justfile b/justfile index c02e4924cbe..ee63a4b6026 100755 --- a/justfile +++ b/justfile @@ -45,16 +45,16 @@ check: cargo check --workspace cargo check --no-default-features --features small # assure compile error occurs - if cargo check --features lean-async 2>/dev/null; then false; else true; fi - if cargo check -p gitoxide-core --all-features 2>/dev/null; then false; else true; fi - if cargo check -p gix-packetline --all-features 2>/dev/null; then false; else true; fi - if cargo check -p gix-transport --all-features 2>/dev/null; then false; else true; fi - if cargo check -p gix-protocol --all-features 2>/dev/null; then false; else true; fi + ! 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 cargo tree -p gix --no-default-features -e normal -i imara-diff 2>&1 | grep warning # warning happens if nothing found, no exit code :/ cargo tree -p gix --no-default-features -e normal -i gix-submodule 2>&1 | grep warning cargo tree -p gix --no-default-features -e normal -i gix-pathspec 2>&1 | grep warning cargo tree -p gix --no-default-features -e normal -i gix-filter 2>&1 | grep warning - if cargo tree -p gix --no-default-features -i gix-credentials 2>/dev/null; then false; else true; fi + ! cargo tree -p gix --no-default-features -i gix-credentials 2>/dev/null cargo check --no-default-features --features lean cargo check --no-default-features --features lean-async cargo check --no-default-features --features max From ce071bf15f4b1f4380e6b6e42d26a263091113a2 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 5 May 2025 21:00:43 +0000 Subject: [PATCH 2/4] Clarify commands where we grep for a warning 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.) --- justfile | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/justfile b/justfile index ee63a4b6026..e71b3b9e5a4 100755 --- a/justfile +++ b/justfile @@ -50,10 +50,11 @@ check: ! 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 - cargo tree -p gix --no-default-features -e normal -i imara-diff 2>&1 | grep warning # warning happens if nothing found, no exit code :/ - cargo tree -p gix --no-default-features -e normal -i gix-submodule 2>&1 | grep warning - cargo tree -p gix --no-default-features -e normal -i gix-pathspec 2>&1 | grep warning - cargo tree -p gix --no-default-features -e normal -i gix-filter 2>&1 | grep warning + # warning happens if nothing found, no exit code :/ + cargo tree -p gix --no-default-features -e normal -i imara-diff 2>&1 | grep -F warning + cargo tree -p gix --no-default-features -e normal -i gix-submodule 2>&1 | grep -F warning + cargo tree -p gix --no-default-features -e normal -i gix-pathspec 2>&1 | grep -F warning + cargo tree -p gix --no-default-features -e normal -i gix-filter 2>&1 | grep -F warning ! cargo tree -p gix --no-default-features -i gix-credentials 2>/dev/null cargo check --no-default-features --features lean cargo check --no-default-features --features lean-async From b8147f41b79c9199ff975422144f628c2d3f59bd Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 5 May 2025 21:36:42 +0000 Subject: [PATCH 3/4] Harden checks that unwanted dependencies are absent 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. --- justfile | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/justfile b/justfile index e71b3b9e5a4..a5020613e31 100755 --- a/justfile +++ b/justfile @@ -51,10 +51,14 @@ check: ! cargo check -p gix-transport --all-features 2>/dev/null ! cargo check -p gix-protocol --all-features 2>/dev/null # warning happens if nothing found, no exit code :/ - cargo tree -p gix --no-default-features -e normal -i imara-diff 2>&1 | grep -F warning - cargo tree -p gix --no-default-features -e normal -i gix-submodule 2>&1 | grep -F warning - cargo tree -p gix --no-default-features -e normal -i gix-pathspec 2>&1 | grep -F warning - cargo tree -p gix --no-default-features -e normal -i gix-filter 2>&1 | grep -F warning + cargo tree -p gix --no-default-features -e normal -i imara-diff \ + 2>&1 >/dev/null | grep '^warning: nothing to print\>' + cargo tree -p gix --no-default-features -e normal -i gix-submodule \ + 2>&1 >/dev/null | grep '^warning: nothing to print\>' + cargo tree -p gix --no-default-features -e normal -i gix-pathspec \ + 2>&1 >/dev/null | grep '^warning: nothing to print\>' + cargo tree -p gix --no-default-features -e normal -i gix-filter \ + 2>&1 >/dev/null | grep '^warning: nothing to print\>' ! cargo tree -p gix --no-default-features -i gix-credentials 2>/dev/null cargo check --no-default-features --features lean cargo check --no-default-features --features lean-async From 0a43f569baee1c33121527942a9964f99ff7d015 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 5 May 2025 23:06:23 +0000 Subject: [PATCH 4/4] Suppress color in `cargo tree` warnings for simpler grepping 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.) --- justfile | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/justfile b/justfile index a5020613e31..57944710593 100755 --- a/justfile +++ b/justfile @@ -51,13 +51,13 @@ check: ! cargo check -p gix-transport --all-features 2>/dev/null ! cargo check -p gix-protocol --all-features 2>/dev/null # warning happens if nothing found, no exit code :/ - cargo tree -p gix --no-default-features -e normal -i imara-diff \ + cargo --color=never tree -p gix --no-default-features -e normal -i imara-diff \ 2>&1 >/dev/null | grep '^warning: nothing to print\>' - cargo tree -p gix --no-default-features -e normal -i gix-submodule \ + cargo --color=never tree -p gix --no-default-features -e normal -i gix-submodule \ 2>&1 >/dev/null | grep '^warning: nothing to print\>' - cargo tree -p gix --no-default-features -e normal -i gix-pathspec \ + cargo --color=never tree -p gix --no-default-features -e normal -i gix-pathspec \ 2>&1 >/dev/null | grep '^warning: nothing to print\>' - cargo tree -p gix --no-default-features -e normal -i gix-filter \ + cargo --color=never tree -p gix --no-default-features -e normal -i gix-filter \ 2>&1 >/dev/null | grep '^warning: nothing to print\>' ! cargo tree -p gix --no-default-features -i gix-credentials 2>/dev/null cargo check --no-default-features --features lean