-
Notifications
You must be signed in to change notification settings - Fork 893
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(cli)!: remove implicit toolchain installation #3985
Conversation
ad2df9e
to
a1292bb
Compare
366cda0
to
6a61fb3
Compare
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.
This is looking great!
I think after this lands we should probably do a release! |
@djc There are 20-ish items remaining in the milestone: https://github.com/rust-lang/rustup/milestone/8. I invite you to check them out and let's decide which ones are here to stay and which ones will be left for v1.28.1, or even v1.29.0. I'm even for having a very long beta cycle for this one: there are just so many changes and I'm afraid of breaking anything by accident: I just don't have that much bandwidth, plus I have to do the legwork around cutting a new release. Update: I've created a new milestone for v1.28.1 and have moved some not-so-urgent items there: https://github.com/rust-lang/rustup/milestone/13 Also, please remember we can add new stuff even in the beta stage, as long as they are minor enough (doc changes, new warnings, etc). We've done this with v1.27.0 as well. |
6a61fb3
to
bfa64d3
Compare
Hi, does this PR mean that if I have a rust-toolchain.toml set to a specific release, rustup will no longer automatically install that release, instead a manual command must be run before that toolchain is available? If so this is going to break CI for many projects who rely on a pinned nightly or pinned stable + fail on warnings. |
Yes, you've understood it correctly, and this breakage is intentional: #3635 (comment) Before the actual release, we'll definitely clearly communicate this change with the community, and we'll use the beta phase to collect user feedback. |
I am looking forward to this change, so thanks for working on it re: breakage Might I suggest adding the new behaviour (#3983), and then waiting a release or two before completely removing the implicit installs? My concern is having a path with some continuity; If there is need - perhaps some tool that is blocked waiting on the new behaviour - during the transitional period an extra flag could be added to opt-out of implicit installs, at the risk of some complexity. |
We currently propose #3635 (comment) as a migration path. If you have any concerns/suggestions/comments, let's continue the discussion over there. |
Closes #3943.
As a follow-up of #3983, this PR makes
rustup toolchain install
(orrustup install
) the only way to ensure the installation of the active toolchain (apart from installing it manually).When the active toolchain needs to be installed, the existing error messages now come to our rescue:
rustup/tests/suite/cli_v2.rs
Lines 320 to 336 in e857897
This PR also makes it possible to merge
find_or_install_active_toolchain()
andfind_active_toolchain()
by reducing the number of call sites of the former.Given the current state of this PR, the CI will certainly fail, since even the test suite also relies on implicit installation.Fixup commits has been added accordingly, which will of course be squashed onto the original commit respectively before actually merging this PR.