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

[ci] Configure clippy to fail on warnings #2669

Closed
wants to merge 7 commits into from
Closed

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Dec 9, 2023

Before this PR: clippy has one warning, but did not fail:
https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4672812

warning: field `cache_dir` is never read
  --> polkadot/node/core/pvf/benches/host_prepare_rococo_runtime.rs:32:2
   |
30 | struct TestHost {
   |        -------- field in this struct
31 |     // Keep a reference to the tempdir as it gets deleted on drop.
32 |     cache_dir: tempfile::TempDir,
   |     ^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default
warning: `polkadot-node-core-pvf` (bench "host_prepare_rococo_runtime") generated 1 warning

After fix in this PR: clippy fails on warnings.
https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4672949

@bkontur bkontur added the R0-silent Changes should not be mentioned in any release notes label Dec 9, 2023
@bkontur bkontur changed the title clippy investigation [ci] Clippy fails on warnings Dec 9, 2023
@bkontur bkontur marked this pull request as ready for review December 9, 2023 21:56
@bkontur bkontur requested a review from a team as a code owner December 9, 2023 21:56
@@ -5,6 +5,7 @@ cargo-clippy:
- .common-refs
- .pipeline-stopper-artifacts
script:
# The removal of `RUSTFLAGS` is related to the configuration of `cargo-clippy` in `.cargo/config.toml`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be more specific here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could/should, but I don't quite understand the full comment here: https://github.com/paritytech/polkadot-sdk/blob/master/.cargo/config.toml#L7-L12

# An auto defined `clippy` feature was introduced,
# but it was found to clash with user defined features,
# so was renamed to `cargo-clippy`.
#
# If you want standard clippy run:
# RUSTFLAGS= cargo clippy

Maybe we should also change/align that?

@liamaharon @bkchr @ggwpez @alvicsam any suggestions?

Copy link
Contributor

@liamaharon liamaharon Dec 11, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If we've updated the compiler to 1.74 then there's a clippy PR that we can land that removes our usage of the hack in .cargo/config.toml as we can now store lints in the workspace toml file. I think in my PR there's a couple of new crates that need to have their Cargo.tomls tweaked and then it should run green. #2390

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gilescope
Giles, cool, thank you for the comment :)
Do you plan to finish #2390? If not, I could. Just, please, let me know.

CI has 1.74:

$ cargo --version
cargo 1.74.0 (ecb9851af 2023-10-18)
...
$ cargo +nightly --version
cargo 1.75.0-nightly (b4d18d4bd 2023-10-31)

@bkontur bkontur changed the title [ci] Clippy fails on warnings [ci] Configure clippy to fail on warnings Dec 11, 2023
@bkontur
Copy link
Contributor Author

bkontur commented Dec 13, 2023

closing this in favor of #2390 which is fixed now

@bkontur bkontur closed this Dec 13, 2023
@bkontur bkontur deleted the bko-clippy2 branch December 13, 2023 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants