Make the justfile more portable and slightly more resilient #1904
Conversation
This file contains 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
This makes some recipes work on Windows, and on other systems if there are wunusual paths, that had previously broken in those scenarios. That includes the default recipe.
This is easier to read and verify for correctness, and also more robust.
This makes two small changes to the style in which quoting is expressed in the `justfile`: - Outside of commands run by a shell (i.e. when `just` interprets the quotes), prefer single quotes over double quotes when there is both no intent for `\`-escape interpretation to occur and no other reason to use `"` (such as a literal `'`). - Inside commands run by a shel (i.e. when the quoting is shell syntax rather than `just` syntax), omit double quotes around a few literal arguments that contain no `$` nor other characters the shell treats specially. They are alredy omitted in most commands similar to these.
Most recipes in `justfile` that use `cargo nextest run` write it out in a command, even though a `nextest` recipe exists. This is often done because the command appears in a non-leading line of the recipe, so having the recipe depend on the `nextest` recipe would not be feasible. However, it also has the benefit of being more clear about exactly what command is being run, especially when the arguments are long and complicated, as in `summarize`. This changes `summarize` so that it invokes nextest explicitly as a step of the recipe, rather than depending on `nextest` and passing its arguments through.
This changes paths that are excuted by a shell in a `justfile` to omit the leading `./` when the path already has a `/` later. A path like `./a` is needed to run `a` from the current directory, but `./a/b` is just a longer way to express `a/b`, since `a/b` already has a `/` in it. It is the presence of a `/`, rather than the presence of a leading `./` specifically, that causes paths to be looked up relative to the current directory rather than by a `PATH` search. The reason to make this change is that a few of the commands that use those paths are about to get a bit more complicated. It is hoped that removing this small amount of noise, though currently inconsequential, will allow the immediately forthcoming change to be made while preserving readability.
- Have the `justfile` wait to run `cargo metadata` until it is running a recipe that actually needs the information from it. (Currently, that information is always the `target` directory location, in whose `debug` subdirectory some binaries are found.) This avoids a delay if `cargo metadata` has not run. The length of the delay varied but was often noticeable on Windows. Because running it again (in the absence of a clean or relevant change) is cheap, it is not a performance problem that this runs the command multiple times instead of once. This also avoids error messages from `cargo metadata` if it can't complete, unless it is actually being used. Those messages didn't prevent other recipes (besides those that used the metadata) from running. But they created the appearance of failure, and also were misleading when they didn't come from the recipe being run. - Handle errors from `cargo metadata` more robustly, by causing recipes that actually need the result of `cargo metadata` to fail if it fail -- and to fail immediately before attempting any other operations -- rather than attempting to use empty data. The metadata are piped to `jq -r .target_directory`, which actually succeeds even if it doesn't receive any data, giving empty output. Before, an attempt was made to use the empty output in building paths meant to go to debug builds of some binaries. Because the command is now running only when the output is actually needed, it is fine to make it hard error when it fails. The natural way to do this would be to `set -o pipefail`. But while it is in the newest POSIX standard, there remain otherwise largely POSIX-compatible `sh` implementations in use that don't support it (koalaman/shellcheck#2555). Our test suite requires `bash`, so the next obvious choice would be to use a shebang recipe or script recipe for `bash`. The problem is that shebang and script recipes don't usually work on Windows, because `just` passes a Windows path with `\` separators to the shell unquoted. This often results in the shell trying to run a file whose name is transformed by treating them specially, then causing them to dropped as if by quote removal. For example, on a recipe named `hello` with a `#!/usr/bin/env bash` shebang: /bin/bash: C:UsersekAppDataLocalTempjust-BVbCgphello: No such file or directory (One way this happens is by argument processing in `cygwin1.dll` or `msys-2.0.dll`, which seek to bridge the gap between the Unix expectation that the caller is responsible for all expansions and the Windows expectation that the callee is responsible for some. See rust-lang/rust#82227. Even when only globbing is attempted, `\` can be treated specially if it seems to escape a wildcard, or to escape another `\` that seems to escape a wildcard, etc.) Fortunately, in this case we can just check if the result of trying to parse out the `target` directory path gave a nonempty result, and treat the failure to do that as a hard error. - Remove the `check-mode` recipe's need for cargo metadata, by having it run the `internal-tools` binary via `cargo run` rather than looking up where the `target` directory is and finding `it` in `debug` under it. That approach is needed for the paths that are passed to the journey test runner, but `check-mode` can just use `cargo`. The preceding build command could be removed, since `cargo run` builds unless the binary is up to date. But keeping them as separate commands may make the output more readable.
One of the `justfile` recipes ran `just --fmt --unstable`, and did so as such. The problem with this is that the `just` executable in the outer `just` call is not guaranteed to be the same `just` that the inner call in the recipe finds. For example: - In general, a user might use a `just` that is not in `$PATH` even when another `just` is in `$PATH`. This would happen when testing changes to `just`, and potentially in other use cases. - The "inner" `just` is looked up by a POSIX-compatible shell that, on Windows, may have a changed environment. For example, the `sh.exe` shim provided by Git for Windows modifies environment variables including placing some directories in the front of `$PATH`. (See discussion in GitoxideLabs#1864 for details.) Thus `just` provides a built-in function `just_executable()` that can be used, and which we already use in the default recipe. This uses that function instead. However, it is not as simple as `{{ just_executable() }} --fmt --unstable`, because the path may require quoting to be run in a shell. In practice, it almost always needs to be quoted on Windows, where otherwise a `\` is being given to the shell (which the shell is required to interpret as a quoting character; this is distinct from a scenario where a path might be passed as an argument to a shell, in which case strange things may or may not occur). It might rarely need to be quoted on other systems too, it it has spaces or other even weirder contents. So this uses `{{ quote(just_executable()) }}`. It does so through the `j` variable that has already been assigned that value.
- Make the style of `justfile` recipe descriptions more consistent - Lightly adjust wording and punctuaiton for clarity - Clarify some to avert misreadings even if a reader is inattentive - Add descriptions for the several recipes that didn't have them
Byron
approved these changes
Mar 22, 2025
There was a problem hiding this comment.
Thanks a lot - this is great and much appreciated.
Indeed, I only gave it a quick look and generally only want these things to work, without maintaining them ideally π. So if you feel there is a delay or it's really nothing you'd think I could object to, then please do feel free to merge these kind of PRs yourself.
(I will always look at them anyway and will stay in the loop that way, should there be something to change, it can always be a follow-up as well)
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.
See the commit messages (and diff) for full details. Some highlights are described below.
When
cargo metadatafails and it's not a problemBefore these changes:
The
copy-packetline.shscript actually succeeded, because it didn't need thecargo manifestoutput. Butcargo manifestwas called anytimejustwas used to run any recipe, or even just to list the recipes. Besides occasionally making the first invocation take noticeably longer, this is misleading, because it looks like something relevant to the requested operation failed. (See also #1310 (comment).)After these changes:
Making future additions more discoverable
Related to the above, there are various approaches we can use to have Rust code that builds and runs even when the workspace is broken. This is relevant if we decide to introduce Rust code that generates Rust code that, if absent, may cause the workspace to be considered broken. One of the approaches for this, which requires no extra tools, is to have a project that opts out of being in any workspace by having an empty
[workspace]section in itsCargo.toml.Such a project is then less discoverable. But it can be made discoverable by including commands that use it in the
justfile. That is only feasible if thejustfileavoids eagerly failing when the workspace is broken.That's actually may main motivation for doing this PR: to open up the ability to do things like
copy-packetline.shin Rust, as I fear may be needed for #1886 (depending on what the answer to #1886 (comment) turns out to be).When
cargo metadatafails and it is a problemRecipes that actually use the output of
cargo metadata, such asjourney-tests, showed the same errors twice. In principle, they could in the future come to have been written in a way that would cause them to behave even more strangely, if they did something first that did not usecargo. Now that we only usecargo metadatawhen we need the output, this is made into a hard error, and thus is only run once.It may be that this was originally intended--though then, since it was run eagerly before all recipes, it would have kept anything from working even when failures was not a problem. The reason it was not a hard error before is that the failing command is on the left side of a pipe, where it does not cause the whole pipeline to fail, and the
jqparsing command on the right side of the pipe produced empty output but still reported success even when its input was empty.Quoting
Before these changes, on Windows:
After these changes, on Windows:
Some other quoting improvements are included. For example,
summarizeno longer pastes single quotes around the expression, but instead quotes it robustly.Although this mainly benefits Windows, in principle it can be needed on any platform, such as when a path used in a shell has spaces in one of its components.
Using the same
justimplementationjust_executable()was already used in the default recipe to make sure we run the samejustforjust --listrather than a differentjust. (It was used in a shell without quoting, but that is fixed here as described above.) But one recipe had previously hard-codedjustand used it for formatting. This fixes that.Descriptions
All recipes have
#descriptions that show when the recipes are listed, and they are revised to slightly improve clarity and to eliminate unnecessary stylistic differences. (As shown above.)For some of the changes here, there is more than one way they could be done, and whether this way is clearer or less clear than alternatives is subjective. Accordingly, even though this is kind of thing that, along the lines of the discussion in #1883, I might consider merging myself without a review, in this case I think it's better that I check.