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: Do not rely on RUSTFLAGS for Clippy lint configuration #499

Merged
merged 5 commits into from
Jun 7, 2024

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Jun 7, 2024

Since Rust 1.74 it's possible to configure lints directly in the Cargo.toml rather than relying on setting custom RUSTFLAGS: https://blog.rust-lang.org/2023/11/16/Rust-1.74.0.html#lint-configuration-through-cargo.

Setting RUSTFLAGS is fragile as there are some corner cases to look out for, like:

  1. it being exclusive with other sources of flags e.g. overwriting build.rustflags or target.<cfg>.rustflags or
  2. forcing a separate rebuild whenever the flags change, preventing the reuse of build cache in common cases (e.g. for a regular build vs one with warnings denied).

See NomicFoundation/slang#880 for what we did for Slang.

This also changes the pre-commit hook to not rely/set the flag and only runs clippy, rather than check + clippy, which seems wasteful in this case, as cargo clippy already runs the underlying rustc machinery for compilation checks.

Copy link

changeset-bot bot commented Jun 7, 2024

⚠️ No Changeset found

Latest commit: 74a9014

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Xanewok Xanewok added the no changeset needed This PR doesn't require a changeset label Jun 7, 2024
@Xanewok
Copy link
Contributor Author

Xanewok commented Jun 7, 2024

https://github.com/NomicFoundation/edr/actions/runs/9415777943/job/25937468081?pr=499 seems spurious, retrying:

 Alchemy Forked provider
         "before each" hook: Initialize provider for "Supports single and batched requests":
     Error: Timeout of 50000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/edr/edr/hardhat-tests/test/internal/hardhat-network/provider/modules/eth/websocket.ts)

@Xanewok Xanewok mentioned this pull request Jun 7, 2024
Copy link
Member

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

One nice-to-have. Otherwise looks good to me.

For next time, please make sure to assign reviewers to your PR. That way people get a more obvious notification and will likely review quicker :)

Cargo.toml Outdated
Comment on lines 17 to 19
rust_2018_idioms = "warn"
nonstandard_style = "warn"
future_incompatible = "warn"
Copy link
Member

Choose a reason for hiding this comment

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

Nice to have: alphabetic ordering

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 74a9014. thanks!

@Wodann Wodann changed the title chore: Do not rely on RUSTFLAGS for Clippy lint configuration build: Do not rely on RUSTFLAGS for Clippy lint configuration Jun 7, 2024
@Xanewok Xanewok merged commit 3e6d4da into main Jun 7, 2024
34 checks passed
@Xanewok Xanewok deleted the chore/no-clippy-rustflags branch June 7, 2024 17:55
@Wodann Wodann added this to the EDR v0.4.1 milestone Jul 11, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no changeset needed This PR doesn't require a changeset
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants