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

Switch GitHub actions to actions-rust-lang/setup-toolchain #1182

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

Twey
Copy link
Contributor

@Twey Twey commented Oct 30, 2023

Motivation

actions-rs has a very old version of rustup and requires a legacy rust-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

  • Need to bump the major/minor version number in the next release of the crates.
  • Need to update the developer manual.
  • This PR is adding or removing Cargo features.
  • Release is blocked and/or tracked by other issues (see links below)

Links

@Twey Twey force-pushed the 10-30-Deprecated_github_action branch 2 times, most recently from 321d7a5 to 3f54494 Compare October 30, 2023 15:14
@Twey Twey marked this pull request as ready for review October 30, 2023 15:54
@Twey Twey requested a review from ma2bd October 30, 2023 16:27
@@ -131,7 +123,7 @@ jobs:
cargo install cargo-sort --locked
- name: Check formatting
run: |
cargo +nightly fmt -- --check
Copy link
Contributor

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

Copy link
Contributor Author

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.

@Twey Twey marked this pull request as draft October 31, 2023 19:21
@Twey Twey marked this pull request as ready for review October 31, 2023 22:44
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.
@Twey Twey force-pushed the 10-30-Deprecated_github_action branch from 181d63a to ce890c9 Compare November 1, 2023 13:01
@Twey
Copy link
Contributor Author

Twey commented Nov 1, 2023

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).

@Twey Twey requested a review from ma2bd November 1, 2023 13:18
@Twey Twey merged commit c384aa0 into linera-io:main Nov 1, 2023
6 checks passed
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.

None yet

2 participants