Simplify rustfmt config and switch to stable rustfmt #242
Conversation
| @@ -1,8 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
@ahl I believe it's correct to remove this file now. I think we did the same for omicron when we switch to stable rustfmt.
| @@ -14,17 +14,12 @@ jobs: | |||
| runs-on: ubuntu-18.04 | |||
| steps: | |||
| - uses: actions/checkout@230611dbd0eb52da1e1f4f7bc8bb0c3a339fc8b7 | |||
| - uses: actions-rs/toolchain@88dc2356392166efad76775c878094f4e83ff746 | |||
There was a problem hiding this comment.
@zephraph I think this change is correct -- we don't have to specify this action any more in this job, right?
There was a problem hiding this comment.
I thought this action was just to get access to any rust toolchain, and we needed:
- "nightly-2021-03-25" here, and
- "Whatever is in the rust-toolchain.toml" file below
But it seems like, based on the "checks" output, we actually happen to be downloading the version we need (specified in the rust-toolchain.toml) on the first call to cargo --version.
Checking my understanding: With this new change, are we just relying on whatever happens to be installed on Github's default ubuntu-18.04 image to bootstrap?
There was a problem hiding this comment.
I don't believe we're relying on whatever happens to be installed because we have a rust-toolchain file in the repo.
There was a problem hiding this comment.
To expand on that: the Actions environment, at least for Ubuntu 18.04, includes Rust 1.57. I believe this is updated pretty regularly when a new Rust release comes out. So this will likely already be the right thing when this runs. But if it's not (e.g., if they've updated to 1.58 and we have not yet updated our rust-toolchain file), then the Rust that is installed by default will honor the rust-toolchain file like it always does, installing the specified toolchain and running the rest of the build with it.
There was a problem hiding this comment.
Right, that's what I meant by "bootstrap" in my comment above. Sounds reasonable, if we expect that'll always exist in the Actions environment across our supported test systems.
There was a problem hiding this comment.
I think that's right. It's documented to be present in the Actions environments for Ubuntu, Windows, and Mac. I would imagine it would be a pretty big breaking change if it were removed. If it were, we could add this action back.
We might also add the action back if we wanted to override rust-toolchain to do builds with nightly or beta or some other stable (e.g., a MSRV).
davepacheco
force-pushed
the
rustfmt-simpler
branch
from
aa4c112 to
e36fc2a
Compare
davepacheco
changed the title
See #77 for lots of background. About a year ago, we switched to a simpler stable-only rustfmt config in omicron to try it out. That's been going well. It's time to apply that here, too.
This change:
cargo fmt)This has a (trivial) conflict with #139 so I've based this on #139 because I expect that to land shortly. I've set the branch to "pin-toolchain" to make the diff easier to read.