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

pin the toolchain version used by clippy #846

Closed
wants to merge 2 commits into from

Conversation

danieleades
Copy link
Contributor

see discussion in #618

  • pin clippy version for consistent lints across all PRs
  • modify MSRV action such that dependabot will bump version of clippy toolchain but not touch msrv

@Philippe-Cholet
Copy link
Member

I was working on this but was kinda blocked (I mostly discover dependabot to be honest), so thanks.

What bug me about this kind of change is #758 that we did not want.

Are you saying that with: toolchain: 1.43.1 will make dependabot ignore it? That sounds so simple as I was looking in another direction to solve this (use "ignore" option).

@danieleades
Copy link
Contributor Author

I was working on this but was kinda blocked (I mostly discover dependabot to be honest), so thanks.

What bug me about this kind of change is #758 that we did not want.

Are you saying that with: toolchain: 1.43.1 will make dependabot ignore it? That sounds so simple as I was looking in another direction to solve this (use "ignore" option).

You certainly can configure dependabot to ignore the whole action, but that would also prevent it from bumping the pinned clippy toolchain version.

Dependabot doesn't parse the toolchain config, only the action version, so this should work fine (though it's untested).

I can probably test it on a fork to confirm it works the way I expect

@Philippe-Cholet
Copy link
Member

Ok that seems good. Another thing that bothered me in #758 was that it suggested to update the version to "1.80.0" while I thought it would have suggested "1.74.0" (or whatever appropriate version at the time it wrote it).

@danieleades
Copy link
Contributor Author

Ok that seems good. Another thing that bothered me in #758 was that it suggested to update the version to "1.80.0" while I thought it would have suggested "1.74.0" (or whatever appropriate version at the time it wrote it).

I missed that, that could be a problem.
Dependabot here is not doing anything clever to check the latest stable version, it's just checking the versions of the GitHub action that have been published. Perhaps these higher numbers are aliases for nightly releases?

@Philippe-Cholet
Copy link
Member

This is intentional but it's convenient ahead-of-time PR.
I'm having a hard time finding a Rust repo pinning clippy (and using Dependabot).

@mightyiam
Copy link
Contributor

For my taste, this is too many things in one PR.

@danieleades
Copy link
Contributor Author

I'm having a hard time finding a Rust repo pinning clippy (and using Dependabot).

pinning the version of linters is a general pattern, not limited to the Rust ecosystem

@danieleades
Copy link
Contributor Author

For my taste, this is too many things in one PR.

      - uses: dtolnay/rust-toolchain@master
        with:
          toolchain: stable
          components: rustfmt

to

      - uses: dtolnay/rust-toolchain@stable
        with:
          components: rustfmt

is not strictly required, but trivial. All other changes are required.

@danieleades
Copy link
Contributor Author

This is dtolnay/rust-toolchain#43 but it's convenient ahead-of-time PR.

i think this might be deal-breaker unfortunately

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Jan 13, 2024

i think this might be deal-breaker unfortunately

I agree. I think we are gonna stick with stable clippy and handle rare breakage as soon as it appears.

PS: There is not that many things in that PR.

@Philippe-Cholet
Copy link
Member

Thank you anyway.

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