-
Notifications
You must be signed in to change notification settings - Fork 127
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
Add MSRV 1.75 workflow check #981
Conversation
Bencher
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 10 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 2 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
@@ -2,7 +2,6 @@ name = "stratum_v2_protocols" | |||
version = "1.0.0" | |||
authors = ["The Stratum v2 Developers"] | |||
edition = "2021" | |||
rust-version = "1.75.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the point of CI, but why is it desirable to remove it from the different Cargo.toml
files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does version pinning locally across the rust toolchain, that can affect some local text editors that are configured for example to use only stable or other version. As the CI will cover any new code that does not pass MSRV, this version pinning becomes redundant and annoying for some :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that has happened to me as well; my code editor continuously updates the Cargo.lock
file because of this pinning. It would be nice to remove this pinning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we keep the removal of the rust-version
lines in the Cargo.toml
s and keep the removal of the rust-toolchain.toml
, but then we clearly document in the README.md
or the CONTRIBUTING
guide that the MSRV is 1.75.0
and we add a pre-commit hook to the .githooks/pre-push
to run this workflow locally before pushing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the README.md
already mention the MSRV, not sure about the hooks tbh as it seems its not widely used
@plebhash I dont think we have much to update. If we had any |
Thanks @rrybarczyk |
c9574e9
to
f616fa2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ltgm. Do we want to add a bash script in scipts/
to call this? I can also do this in the #1039 PR I have open if you want to add that but don't want to do it in this PR.
f616fa2
to
08adad9
Compare
@plebhash I think this is good to go |
resolves #980