-
Notifications
You must be signed in to change notification settings - Fork 178
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
Switch GitHub actions to actions-rust-lang/setup-toolchain #1182
Conversation
321d7a5
to
3f54494
Compare
@@ -131,7 +123,7 @@ jobs: | |||
cargo install cargo-sort --locked | |||
- name: Check formatting | |||
run: | | |||
cargo +nightly fmt -- --check |
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.
We need this one to fully normalize the imports with imports_granularity = Crate
.
rust-lang/rustfmt#4991
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.
Okay, I'll submit a patch to the action — currently if you have a toolchain file there's no way to use a different toolchain than the one specified there.
Removes explicit reference to Swatinem/rust-cache as actions-rust-lang/setup-rust-toolchain automatically uses this action to enable caching (unless we set `cache: false`). Also switches our toolchain environment to be the same as the build environment, as the `rust-toolchain.toml` file overrides it anyway. This is undesirable: nightly rustfmt has some additional options we would like to keep.
Since we use nightly Rust tooling for our linting, we want to override our usual toolchain for the linting step. Upstream `setup-rust-toolchain` currently doesn't allow this. I have submitted a PR at actions-rust-lang/setup-rust-toolchain#26 to add the functionality, but until then this commit points to a branch on my personal repository at https://github.com/Twey/setup-rust-toolchain/. We should revert it once the PR is merged.
181d63a
to
ce890c9
Compare
At the moment this PR uses my branch on https://github.com/Twey/setup-rust-toolchain to allow for overriding the toolchain just for linting. I have a PR open against https://github.com/actions-rust-lang/setup-rust-toolchain that will hopefully eventually be merged to enable this functionality for that upstream repository; I'm unclear whether we want to merge now (and revert the commit once the upstream repository includes the functionality) or wait until that is merged (and remove the commit pointing us at my own GitHub from this PR). |
Motivation
actions-rs
has a very old version ofrustup
and requires a legacyrust-toolchain
file. It has also been recently archived for reasons unknown to the community.Proposal
This PR replaces it with the hopefully-compatible
actions-rust-lang/setup-rust-toolchain
.Test Plan
If GitHub checks pass on this PR then the actions are correct.
Release Plan
Links