Add a Windows CI job covering lint, compile, and test (#518) - #562
Conversation
Reviewer's GuideAdds a non-blocking Windows CI job that mirrors the Linux build-test pipeline, provisioning Windows-specific tooling (GNU Make, Ninja, cargo-nextest, Whitaker) and running format, lint, and test targets under the pinned Polonius nightly toolchain. Flow diagram for the new Windows CI build-test jobflowchart TD
subgraph build-test-windows
A[actions/checkout] --> B[Install_GNU_Make]
B --> C[Setup_Rust]
C --> D[Install_Ninja]
D --> E[Install_cargo-nextest]
E --> F[Show_rustc_version]
F --> G[Show_Ninja_version]
G --> H[Format: make check-fmt]
H --> I[Lint_Clippy: make lint-clippy]
I --> J[Cache_Whitaker_installer]
J --> K[Install_Whitaker]
K --> L[Lint_Whitaker: make lint-whitaker]
L --> M[Test: make test]
end
classDef nonblocking stroke-dasharray: 3 3
class K,L build-test-windows,nonblocking
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
WalkthroughSummaryAdd a Windows CI merge gate. Align platform-specific code and tests with Windows builds. Canonicalise configuration paths and remove duplicate project layers. Strengthen workflow, coverage, packaging, and toolchain contracts. ChangesWindows CI and cross-platform support
Sequence Diagram(s)sequenceDiagram
participant WindowsRunner
participant RustSetup
participant Make
participant TestSuite
WindowsRunner->>RustSetup: Install pinned Rust toolchain with warnings and Polonius flags
WindowsRunner->>Make: Run check-fmt, lint, and test through Git Bash
Make->>TestSuite: Compile and execute Windows-specific tests
TestSuite-->>WindowsRunner: Return blocking CI status
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat o11y, code safety, documentation and validation coverage as in scope). #518 Assess and record whether #[cfg(any(windows, test))] in src/stdlib/which/env.rs can revert to #[cfg(windows)], updating code or documentation accordingly. ❌ The diff only adds the Windows CI job; it does not modify src/stdlib/which/env.rs or any documentation/ADR to record a concrete decision about reverting #[cfg(any(windows, test))] to #[cfg(windows)]. The PR body mentions that this can be reassessed, but does not actually document or implement a conclusion. |
This comment was marked as resolved.
This comment was marked as resolved.
Remove continue-on-error from build-test-windows and its lint and test steps now that the cfg(windows) tree is green under -D warnings. Whitaker installs and runs on windows-latest (verified in #562), so its install and lint steps become blocking too. Update the developer guide to state that the job is a merge gate.
Inspect the YAML hook structure by lines so the workflow contract accepts both LF and CRLF input while retaining the requirement that the fallback runs inside the build-scoped pre-hook.
Reject metadata lookups whose path has a regular-file ancestor. Windows otherwise aliases such a lookup to the ancestor, violating the test-support filesystem helper's contract to surface non-NotFound errors.
Build the fixture's expected manifest path with the same UTF-8 path joining logic as the manifest helper. This preserves the assertion on Windows, whose native paths retain a backslash separator.
Construct the triple-fallback expectation one component at a time, matching the locator. This preserves the test's complete-diagnostics contract on native Windows paths.
- Route test-support canonicalization through a Camino return type and move it into its own module so `fs` stays within the Whitaker `module_max_lines` cap, while keeping the ambient `std::fs` call behind the crate documented ambient boundary (cap-scoped `Dir::canonicalize` returns relative paths and cannot reproduce an absolute tempdir path). - Replace the remaining `std::fs` fixture writes in the Unix ninja snapshot tests with the shared `test_support::fs` helper, leaving `std::process::Command` untouched. - Scope the GoReleaser hook contract to the fallback build by parsing `.goreleaser.yaml` structurally: assert the `pre` hook that branches on GOOS/GOARCH belongs to the `netsuke` build, require it to be the only build-level `pre` hook, and keep the no-global-`before` guard. Add a regression test proving an unrelated build-level hook is rejected. - Exclude the local `.vtcode` tooling scratch directory from the Markdown file find so `make markdownlint` does not scan ignored tooling output.
Replace the Windows-only path-spelling assertion with a test that creates a current and a stale sidecar, acquires the publication lease, and invokes `prune_dyndep_sidecars` before asserting the current bundle survives and the stale candidate is removed through native Windows path resolution. The previous test compared two constructed paths and never called the pruning logic, so it could not catch the path-identity bug fixed by 57ae88f.
Retain the post-rebase discovery seams in the generated path test and assert only the layer contract where trace events are intentionally deferred. Require removed Windows sidecars to report `NotFound`.
Keep discovery, canonical comparison, and the project-scope second pass as separate responsibilities. The extracted private helper preserves the trace, de-duplication, telemetry, and layer-ordering contracts.
Explain the ambient canonicalization boundary, UTF-8 and native Windows path identity rules, and the `TempPath` writer-lifetime contract. Record the named regression test that protects temporary Ninja file reuse.
Tell test authors to use `test_support::fs::canonicalize` when comparing native path identity, including Windows short-name and long-name spellings.
Correct the documented `handwritten` spelling without changing the canonicalization guidance or any other documentation.
Exercise dot, symlink, and non-UTF-8 resolved fixture paths so `canonicalize` cannot become a no-op. Pass the generated LCOV report to the main CodeScene upload and pin the report's production and upload ordering in the workflow contract.
Correct the two requested `-ize` spellings without changing the surrounding fixture and temporary Ninja lifecycle guidance.
Emit a project-scope trace only after the second discovery pass loads at least one layer. This prevents an absent project configuration from being misreported as a deduplicated layer while retaining the existing positive deduplication diagnostic.
Record that bounded layer counts are retained for replay through `DiscoveryOutcome::emit_diagnostics` rather than emitted during collection.
Retain bounded project-layer counts until discovery diagnostics replay. This keeps collection side-effect free while preserving the branch diagnostics without an additional environment read. Cover the missing-fixture canonicalization error contract.
Keep the CI contract documentation focused on maintained contributor guidance rather than review metadata.
Compare captured stdlib paths within the Camino path model used by the BDD workspace fixtures.
Distinguish the historical milestone record from the later canonical comparison, de-duplication, and deferred diagnostics implementation.
Restore the BDD step imports while retaining the Unix-only gates, and format the merged stderr-routing parameterization.
Share the fixture setup across canonicalization regressions and validate job environments before enforcing the workflow-scoped nextest pin.
Accept the Windows raw exit-status type in the test helper so the platform extension receives it without a potentially lossy signed cast.
Describe normalizer-backed layer de-duplication and deferred diagnostics at the current collection boundary, and remove stale follow-up references from the historical ExecPlan.
Build expected command arguments with the same path canonicalizer as production so equivalent Windows path spellings compare consistently.
Retain the normalizer-aware discovery boundary and make literal fallback comparison preserve the deferred de-duplication trace. Split replay assertions into their own module to keep the file-layer tests within the enforced module-size limit. Repair the affected developer docs.
b5ff390 to
0b8d476
Compare
Closes #518
Summary
Netsuke ships Windows binaries that no CI job ever compiles. 47
#[cfg(windows)]sites across 14 files were never linted, type-checked,or tested, and reached users compiled for the first time at packaging
time — the worst place to discover a failure. This pull request adds a
build-test-windowsjob to.github/workflows/ci.ymlthat mirrors theLinux
build-testjob onwindows-latest, restricted to what isplatform-relevant.
What the job runs
make check-fmtmake lint-clippy(Clippy andcargo docunder-D warnings)make lint-whitaker(Whitaker installs and runs on Windows)make test(cargo-nextest + doctests under-D warnings -Zpolonius=next)What is excluded (platform-independent, already covered on Linux)
make spelling,make markdownlint,make nixiemake test-workflow-contractsTooling provisioned for Windows
choco install make)seanmiddleditch/gha-setup-ninjacargo-nextestviataiki-e/install-action, pinned toNEXTEST_VERSIONdefaults.run.shell: bash), with everymake invocation overriding
SHELLto bash because GNU Make's Windowsdefault recipe shell is cmd.exe
-D warnings -Zpolonius=nextpassed through theshared
setup-rustwith.rustflagsinput, per the Polonius toolchaincontract (no job-level
env.RUSTFLAGS)whitaker-installershipswhitakeras a PowerShell wrapper onWindows; a bash shim in the cargo bin directory invokes it through
PowerShell so
make lint-whitakercan run it from Git BashRollout posture
The job is a blocking merge gate: no
continue-on-errorremains onthe job or any of its steps, so a Windows failure or warning blocks the
merge. Making it blocking surfaced the never-compiled
#[cfg(windows)]surface under
-D warnings; the findings were cleared at the source:test_supportand theWindows-only test arms
missing_const_for_fn,unnecessary_wraps,needless_pass_by_value, shadowing, format-arginlining, unused imports)
no_std_fs_operationsfindings in the Windows grep-streamtest, routed through
test_support::fsruns instead of failing on a missing command
Remaining Windows failures (blocking the merge)
The
Teststep currently fails on threecli::discoverytests onwindows-latest:cli::discovery::layer_tests::normalization_failure_does_not_fail_discoverycli::discovery::layer_tests::existing_project_scope_layer_is_not_appended_twicecli::discovery::tests::collect_diag_file_layers_uses_injected_explicit_configThese are pre-existing Windows path-identity bugs in
src/cli/discovery*,unrelated to the CI job change and out of this PR's scope. Root cause:
tempdir()returns short-name paths (C:\Users\RUNNER~1\...) onWindows while
ortho_configcanonicalises layer paths to long names(
C:\Users\runneradmin\...), so the project-scope dedup key nevermatches the recorded layer path and the layer is appended twice. They are
tracked for a follow-up; the job correctly blocks until they are fixed.
Known unknowns resolved during implementation
choco install makeplus GitBash with
SHELL=bashoverrides.gha-setup-ninjaand aninja --versionassertion step.
on
windows-latest; the PowerShell wrapper is shimmed for Git Bash.make powershell-wrapper-validate: the target does not exist in thecurrent Makefile, so it is not reachable and not added to this job.
cfg widening assessment (env.rs) — decision: keep the widening
DEFAULT_PATHEXT,default_pathext, andparse_pathextinsrc/stdlib/which/env.rsare gated#[cfg(any(windows, test))]so theUnix CI host could reach them (see #503). With a Windows job that
compiles and tests the
#[cfg(windows)]arm directly, the widening wasreassessed:
gone.
#[cfg(windows)]would drop Unix-host coverage ofparse_pathext's pure string logic (normalization, de-duplication,fallback), which
src/stdlib/which/pathext_tests.rspins on everyhost. There is no equivalent Unix-side test for a Windows-only
function.
both Linux and Windows, and a Windows-gated regression cannot hide
from the Unix suite. Recorded in
docs/developers-guide.md.References
#503,
#493
Summary by Sourcery
Add a blocking Windows CI gate and harden platform-specific behavior so Windows builds, linting, and tests are validated before merge.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: