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

Consider adding some kind of (optional?) toolchain pinning to the CI #498

Closed
nicholasbishop opened this issue Sep 1, 2022 · 2 comments
Closed

Comments

@nicholasbishop
Copy link
Member

Overall the nightly channel is remarkably stable, but every now and then there's a bug that breaks the uefi-rs CI. For example: #494

When such a bug occurs it can take a while for things to get back to working; we have to first notice the bug, then bisect to where it fails, then file an issue in the rust repo, maybe put up a fix or wait for someone else to find a fix, wait for it to get reviewed, wait for it to get merged, and wait for the following nightly build to be created. So even under the best circumstances it will take some days to be resolved.

To minimize disruption when a thing like this occurs, it would be nice to pin to a known-working nightly release so that we can continue to merge PRs. On the other hand, it's also nice to generally test against latest nightly so that we can quickly respond to "legitimate" breakages, like when an unstable feature that we rely on is changed.

I'm thinking maybe something along the lines of a NIGHTLY_TOOLCHAIN=nightly in the workflow file, and adjust all the jobs to use that variable. When a breakage occurs we can temporarily set it to e.g. nightly-2022-01-02, and then revert back to nightly once the bug has been fixed.

@phip1611
Copy link
Member

phip1611 commented Sep 1, 2022

I'm in favor of it. How about using rust-toolchain.toml? This works not only for the CI but also for the local development.

I use it in all my personal projects.

@nicholasbishop
Copy link
Member Author

nicholasbishop commented Sep 1, 2022

Thanks, I'll take a look at that. I've never used rust-toolchain.toml before

nicholasbishop added a commit to nicholasbishop/uefi-rs that referenced this issue Sep 4, 2022
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 `toolchain` action used in the CI will pick up on this file as well,
so removed all the sections spelling out the toolchain and
components. 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
nicholasbishop added a commit to nicholasbishop/uefi-rs that referenced this issue Sep 4, 2022
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 `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.

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
nicholasbishop added a commit to nicholasbishop/uefi-rs that referenced this issue Sep 4, 2022
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 `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.

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
nicholasbishop added a commit to nicholasbishop/uefi-rs that referenced this issue Sep 4, 2022
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.

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
nicholasbishop added a commit to nicholasbishop/uefi-rs that referenced this issue Sep 5, 2022
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.

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
nicholasbishop added a commit to nicholasbishop/uefi-rs that referenced this issue Sep 5, 2022
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
nicholasbishop added a commit to nicholasbishop/uefi-rs that referenced this issue Sep 6, 2022
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
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

No branches or pull requests

2 participants