ci: EXPERIMENT — what a windows-latest job needs (reference, do not merge) - #1
Open
JamBalaya56562 wants to merge 2 commits into
Open
ci: EXPERIMENT — what a windows-latest job needs (reference, do not merge)#1JamBalaya56562 wants to merge 2 commits into
JamBalaya56562 wants to merge 2 commits into
Conversation
Reviewer's GuideMakes the shell completion and example tests robust on Windows by normalising paths, respecting USAGE_SHELL_* overrides consistently, probing shells via real scripts instead of simple version checks, and adds a Windows CI job that builds and runs the full Rust test suite. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="cli/tests/examples.rs" line_range="10-14" />
<code_context>
-
- let mut cmd = Command::new("../examples/test-empty-defaults.sh");
- cmd.env("PATH", env::join_paths(path).unwrap());
+ // Through `usage bash`, like every other test here, rather than spawning the script and
+ // letting its `#!/usr/bin/env -S usage bash` shebang do the work. Windows cannot execute a
+ // `.sh` at all — the spawn fails with os error 193 before the shebang is ever read — and
+ // going through the binary also drops the PATH juggling that let the shebang find `usage`.
+ let mut cmd = Command::new(cargo::cargo_bin!("usage"));
+ cmd.args(["bash", "../examples/test-empty-defaults.sh"]);
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that specifically validates the example’s documented behavior, not just that it succeeds
The test currently only asserts `success()`, even though the example is meant to show the behavior difference between `default=""` and no default. Please add an assertion on the output (e.g., expected substring or pattern) that verifies this behavior, so changes to default semantics can’t slip through as long as the script exits with status 0.
Suggested implementation:
```rust
assert!(
output.status.success(),
"examples/test-empty-defaults.sh should exit successfully"
);
let stdout = String::from_utf8_lossy(&output.stdout);
// Assert that the example actually demonstrates the documented behavior difference
// between `default=""` and no default. These expected substrings should match what
// the script prints for each case.
assert!(
stdout.contains("...expected output when default is empty..."),
"stdout did not contain the expected marker for the `default=\"\"` case:\n{}",
stdout
);
assert!(
stdout.contains("...expected output when there is no default..."),
"stdout did not contain the expected marker for the no-default case:\n{}",
stdout
);
```
You will need to:
1. Replace the placeholder strings `"…expected output when default is empty…"` and `"…expected output when there is no default…"` with concrete substrings or regex markers that match the documented output of `examples/test-empty-defaults.sh` (for example, lines showing how the resolved values differ when a default is an empty string vs not provided).
2. If the script prints the relevant information to stderr instead of stdout, change the assertion to inspect `output.stderr` instead.
3. Optionally, if the example is documented in README or usage docs, align the expected substrings with that documentation so the test directly validates the documented behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+10
to
+14
| // Through `usage bash`, like every other test here, rather than spawning the script and | ||
| // letting its `#!/usr/bin/env -S usage bash` shebang do the work. Windows cannot execute a | ||
| // `.sh` at all — the spawn fails with os error 193 before the shebang is ever read — and | ||
| // going through the binary also drops the PATH juggling that let the shebang find `usage`. | ||
| let mut cmd = Command::new(cargo::cargo_bin!("usage")); |
There was a problem hiding this comment.
suggestion (testing): Add an assertion that specifically validates the example’s documented behavior, not just that it succeeds
The test currently only asserts success(), even though the example is meant to show the behavior difference between default="" and no default. Please add an assertion on the output (e.g., expected substring or pattern) that verifies this behavior, so changes to default semantics can’t slip through as long as the script exits with status 0.
Suggested implementation:
assert!(
output.status.success(),
"examples/test-empty-defaults.sh should exit successfully"
);
let stdout = String::from_utf8_lossy(&output.stdout);
// Assert that the example actually demonstrates the documented behavior difference
// between `default=""` and no default. These expected substrings should match what
// the script prints for each case.
assert!(
stdout.contains("...expected output when default is empty..."),
"stdout did not contain the expected marker for the `default=\"\"` case:\n{}",
stdout
);
assert!(
stdout.contains("...expected output when there is no default..."),
"stdout did not contain the expected marker for the no-default case:\n{}",
stdout
);You will need to:
- Replace the placeholder strings
"…expected output when default is empty…"and"…expected output when there is no default…"with concrete substrings or regex markers that match the documented output ofexamples/test-empty-defaults.sh(for example, lines showing how the resolved values differ when a default is an empty string vs not provided). - If the script prints the relevant information to stderr instead of stdout, change the assertion to inspect
output.stderrinstead. - Optionally, if the example is documented in README or usage docs, align the expected substrings with that documentation so the test directly validates the documented behavior.
JamBalaya56562
force-pushed
the
windows-ci-experiment
branch
3 times, most recently
from
August 2, 2026 05:57
5a4d8de to
cd53c54
Compare
`cargo test --all --all-features` on a Windows machine with WSL gives 15
failures, and `cli/tests/shell_override.rs` never runs at all. None of it is a
bug in usage; the tests are.
`bash` there is the WSL launcher. The Win32 executable search order puts the
system directory ahead of PATH and installing WSL puts `bash.exe` in it, so a
bare `bash` cannot open the Windows paths these tests write. GitHub's
windows-latest is the same: `where bash` lists Git Bash first, but
`Command::new("bash")` still reaches System32, and without an override it exits
1 with "Windows Subsystem for Linux has no installed distributions".
Both skip guards missed this by probing a proxy rather than the precondition:
`--version` spawns fine under WSL bash, and in `complete_word.rs` the probe
asked about `sh` while the mount fixtures need `bash` — on Windows the two need
not be the same family. Both now run a script, and a real fixture, and require
it to exit cleanly, so an unusable shell skips instead of failing. The fixture
is named from `CARGO_MANIFEST_DIR` rather than `fs::canonicalize`, whose `\?\`
prefix usage cannot open: probing with one made the guard fail on the shape of
the path rather than on the shell, skipping every mount test on a machine where
they run.
Shell resolution goes through `USAGE_SHELL_<SHELL>`, the variable
`shell_program_override` already reads, so one setting covers the whole suite.
Every invocation is built by one `script_command`, so the guard and the tests it
guards cannot drift apart — not in which program they run, nor in how. pwsh is
where they would have: it needs `-File` to take a script path, and `-NoProfile
-NonInteractive` so a developer's profile cannot add output to a run whose
stdout is compared against a single token. The zsh pty test's `timeout` wrapper
goes through the shell too, since spawning it directly walks into the same trap
as `bash` — Windows keeps an unrelated `timeout.exe`, a sleep rather than a
watchdog, in the system directory.
Three places hand a Windows path to something that parses it, and each needed a
different answer. Paths interpolated into shell script bodies are normalized to
`/`, because `\` is an escape character there and `C:\Users\Jam` was arriving as
`C:UsersJam`. `$PATH` entries go through `cygpath` instead: `$PATH` is
colon-separated, so `C:/Users/...` splits into `C` and `/Users/...` and neither
resolves, and only the shell in use knows whether the answer is `/c/...` or
`/cygdrive/c/...`. And `sdk_compile.rs` put the SDK directory inside a Python
string literal, where `C:\Users\RUNNER~1\...` fails to parse at all because
`\U` opens a unicode escape — that one now travels as argv, clear of any
escaping rules.
`test_empty_defaults_example` now runs through `usage bash` like the nine other
tests in its file. It was the only one relying on the shebang, and Windows
cannot execute a `.sh` at all — os error 193, before the shebang is read.
`shell_override.rs` loses `#![cfg(unix)]`. Its substitute program is `$CARGO`
rather than `/bin/echo`, which has no Windows counterpart: cargo sets the
variable for the processes it spawns, and handed a path it does not recognize it
repeats it back, which is what makes the substitution observable. The three
fallback tests compare against a baseline run with nothing overridden instead of
asserting the script's output, since whether the default shell works is a
property of the machine, while "same as unset" is what those tests are about.
Tests only; no library or CLI source changes. Measured on a windows-latest
runner: with `USAGE_SHELL_BASH` naming a real bash and `core.autocrlf` off, the
whole suite passes with nothing skipped. Without the override the
shell-dependent tests skip rather than fail. Linux is unchanged.
Not for upstream as-is. Measuring what a Windows job actually needs. Round 1 found two things, both now compensated for here: `bash` on windows-latest is the WSL launcher. The image carries System32's bash.exe with no distribution installed, and the executable search order puts it ahead of Git Bash, so `usage bash` exits 1 with "Windows Subsystem for Linux has no installed distributions". My assumption that a stock runner would resolve to a real bash was wrong. `USAGE_SHELL_BASH` names the one meant, which is what jdx#767 added it for. git defaults to core.autocrlf=true on Windows and the repo has no .gitattributes, so fixtures and expected outputs arrived with CRLF and seven help tests plus two markdown tests failed on an ending nobody can see — the diff printed identical text. Set before checkout, or it is too late. `skip_if_shell_missing` now only refuses to skip under CI on Unix, because the workflow installs zsh and fish for the Linux job alone; on Windows their absence is expected rather than a configuration bug. mise draws the same line. Round 1 confirmed this works: shell_completions_integration passed 19/19 with zsh and fish skipping and pwsh running. `skip_if_posix_shell_missing` in complete_word.rs stays strict on purpose, so an unusable bash is reported rather than skipped quietly. In round 1 it fired, correctly. The job runs `cargo test --no-fail-fast` directly rather than `mise r test`, so one failing binary does not hide the rest, and skips render and lint: render asserts a clean git diff, which line endings make unreliable here, and lint is a property of the source rather than the platform.
JamBalaya56562
force-pushed
the
windows-ci-experiment
branch
from
August 2, 2026 08:34
cd53c54 to
f6087fb
Compare
JamBalaya56562
changed the title
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fork-internal experiment, kept for reference when the upstream Windows CI job is written. Not for upstream.
Findings are written out below rather than left in the run logs, which GitHub expires after 90 days.
Result
windows-latest, after five rounds: 538 passed, 0 failed, 0 skipped. The zero skips matter — every shell-dependent test genuinely ran rather than passing by skipping.What a Windows job needs
All three measured, not assumed:
git config --global core.autocrlf false, in a step beforeactions/checkout. git defaults it totrueon Windows and this repo has no.gitattributes, so fixtures and expected outputs arrive with CRLF. Seven help tests inlib/tests/parse.rsand two incli/tests/markdown.rsfail on it, and the failure is horrible to read — pretty_assertions prints the two sides as identical, because the only difference is invisible.USAGE_SHELL_BASH: C:\Program Files\Git\bin\bash.exe. The runner carries System32'sbash.exe— the WSL launcher, no distribution installed. Without the override,usage bashexits 1 withWindows Subsystem for Linux has no installed distributions.The
CIpanic inskip_if_shell_missingrelaxed on Windows (cfg!(unix) &&). The rule exists because the Linux job installs zsh and fish deliberately; nothing installs them on Windows, so absence is the expected state rather than a configuration bug. mise draws the same line — its Windows jobs install no POSIX shells at all.Also: run
cargo testrather thanmise r testif you want--no-fail-fast, and skiprenderandlinton Windows.renderasserts a cleangit diff, which line endings make unreliable, andlintis a property of the source rather than of the platform.The
where/CreateProcessmeasurementWorth keeping. This is jdx#767's premise, observed directly on a stock runner:
Git Bash is first on
PATH, andCommand::new("bash")still reaches System32 —wheresearchesPATH,CreateProcesssearches the system directory before it.shhas no System32 impostor, which is why it resolves to Git Bash and why probingshsaid nothing useful aboutbash.Rounds
Each round overturned something I had assumed:
examples.rswent green with the override.fs::canonicalizegives a\\?\path thatusagecannot open, so the mount guard failed on the path shape and skipped all eight tests on a machine where they work. Invisible locally, because a skip counts as a pass.sdk_compile.rsstopped putting a Windows path inside a Python string literal.Rounds 4 and 5 produced real fixes, now folded into jdx/usage#771.