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: use workspace inheritance to enforce lints in all crates #4575

Merged
merged 22 commits into from
Oct 9, 2023

Conversation

binsta
Copy link
Contributor

@binsta binsta commented Sep 30, 2023

Description

Starting with nightly-2023-09-10, the [lints] section in Cargo.toml files is stable. Together with workspace inheritance, this can be used to declare all lints we want to enforce in a single place.

Resolves: #4484.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@binsta
Copy link
Contributor Author

binsta commented Sep 30, 2023

@thomaseizinger Please take a look. If any changes are needed, please let me know. if the [lints] tag is in a stable release it will cool.

Cargo.toml Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

@thomaseizinger Please take a look. If any changes are needed, please let me know. if the [lints] tag is in a stable release it will cool.

Thanks for getting this started :)

@binsta
Copy link
Contributor Author

binsta commented Oct 1, 2023

@thomaseizinger I have a quick question on unreachable_pub. Running on cargo +nightly clippy

warning[E0602]: unknown lint: `clippy::unreachable_pub

Please check clippy::useless_attribute

@thomaseizinger
Copy link
Contributor

@thomaseizinger I have a quick question on unreachable_pub. Running on cargo +nightly clippy

warning[E0602]: unknown lint: `clippy::unreachable_pub

Please check clippy::useless_attribute

That is because it is a rustc lint, not a clippy lint! I think you'll have to move it out of the clippy group in Cargo.toml :)

core/Cargo.toml Outdated Show resolved Hide resolved
core/Cargo.toml Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Oct 1, 2023

This pull request has merge conflicts. Could you please resolve them @binsta? 🙏

@thomaseizinger
Copy link
Contributor

@binsta Once you've resolved the merge conflicts, CI can run again :)

@thomaseizinger
Copy link
Contributor

@binsta Once you've resolved the merge conflicts, CI can run again :)

Btw, merging master is fine, we don't really do rebases here! See https://github.com/libp2p/rust-libp2p/blob/master/CONTRIBUTING.md#we-squash-merge-pull-requests.

@binsta binsta changed the title [workspace.lints] add in Cargo.toml and nightly add CI for experiment … feat: [workspace.lints] add in Cargo.toml and nightly add CI for experiment … Oct 2, 2023
@binsta binsta changed the title feat: [workspace.lints] add in Cargo.toml and nightly add CI for experiment … feat: [workspace.lints] add in Cargo.toml Oct 2, 2023
@binsta
Copy link
Contributor Author

binsta commented Oct 2, 2023

@thomaseizinger Please review every conflict that has been resolved.

@binsta
Copy link
Contributor Author

binsta commented Oct 2, 2023

@binsta Once you've resolved the merge conflicts, CI can run again :)

Btw, merging master is fine, we don't really do rebases here! See https://github.com/libp2p/rust-libp2p/blob/master/CONTRIBUTING.md#we-squash-merge-pull-requests.

Thank you, Hereafter ill follow the same, methodology

@thomaseizinger
Copy link
Contributor

@thomaseizinger Please review every conflict that has been resolved.

Thanks! They weren't quite resolved but I went ahead and fixed them for you :)

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

CI for clippy is passing which is nice! See https://github.com/libp2p/rust-libp2p/actions/runs/6386769096/job/17333995887?pr=4575.

I think we can merge this as is and simply swap out the clippy jobs again once this hits stable!

Cargo.toml Outdated Show resolved Hide resolved
examples/browser-webrtc/Cargo.toml Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title feat: [workspace.lints] add in Cargo.toml ci: use workspace inheritance to enforce lints in all crates Oct 2, 2023
@binsta
Copy link
Contributor Author

binsta commented Oct 3, 2023

@thomaseizinger Please take a look

swarm/Cargo.toml Outdated Show resolved Hide resolved
transports/quic/Cargo.toml Outdated Show resolved Hide resolved
transports/quic/Cargo.toml Outdated Show resolved Hide resolved
transports/tcp/Cargo.toml Outdated Show resolved Hide resolved
transports/tls/Cargo.toml Outdated Show resolved Hide resolved
transports/webrtc-websys/Cargo.toml Outdated Show resolved Hide resolved
thomaseizinger
thomaseizinger previously approved these changes Oct 3, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks! This is good to go from my end :)

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Oct 3, 2023

@libp2p/rust-libp2p-maintainers: Starting with this PR, you'll have to use nightly Rust for linting locally for a limited time. It is generally a good idea to use nightly Rust for local development because it helps detecting compiler bugs early. Our CI enforces that things work with stable anyway.

This feature is due to be stabilized in 1.74 I think hence I think it makes sense to already start using this. Once it is actually stable, we can switch CI back to running on the current stable and beta.

If there are no concerns, I'll merge this in the upcoming days.

@mergify
Copy link
Contributor

mergify bot commented Oct 3, 2023

This pull request has merge conflicts. Could you please resolve them @binsta? 🙏

@thomaseizinger
Copy link
Contributor

@libp2p/rust-libp2p-maintainers: Starting with this PR, you'll have to use nightly Rust for linting locally for a limited time. It is generally a good idea to use nightly Rust for local development because it helps detecting compiler bugs early. Our CI enforces that things work with stable anyway.

This feature is due to be stabilized in 1.74 I think hence I think it makes sense to already start using this. Once it is actually stable, we can switch CI back to running on the current stable and beta.

If there are no concerns, I'll merge this in the upcoming days.

Most notably, nothing changes for users that are currently using stable rust and run cargo clippy. They'll still see a bunch of warning that are only ignored via our custom-clippy alias. To transition those, we could retain the alias for now but change it to run on a specific Rust toolchain, i.e. the nightly version that stabilizes the [lints] feature.

Or we just retain the alias altogether until [lints] stabilizes completely. We just need to manually keep it in sync until then but that shouldn't be too difficult given how rarely it changes.

@mergify

This comment was marked as resolved.

@mergify

This comment was marked as resolved.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

As discussed in our 1on1, feel free to go ahead here @thomaseizinger.

@mergify mergify bot dismissed thomaseizinger’s stale review October 9, 2023 00:07

Approvals have been dismissed because the PR was updated after the send-it label was applied.

thomaseizinger
thomaseizinger previously approved these changes Oct 9, 2023
@mergify mergify bot dismissed thomaseizinger’s stale review October 9, 2023 01:00

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit 0e9d339 into libp2p:master Oct 9, 2023
72 checks passed
monoid pushed a commit to fluencelabs/rust-libp2p that referenced this pull request Oct 9, 2023
Starting with nightly-2023-09-10, the `[lints]` section in `Cargo.toml` files is stable. Together with workspace inheritance, this can be used to declare all lints we want to enforce in a single place.

Resolves: libp2p#4484.

Pull-Request: libp2p#4575.
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
Starting with nightly-2023-09-10, the `[lints]` section in `Cargo.toml` files is stable. Together with workspace inheritance, this can be used to declare all lints we want to enforce in a single place.

Resolves: libp2p#4484.

Pull-Request: libp2p#4575.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replace custom-clippy alias with workspace-wide [lints] configuration
3 participants