Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: Tweak make validate-rust #1159

Merged
merged 1 commit into from
Mar 1, 2025

Conversation

cgwalters
Copy link
Collaborator

This took me some thinking and experimenting. Basically we want:

  • Hard deny some warnings (this is covered by the Cargo.toml workspace.lints.rust)
  • Gate merging to main in CI on an exact set of warnings we want to forbid, but without also using a blanket -Dwarning deny policy because that could break our build when the compiler revs.
  • A corollary to the previous: allow developing locally without killing the build just because you have an unused import or some dead code (for example). So we don't want to add dead_code = deny into the Cargo.toml.
  • Be able to easily reproduce locally what CI is gating on in an efficient way.

We already had make validate-rust which was intending to navigate this, but what was missing was the "deny extended set of warnings" so we got code committed to git main which hit unused_imports.

Clippy upstream docs recommend the RUSTFLAGS = -Dwarnings approach in e.g.
https://doc.rust-lang.org/clippy/continuous_integration/github_actions.html but again I think this is a problem because it can break with updated Rust/clippy versions (unless you pin on those, but that becomes a pain in and of itself).

The problem also with doing RUSTFLAGS = -Dwarnings locally is it blows out the cargo cache.

So here's the solution I came to: We run cargo clippy -A clippy:all, and then deny some specific clippy lints and the core Rust warnings we want (unused_imports type things) at this stage. The advantage is this doesn't blow out the main Cargo cache, and I can easily reproduce locally exactly what CI would gate on.

Also while we're here, add make fix-rust which is a handy way to use the existing clippy --fix to locally fix things like unused imports as well as other machine-applicable things that are in e.g. clippy::suspicious.

This took me some thinking and experimenting. Basically we want:

- Hard deny some warnings (this is covered by the Cargo.toml
  workspace.lints.rust)
- Gate merging to main in CI on an exact set of warnings we
  want to forbid, but *without* also using a blanket
  -Dwarning deny policy because that could break our build
  when the compiler revs.
- A corollary to the previous: allow developing locally
  without killing the build just because
  you have an unused import or some dead code (for example).
  So we don't want to add `dead_code = deny` into the Cargo.toml.
- Be able to easily reproduce locally what CI is gating on
  in an efficient way.

We already had `make validate-rust` which was intending to navigate
this, but what was missing was the "deny extended set of warnings"
so we got code committed to git main which hit `unused_imports`.

Clippy upstream docs recommend the `RUSTFLAGS = -Dwarnings`
approach in e.g.
https://doc.rust-lang.org/clippy/continuous_integration/github_actions.html
but again I think this is a problem because it can break with
updated Rust/clippy versions (unless you pin on those, but that
becomes a pain in and of itself).

The problem also with doing `RUSTFLAGS = -Dwarnings` *locally*
is it blows out the cargo cache.

So here's the solution I came to: We run `cargo clippy -A clippy:all`,
and then deny some specific clippy lints *and* the core Rust
warnings we want (`unused_imports` type things) at this stage.
The advantage is this doesn't blow out the main Cargo cache,
and I can easily reproduce locally exactly what CI would gate on.

Also while we're here, add `make fix-rust` which is a handy
way to use the existing `clippy --fix` to locally fix things
like unused imports as well as other machine-applicable
things that are in e.g. `clippy::suspicious`.

Signed-off-by: Colin Walters <[email protected]>
@cgwalters cgwalters enabled auto-merge March 1, 2025 18:22
@cgwalters cgwalters merged commit 5b15d68 into bootc-dev:main Mar 1, 2025
23 checks passed
@cgwalters
Copy link
Collaborator Author

Wait that's weird...this auto-merged without an approving review.

@cgwalters
Copy link
Collaborator Author

The github status even says
image

@jeckersb
Copy link
Contributor

jeckersb commented Mar 4, 2025

Retroactive 👍 ? 😆

@cgwalters
Copy link
Collaborator Author

I filed a support ticket asking this to be diagnosed.

@cgwalters
Copy link
Collaborator Author

I filed a support ticket asking this to be diagnosed.

FTR support got back to me and noted this commit was rolled into
#1157
and when that merged, it noted this exact commit was merged and closed it. This is a known behavior, and makes sense. The obvious thing to do on GH's side though is just add pretty visible note in the PR making clear when this happens.

I messed up and I hadn't meant to roll this commit into that PR, that's why I got confused.

Anyways, all good!

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