Skip to content

ci: run clippy on Windows so cfg(windows) code is lint-gated too - #488

Open
rominf wants to merge 1 commit into
mainfrom
ci-clippy-on-windows
Open

rominf wants to merge 1 commit into
mainfrom
ci-clippy-on-windows

Conversation

@rominf

@rominf rominf commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator

What was wrong

The clippy job runs on Linux only. It therefore compiles the cfg(unix) side of every platform conditional and never sees the cfg(windows) side.

That is not "Windows code is under-linted" — it is outside the warnings-as-errors gate entirely. A lint could land in Windows-gated code and no check in the set could fail, because no check ever compiled it. windows-build-and-test builds and tests on Windows, so type errors are caught there; lints are not.

What this does

Adds both of the Linux job's clippy invocations to windows-build-and-test, before the build so a lint failure costs seconds rather than the full build/test/package cycle — the same reasoning behind that job's existing needs: clippy.

It goes in the existing job rather than a new one on purpose: windows-build-and-test is already a required status check, so the gate becomes load-bearing immediately with no branch-protection change. That job currently runs ~13 minutes against a 45-minute timeout, so there is headroom.

The 21 violations it surfaced

Most are mechanical and are fixed here:

  • &mut x passed to a Win32 call → &raw mut x (also avoids materialising a reference just to take a pointer from it)
  • if c { 1 } else { 0 } → i32::from(c)
  • map(..).unwrap_or(..) → is_ok_and(..), map(..).unwrap_or_else(..) → map_or_else(..)
  • use_self, uninlined_format_args

The rest are deliberately silenced rather than "fixed", and that is the part worth reviewing.

Eleven are missing_const_for_fn or unused_async on platform stubs. Applying Clippy's suggestion there is wrong in two ways:

  1. Making only the off-unix arm of a cfg-split pair const gives the two platforms different signatures. Two of these are pub, so a const-context call would compile on Windows and fail on Linux.
  2. It cascades. Making process_start_ticks const pushed the same lint onto ProcessIdentity::capture; making probe_usable_amd_gpu_indices const pushed it onto usable_amd_gpu_indices. This is why the burn-down grew on each pass rather than shrinking.

The async stubs are the same shape: callers .await them on both platforms, so dropping async would not compile. Each site carries its reason inline. The one case where const is applied is WindowsExamineInventory::is_empty, which is cfg(windows)-only with no sibling to disagree with.

Verification, and its limit

Cross-compiled the whole workspace to x86_64-pc-windows-gnu with mingw. Both clippy invocations are clean for that target, both are clean for Linux (no regression), and cargo test --workspace --all-targets passes.

The msvc target could not be checked locally: ring and aws-lc-sys need a Windows C toolchain to build their build scripts, which does not exist off Windows. The cfg(windows) code is identical between the two targets, but an msvc-only lint would appear for the first time in this PR's own CI run — which is, conveniently, exactly the gate being added. If one shows up, it is a real finding, not a flake.

Clippy also stops at the first failing crate, so this converged over several passes (e2e-cucumber → rocm-core → rocmd → apps/rocm). The final pass reports zero.

Scope

CI config plus the lint fixes it requires — no behaviour change. No Gherkin scenario: nothing user-observable changes. The &raw mut rewrites are the only edits inside unsafe FFI blocks, and they are the compiler-suggested form for taking a raw pointer without an intervening reference.

The clippy job runs on Linux only, so it compiles the cfg(unix) side of
every platform conditional and never sees the cfg(windows) side. Windows
-gated code was outside the warnings-as-errors gate entirely, not merely
under-linted: a lint could land there and no check in the set could fail.

Add both of the Linux job's clippy invocations to windows-build-and-test,
before the build so a lint failure costs seconds rather than the full
build/test/package cycle. That job is already a required check, so the
gate becomes load-bearing without a branch-protection change.

Turning it on surfaced 21 existing violations, fixed here. Most are
mechanical: `&mut x` passed to a Win32 call becomes `&raw mut x`, which
also avoids materialising a reference just to take a pointer from it;
`if c { 1 } else { 0 }` becomes `i32::from(c)`; `map(..).unwrap_or(..)`
becomes `map_or_else`/`is_ok_and`.

The rest are `missing_const_for_fn` and `unused_async` on platform stubs,
and those are deliberately silenced rather than "fixed". Making only the
off-unix arm of a cfg-split pair `const` would give the two platforms
different signatures — two of these are `pub` — and would push the same
lint onto every caller in turn, which is how the burn-down kept growing.
The `async` stubs must keep their signature because callers `.await` them
either way. Each site carries the reason inline.

Verified by cross-compiling the whole workspace to x86_64-pc-windows-gnu
(the msvc target cannot be checked off Windows: ring and aws-lc-sys need
a Windows C toolchain to build their build scripts). Both clippy
invocations are clean for that target and for Linux, and the test suite
passes.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf
rominf marked this pull request as ready for review October 2, 2026 09:55
@rominf
rominf requested a review from a team as a code owner October 2, 2026 09:55
@rominf
rominf requested a review from tomastola October 2, 2026 09:55

@jussielo-amd jussielo-amd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

Blocking

  1. .github/workflows/ci.yml:590 — The new "Clippy (Windows targets)" step runs two cargo clippy commands in one pwsh script with no error handling. pwsh defaults to $ErrorActionPreference: Continue, and GitHub Actions reads the step's pass/fail from $LASTEXITCODE at the end of the script — which only reflects the last command. So if the first (broader) clippy invocation fails but the second (narrower, e2e-cucumber-only) one passes, the whole step reports green. The entire point of this PR is to lint-gate cfg(windows) code, and this bug means it can silently fail to do that. The same file's "Probe sccache cache backend" step already handles this correctly with explicit $LASTEXITCODE checks, so this isn't a tooling limitation — just a gap in the new step.

Worth raising

  1. ci.yml:590 — Running cargo clippy --workspace --all-targets immediately before cargo build --workspace --all-targets in the same job causes Cargo to recompile the workspace twice (clippy's RUSTC_WORKSPACE_WRAPPER changes the build fingerprint, so build doesn't reuse clippy's artifacts). Risks eating into the job's ~13min/45min timeout headroom the PR itself cites.
  2. ci.yml:590 — This new lint step runs serially inside windows-build-and-test (the repo's slowest job), rather than following the existing convention (see powershell-lint) of a separate parallel job — so it adds to critical-path time instead of overlapping with the Linux clippy job, and a Windows-lint failure surfaces buried inside a build/test job rather than its own check.
  3. Cargo.toml:74 — The workspace already has a [workspace.lints.clippy] allow-list mechanism for exactly this kind of structural false-positive (nursery lints on cfg-split platform stubs), but the PR hand-applies #[allow(...)] with duplicated justification comments to 9+ individual functions instead of one workspace-level policy change. Future platform stubs will keep re-triggering this and require copy-pasting the same idiom.

Minor

  1. crates/rocm-core/src/uv.rs:283 — The justification comment is copy-pasted near-verbatim across 9 files; one of them (make_executable) already describes a structurally different situation using near-identical wording, so drift has already started.
  2. ci.yml:593 — The two clippy invocations run sequentially with no data dependency forcing that.
  3. apps/rocm/src/main.rs:3492 — #[allow(clippy::missing_const_for_fn)] is applied unconditionally to a function that isn't actually cfg-split (just has one non-const branch), unlike every other suppression in this PR — if that branch ever becomes const-compatible, the suppression would stay in place with nothing forcing re-evaluation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants