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

Retire stable-rust clippy for still buggy redundant_clone #1661

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

ryoqun
Copy link
Collaborator

@ryoqun ryoqun commented Jun 10, 2024

Problem

the clippy lint called redundant_clone got buggy in the past.

As a work around, we enabled stable clippy along side nightly clippy, to avoid back-port churns. that worked because we used non-buggy clippy from older rust toolchain at the time in the target backport branches, instead of disabling the useful redundant_clone across the board.

ref: solana-labs#31834

Summary of Changes

after 1 year, redundant_clone isn't fixed upstream yet. it's effectively disabled by being put under the nursery category of lints (since v1.71.0, rust-lang/rust#113417). now that, the lint is disabled across all of our active branches (master, v1.18, v1.17).

we have no choice but can't enable redundant_clone and this is true across board.

in a say, the situation is a redundant_clippy. lol. well well, this is such a life. we disable the dual clippy for faster ci with tears.

@ryoqun ryoqun requested review from CriesofCarrots and yihau June 10, 2024 00:39
@ryoqun ryoqun changed the title Retire stable-rust clippy for buggy redundant_clone Retire stable-rust clippy for still buggy redundant_clone Jun 10, 2024
Copy link
Member

@yihau yihau left a comment

Choose a reason for hiding this comment

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

thank you for bumping this up! it's good to see this one happen! could we have these modifications in this PR as well?

  • delete ./scripts/cargo-clippy-stable.sh. I think no one will use it anymore 🤔
  • removed these lines in the .mergify.yml

    agave/.mergify.yml

    Lines 64 to 66 in 5263c9d

    - or:
    - check-success=clippy-stable (macos-latest)
    - check-success=clippy-stable (macos-latest-large)

@@ -60,13 +60,9 @@ pull_request_rules:
- status-success=build & deploy docs
- or:
- -files~=(\.rs|Cargo\.toml|Cargo\.lock|\.github/scripts/cargo-clippy-before-script\.sh|\.github/workflows/cargo\.yml)$
- and:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

according to https://docs.mergify.com/configuration/conditions/, i think we can also remove this and: because it would only have a single or.

@ryoqun ryoqun requested a review from yihau June 10, 2024 04:45
Copy link
Member

@yihau yihau left a comment

Choose a reason for hiding this comment

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

looks good to me!

@ryoqun ryoqun merged commit cd6825f into anza-xyz:master Jun 10, 2024
52 checks passed
samkim-crypto pushed a commit to samkim-crypto/agave that referenced this pull request Jul 31, 2024
)

* Retire stable-rust clippy for buggy redundant_clone

* Remove unused scripts/cargo-clippy-stable.sh

* Remove clippy-stable from .mergify.yml
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