-
Notifications
You must be signed in to change notification settings - Fork 72
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
feat(toolchain): deprecate rust-toolchain-version in favor of rust-toolchain.toml #275
Conversation
…olchain.toml Deprecation reason: [rust-toolchain.toml](https://rust-lang.github.io/rustup/overrides.html#the-toolchain-file) is a more standard/universal mechanism for pinning toolchain versions for reproducibility. Teams without dedicated release engineers will likely benefit from unpinning their toolchain and letting the underlying CI vendor silently update them to 'some recent stable toolchain', as they will get updates/improvements and are unlikely to have regressions. If you delete the key, generate-ci won't explicitly setup a toolchain, so whatever's on the machine will be used (with things like rust-toolchain.toml behaving as normal). Before being deprecated the default was to ustup update stable, but this is no longer the case. We will warn if the key is set, recommending rust-toolchain.toml
This provides some minimal reproducibility aide for users who aren't pinning their rust toolchains to go back and find out what toolchain their infra was running at the time of the build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we think that we should fully deprecate on the off-chance people who do not use rustup want a way to pin?
"type": "object", | ||
"properties": { | ||
"cargo_version_line": { | ||
"description": "The version of Cargo used (first line of cargo -vV)\n\nNote that this is the version used on the machine that generated this file, which presumably should be the same version used on all the machines that built all the artifacts, but maybe not! It's more likely to be correct if rust-toolchain.toml is used with a specific pinned version.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"It's more likely" -> if rust-toolchain.toml is present and uses a specific version it will always be correct, right? the only way it wouldn't be correct is if rust-toolchain.toml wasn't present or if it was but specified something like "latest"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per https://rust-lang.github.io/rustup/overrides.html, several things have precedence over rust-toolchain.toml (like RUSTUP_TOOLCHAIN and rustup override
), and I can't guarantee you haven't messed up your build system by using one of these in addition to rust-toolchain.toml.
The way we use this field is to invoke rustup, so the feature was always dependent on it. The new behaviour when the field is removed essentially prevents cargo-dist from getting in the way of someone using their own system like nix or whatever to setup the toolchain. |
Thankyou! |
Deprecation reason: rust-toolchain.toml is a more standard/universal mechanism for pinning toolchain versions for reproducibility. Teams without dedicated release engineers will likely benefit from unpinning their toolchain and letting the underlying CI vendor silently update them to 'some recent stable toolchain', as they will get updates/improvements and are unlikely to have regressions.
If you delete the key, generate-ci won't explicitly setup a toolchain, so whatever's on the machine will be used (with things like rust-toolchain.toml behaving as normal). Before being deprecated the default was to
ustup update stable, but this is no longer the case.
We will warn if the key is set, recommending rust-toolchain.toml
This also adds system_info.cargo_version_line to dist-manifest.json, giviing back some reproducibility info to folks using unpinned toolchains.
fixes #205