Repository navigation
fix(cargo-anvil): Force Anvil's cargo-check to fail on Warnings - #348
Nikoloz Gamkrelidze (nikagamkrelidze) wants to merge 6 commits into
Conversation
Nikoloz Gamkrelidze (nikagamkrelidze)
added
the
human-review-required
There was a problem hiding this comment.
🟡 Changes recommended
The new test writes mutable fixtures and depends on installed external tools, allowing coverage to be silently skipped and violating the required in-process, immutable-input test strategy.
1 open issue
What changed in this PR
This PR makes Anvil’s package-isolated check-all-targets recipe reject warnings in both default and no-default-feature configurations, catching warnings that all-features Clippy checks can miss.
Changes:
- Adds
build.warnings="deny"without replacing existing compiler flags. - Adds regression coverage and updates generated recipe snapshots.
- Documents the Cargo 1.97 requirement and bumps cargo-anvil to 0.26.0.
| File | Description |
|---|---|
justfiles/anvil/checks/check-all-targets.just |
Applies warning denial to the generated recipe. |
crates/cargo-anvil/tests/snapshots/snapshots__local_only.snap |
Updates the local-backend recipe snapshot. |
crates/cargo-anvil/tests/snapshots/snapshots__github_backend.snap |
Updates the GitHub-backend recipe snapshot. |
crates/cargo-anvil/tests/snapshots/snapshots__ado_backend.snap |
Updates the Azure DevOps-backend recipe snapshot. |
crates/cargo-anvil/tests/recipe_contracts.rs |
Updates command expectations and adds warning-policy coverage. |
crates/cargo-anvil/templates/justfiles/anvil/checks/check-all-targets.just |
Adds warning denial to the source template. |
crates/cargo-anvil/README.md |
Refreshes generated version metadata and API links. |
crates/cargo-anvil/docs/design/README.md |
Documents the check’s minimum Cargo version. |
crates/cargo-anvil/docs/design/checks.md |
Describes warning denial and compiler-flag preservation. |
crates/cargo-anvil/Cargo.toml |
Bumps cargo-anvil to 0.26.0. |
Cargo.lock |
Synchronizes the cargo-anvil version. |
.anvil.lock |
Updates the generator version and checksums. |
Review effort: Balanced
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #348 +/- ##
=====================================
Coverage 99.5% 99.5%
=====================================
Files 351 351
Lines 64349 64349
=====================================
Hits 64050 64050
Misses 299 299
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The documented Cargo 1.97 minimum is not enforced, allowing older selected toolchains to reach the check without the required version gate and actionable upgrade guidance.
1 open issue
1 resolved since last review
Review effort: Balanced
| Write-Host "anvil-check-all-targets: checking $($cfg.name) features" | ||
| $featureArgs = $cfg.flags | ||
| & cargo {{_anvil_stable_toolchain_args}} each @pkg --keep-going '--' cargo {{_anvil_stable_toolchain_args}} check --package '{cargo-spec}' --all-targets @featureArgs --locked | ||
| & cargo {{_anvil_stable_toolchain_args}} each @pkg --keep-going '--' cargo {{_anvil_stable_toolchain_args}} check --package '{cargo-spec}' --all-targets @featureArgs --locked --config "build.warnings='deny'" |
| adopter selects, including a declared MSRV. Recipes rely on its behaviour, such as | ||
| source-qualified package IDs in `cargo metadata`, and keep no workarounds for older Cargo. | ||
| The selected stable toolchain for `check-all-targets` requires Cargo 1.97 or newer | ||
| to enforce its warnings-as-errors policy. |
There was a problem hiding this comment.
[copilot speaking] [non-blocking] Could we append “; older Cargo versions ignore this setting” to make the behavior below Cargo 1.97 explicit?
There was a problem hiding this comment.
🟡 Changes recommended
The public setup instructions omit the separate Cargo 1.97+ stable-check requirement, allowing adopters following the documented 1.95+ minimum to select an incompatible Cargo version for that check.
2 open issues
Review effort: Balanced
| - The selected stable toolchain for `check-all-targets` requires **Cargo 1.97 | ||
| or newer** to enforce its warnings-as-errors policy. |
| " @{ name = 'no-default'; flags = @('--no-default-features') }\n", | ||
| " )\n", | ||
| " foreach ($cfg in $configs) {\n", | ||
| " Write-Host \"anvil-check-all-targets: checking $($cfg.name) features\"\n", |
There was a problem hiding this comment.
🤖: Non-blocking — The test suite does not protect the intended exception for warnings explicitly allowed in source.
Add a recipe-level success case where an otherwise warning-producing item has #[allow(...)], and assert both feature passes succeed. Without that case, a future stricter warning policy could reject intentionally allowed code while the current source-string assertions remain green.
There is inconsistency between how Anvil's recipes for
cargo-checkandcargo-clippytreat warnings.Because
cargo-clippyonly runs on codebase with all features turned on, it can miss dead code that is only dead when some features are turned off. Meanwhile, if such some downstream crate uses the problematic crate without those features, that particular Clippy run won't be ran with those features on in the upstream crate and will fail. The dead code warnings were intercepted in the runs for PR that introduced them, but becausecargo-check(which caught them) doesn't fail on Warnings, they went unnoticed.This PR is forcing
cargo-checkto fail on Warnings and ensures the consistency of checking a crate as the target with changes vs checking it as an upstream dependency.