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

docs: mention that Clippy must be run with the MSRV #4676

Merged
merged 3 commits into from
May 10, 2022

Conversation

SabrinaJewson
Copy link
Contributor

Motivation

In #4675 I learnt the hard way that Tokio uses Clippy on its MSRV.

Solution

Document this in the contributor's guide.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

thanks for documenting this, this is definitely good to have written down!

i wonder if we want to add a bash script or makefile or something for running clippy with the correct version, eventually?

i suggested adding some other places where we record the MSRV in the lists of places that need to be updated, but besides that, this change looks good to me!

CONTRIBUTING.md Outdated
Comment on lines 137 to 142
<!--
When updating this, also update:
- .github/workflows/ci.yml
- README.md
- tokio/README.md
-->
Copy link
Member

Choose a reason for hiding this comment

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

it would be really nice to have a script for this or something, or some way to to have a single authoritative definition of the MSRV that is referenced elsewhere. there are now several places where the MSRV will need to be updated.

obviously, this would be best left for a separate PR, but i thought it was worth noting.

.github/workflows/ci.yml Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
README.md Show resolved Hide resolved
tokio/README.md Show resolved Hide resolved
@hawkw hawkw enabled auto-merge (squash) May 10, 2022 17:11
@hawkw
Copy link
Member

hawkw commented May 10, 2022

ugh...i forgot that we have a CI job that checks that the two README files are identical, which is now failing because they list different other locations that need to be updated: https://github.com/tokio-rs/tokio/runs/6374617535?check_suite_focus=true

we should probably just add each README's own path to the list of updated items so that CI job can pass.

README.md Show resolved Hide resolved
tokio/README.md Show resolved Hide resolved
@hawkw hawkw merged commit 593b042 into tokio-rs:master May 10, 2022
@SabrinaJewson SabrinaJewson deleted the clippy-msrv-contributing branch May 10, 2022 18:04
Comment on lines +134 to +135
Clippy must be run using the MSRV, so Tokio can avoid having to `#[allow]` new
lints whose fixes would be incompatible with the current MSRV:
Copy link
Member

Choose a reason for hiding this comment

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

Note that we are actually using a newer version of clippy than MSRV (it is needed for some regression tests).

rust_clippy: 1.52.0

Comment on lines +16 to +23
# When updating this, also update:
# - README.md
# - tokio/README.md
# - CONTRIBUTING.md
# - tokio/Cargo.toml
# - tokio-util/Cargo.toml
# - tokio-test/Cargo.toml
# - tokio-stream/Cargo.toml
Copy link
Member

Choose a reason for hiding this comment

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

There is also .clippy.toml (it is a way to tell MSRV to clippy).

msrv = "1.49"

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.

3 participants