paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Explicit definition of required rust toolchain #11307

Open andresilva opened 2 years ago

andresilva commented 2 years ago

Currently we require a nightly toolchain to build the runtime (#1252). As part of CI we are currently using a nightly compiler for building the runtime and a stable compiler for the client code. It is not always obvious which nightly is needed to build Substrate at any given point (it's a common question in the support channels). Additionally, every time we need to bump the nightly version we need to ask the CI team to update the docker images otherwise the builds fail.

IMO we should introduce a rust-toolchain file that is supported by rustup (https://rust-lang.github.io/rustup/overrides.html#the-toolchain-file). We could still have CI docker images that come with a rust toolchain preloaded as a performance optimization (to avoid always downloading the toolchain), but this would guarantee that we wouldn't need to bother CI team to update the image whenever we need to bump the nightly (they could do it asynchronously since it's just a performance optimization).

This has one problem though: the rustup toolchain file only allows defining one toolchain to be used, whereas we currently use two toolchains. We could get around this by either: building everything with nightly (behavior change from what we do now), or using a stable toolchain with RUSTC_BOOTSTRAP=1 (this is how we currently build our production runtimes AFAIK). The fact that users would have to setup that environment variable might be troublesome though (or we can introduce direnv to this project :)

(I had previously created a PR for this: https://github.com/paritytech/substrate/pull/10607)

pepyakin commented 2 years ago

The fact that users would have to setup that environment variable might be troublesome though (or we can introduce direnv to this project :)

There were ideas of using the xtask approach, specifically for building the runtime in post native-optimization world (for context about that world refer to https://github.com/paritytech/substrate/issues/7288 https://github.com/paritytech/polkadot-sdk/issues/62).

Maybe that thing could provide a proper interface hiding the RUSTC_BOOTSTRAP thing.

andresilva commented 2 years ago

We could also make wasm-builder inject the RUSTC_BOOTSTRAP environment variable.

ggwpez commented 2 years ago

I am also in favor of adding a rust-toolchain file since that could reduce the number of people that experience compile errors and open issues for it.
Maybe we can use one of the overrides for the wasm code? Setting the RUSTUP_TOOLCHAIN in the wasm builder or using a directory override for it: https://rust-lang.github.io/rustup/overrides.html ?

bkchr commented 2 years ago

While I'm for moving the control about the rust version to use into the repo instead of having the CI file defining this, I'm not convinced to introduce any kind of further magic. RUSTC_BOOTSTRAP is a hack, by the compiler team. We could use it in our CI, I'm fine with this, but not setting this by default by the wasm-builder.

The rust-toolchain will also not fix issues by people experiencing build problems, because they probably depend on Substrate and their this toolchain file has no influence. Meaning they can still use some newer rust version that is maybe breaking something.

TriplEight commented 2 years ago

Good idea, thanks.

A short explainer about how CI automatically updates Rust versions currently: On the example of CI image for Substrate. The job rebuilds the images nightly. While stable is installed in a base image and not pinned to an exact version, we've moved to pinning the nightly due to there's a code to update every time. Then ci-linux:staging being scanned for vulnerabilities and tested against substrate and polkadot. If everything's green, it's promoted to production.

Of course we could read some particular (nightly) version from i.e. substrate repo and build a staging image. This would advance the automation, since now the entire procedure of updating a new Rust version can be done by someone who updates the broken code.

athei commented 1 year ago

I think we should just close this once we are using the same stable toolchain for both client and runtime: https://github.com/paritytech/substrate/pull/13580#issuecomment-1466549774