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

Add rust-toolchain.toml and pin to a working nightly #502

Merged

Conversation

nicholasbishop
Copy link
Member

(See the two commits for more details)

@nicholasbishop nicholasbishop force-pushed the bishop-toolchain-file-2 branch 3 times, most recently from df039a5 to 154d4bf Compare September 4, 2022 20:02
@nicholasbishop
Copy link
Member Author

Thanks @phip1611 for the suggestion to use rust-toolchain.toml. I was completely unaware that existed, it sure simplifies a lot of things here :)

@phip1611
Copy link
Member

phip1611 commented Sep 4, 2022

I'm in favor of this! Will approve once the comments are addressed. Thanks for working on this!

@nicholasbishop
Copy link
Member Author

Will approve once the comments are addressed.

Double-checking -- I don't see any review comments here, maybe still in draft form?

@phip1611
Copy link
Member

phip1611 commented Sep 5, 2022

Will approve once the comments are addressed.

Double-checking -- I don't see any review comments here, maybe still in draft form?

Oh no. I ran into this issue: community/community#10369

Should be fine now

# don't have miri available though, so pin to a slightly older version.
channel = "nightly-2022-08-26"
components = [
# Needed for `-Zbuild-std`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very minor nit: probably you can add a comment here

"additive components to the default profile" or so

Feel free to merge with or without this!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 added

Add a `rust-toolchain.toml` file that specifies the channel and
components needed to build uefi-rs. See
https://rust-lang.github.io/rustup/overrides.html#the-toolchain-file for
documentation of the `rust-toolchain.toml` format.

Remove the `--toolchain` option from `xtask`, as using this file is a
simpler alternative. Also remove the instructions for installing the
nightly toolchain from the readme, as `rustup` will do this
automatically now.

The template application has been updated to add a
`rust-toolchain.toml`, and `BUILDING.md` has been updated to mention it
as well.

The `toolchain` action previously used in the CI is unmaintained and
doesn't understand `rust-toolchain.toml` (it does understand the older
`rust-toolchain` file, but that doesn't allow for including required
components). Since the runners come with rustup already installed, we
don't actually need a complex action here anyway, so just remove those
sections entirely. A few jobs need extra components installed, do that
with `rustup component add`.

Note that this change will also cause `xtask` itself to be built with
nightly. That should generally be fine, we only had it using stable
because of rust-osdev#397, and in the
future we can easily temporarily pin to a non-current nightly if a bug
occurs with an `xtask` dependency again.

Fixes rust-osdev#498
Temporarily modify rust-toolchain.toml to pin to `nightly-2022-08-26`,
as compilation is broken on current nightly due to
rust-osdev#500. `nightly-2022-09-01`
is the most recent nightly that avoids the bug, but it doesn't have miri
(which is run by CI), so pin to a slightly older version.

Also pinned the toolchain in the template app.

This change should be reverted once rust-osdev#500 is fixed.
@nicholasbishop nicholasbishop force-pushed the bishop-toolchain-file-2 branch from d30b454 to 7a936fd Compare September 6, 2022 15:17
@nicholasbishop nicholasbishop merged commit c6088c0 into rust-osdev:main Sep 6, 2022
@nicholasbishop nicholasbishop deleted the bishop-toolchain-file-2 branch September 6, 2022 15:21
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.

2 participants