coconut-svsm / svsm

COCONUT-SVSM
MIT License
122 stars 43 forks source link

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

Closed chris-oo closed 3 months ago

chris-oo commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

We should probably discuss this in the next SVSM call

AdamCDunlap commented 4 months ago

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.

joergroedel commented 3 months ago

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

chris-oo commented 3 months ago

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

chris-oo commented 3 months ago

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.