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

build: pin rust toolchain to specified version in ci and locally #428

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

chris-oo
Copy link
Contributor

Rather than specifying stable as the toolchain to use in CI and locally, instead pin to a specific version of stable. This allows developers to manually fixup lints and issues with new toolchain updates and rev the appropriate versions across the code.

Additionally, add a minimum supported rust version to the main svsm crate, to give a more helpful compiler error when your toolchain does not match.

@chris-oo
Copy link
Contributor Author

I have a separate meta comment about rust-toolchain.toml, do we feel like we still need it? It stops folks from easily building with newer versions without doing some overrides.

In other projects, I've generally relied on the minimum rust version and pinning CI to a specific toolchain version. And, if you declare a given commit as a release branch, then we'd add the corresponding rust-toolchain.toml to pin the whole branch to a specific version. Otherwise, on main folks are free to run whatever toolchain version they'd like as long as it's greater than the minimum specified in Cargo.toml. Thoughts on that change?

@00xc
Copy link
Member

00xc commented Jul 29, 2024

We should probably discuss this in the next SVSM call

@AdamCDunlap
Copy link
Contributor

Something like this would be helpful for us as well.

You'll also want to update the Makefile where it specifies "cargo +nightly" in the bin/test-kernel.elf rule (even better would be if it is possible to have this specified in a Cargo.toml file instead of the makefile?)

You should also consider updating scripts/container/opensuse-rust.docker to install the correct version in the container, but this is not technically necessary since rustup will install the correct version for you at build time.

Copy link
Member

@joergroedel joergroedel left a comment

Choose a reason for hiding this comment

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

Hmm, this requires to specify the stable Rust version at three places. I guess there is no way to specify it in a single place for local and CI builds?

@joergroedel
Copy link
Member

Hey @chris-oo, can you please update this PR to make the CI pass? This is currently blocking the merge.

@joergroedel joergroedel added the wait-for-update PR is waiting to be updated to address review comments label Aug 16, 2024
@chris-oo
Copy link
Contributor Author

Sorry about the delay - I'll fix it up shortly.

@chris-oo
Copy link
Contributor Author

Hmm, this requires to specify the stable Rust version at three places. I guess there is no way to specify it in a single place for local and CI builds?

So the rust-version in Cargo.toml is the minimum supported rust version for the crate. Generally, I would keep it in lockstep with whatever CI is set to. I think though, that we should consider removing rust-toolchain.toml and just have the Cargo.toml enforce the minimum version, and CI can run against that version/be manually updated as appropriate, but I won't remove that in this PR.

Pin rust toolchain versions in CI to a specific version, 1.80, along with rust-toolchain.toml.

Additionally, add the matching minimum supported rust version to kernel/Cargo.toml.

Signed-off-by: Chris Oo <[email protected]>
@joergroedel joergroedel merged commit 6d8a3e9 into coconut-svsm:main Aug 27, 2024
3 checks passed
@joergroedel joergroedel removed the wait-for-update PR is waiting to be updated to address review comments label Aug 27, 2024
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.

4 participants