-
Notifications
You must be signed in to change notification settings - Fork 157
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 Rust to 1.81.0 #1073
Pin Rust to 1.81.0 #1073
Conversation
Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
@unexge, is the |
@passaro, not sure. It's a popular request for |
Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
.github/workflows/bench.yml
Outdated
- name: Set up stable Rust | ||
uses: dtolnay/rust-toolchain@stable | ||
- name: Set up Rust toolchain | ||
uses: dtolnay/rust-toolchain@master |
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.
My suggestion was to remove this step entirely, not to revert to stable
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.
Alternative: we could look at some other actions, e.g. https://github.com/marketplace/actions/rust-toolchain-toml-install
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.
Agh, I misunderstood. I think dtolnay/rust-toolchain
installs rustup
/cargo
. We might look installing it in .github/actions/install-dependencies
step, not sure if there are any downsides.
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 was wondering about jumping to a new action, I saw this one that looks well maintained: https://github.com/actions-rust-lang/setup-rust-toolchain
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.
Updated to use actions-rust-lang/setup-rust-toolchain
. I had to override rustflags
option though, it's -D warnings
by default which turns all warnings into compile errors, and vendor/fuser
has lots of warnings. It'd be nice to get rid of those warnings eventually, but not turning this on should be fine for now.
This new action also surfaces warnings on PRs, e.g., scroll to the bottom on https://github.com/awslabs/mountpoint-s3/pull/1073/files. We can turn this of it we find it noisy, but eventually I'd like to address these warnings as well.
Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
Signed-off-by: Burak Varlı <burakvar@amazon.co.uk>
rust-toolchain.toml
actions-rust-lang/setup-rust-toolchain
GitHub action as it supportsrust-toolchain.toml
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).