-
Notifications
You must be signed in to change notification settings - Fork 895
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
rustup.rs install should update old toolchains #2330
Comments
For the record, here is how I encountered this bug "in the wild":
As a know-nothing end user it is my expectation that when you run the rustup.rs script, that is "downloading an installer" and it should install the newest stable version. If there's a reason not to upgrade you could at least offer at the prompt (since you already show a prompt) to let me upgrade. (I do not object to the behavior that deleting |
This might be resolved by #2201 which will be part of 1.22.0 - if someone wants to try a build of master and see if this is resolved sufficiently then that'd be super. |
I tried it out, #2201 fixes the issue where if you try installing a toolchain that does not exist on your system. However, if the toolchain exists but is out of date, rustup says |
So part of this is the semantics of the fact that The issue title suggests that a good behaviour change could be to first apply any defaults and component/target selections to the default toolchain, and then to run the equivalent of How do you feel about that as a possible approach? |
I like this approach. A slight issue I have with not doing this on I actually do not think CI will be affected that much by this, however. I think it might be fine to make the change global. Could also do a migration period where |
It'd be surprising for some people who are used to the toolchain not moving on things like github actions, and I've taken a lot of flak in the past for making changes which might make CI behave subtly differently. Heck we still silently "support" the never-official never-not-hidden |
Having said that, I've grown a new flak jacket since then, so perhaps I might take it on the chin and just always update by default. |
haha. If we're concerned we can go with the behavior you suggested for |
Perhaps the answer instead then is to run |
Hmmm. I would prefer |
Okay so current total proposal is: Interactive mode (no Non-interactive mode ( I'm not sure I want issues for people to kvetch on, and this way it's entirely configurable, the only change in "default" behaviour is interactive mode and that includes prompting for users anyway. How does that sit with you as a spec? |
Can you expand on what this would mean? "You have existing out of date toolchains installed, would you like to update them?". It feels like we can unconditionally update but this works too. Non-interactive mode should also print something about this step being skipped. |
I just meant it'd be an option in the install menu prompts like things like default toolchain etc. Probably labelled "Update any existing toolchains?" |
Ah. This seems fine, though I kinda feel like we should just do it (and perhaps mention it is being done). |
If it's something configurable we probably ought to prompt for it, but if you strongly believe that everyone will want it except for rare circumstances then i suppose we can skip the interactive prompting. |
From my end user's perspective: It seems to me that since you are already printing an interactive prompt on a normal run of rustup.sh, there is nothing wrong with offering this option as an interactive prompt. If anything it is good because showing the interactive prompt means I know what rustup.sh did for certain without having to look it up. |
That seems fine. Perhaps we should have the default option (i.e. what happens when you press enter without selecting Y or N) be Y? I don't have a strong feeling here, I just think the UX would be smoother. But it's not a big deal. |
The interactive prompting means that'd be effectively the case by default yes. I have a draft implementation, I'm just running the test suite then I'll push a PR for your perusal. It adds a |
Oh, but if the install was clearly from scratch (i.e. no toolchains before the install) it doesn't do either, even if specified, since there's literally no point. |
FWIW I think that we should think about the user model here, decide what a good outcome would be, and then decide how to get there, internet opinions are just data for that. So specifically for me:
Should a user need to know whether rustup is already installed or not in order to achieve their desired outcome? I don't think that makes sense. If they are running the installer, they have declared their desired outcome already; and rustup should just strive to make it happen. -> I don't think we should prompt. I think we should just update if needed. |
I'm happy with the idea that we do the update without prompting, and use |
I think just the tool chain we're installing would make sense: we haven't been invoked in as broad a context as 'rustup update'. |
Okay, so the approach will be:
Does that sound good to all concerned? (i.e. stop worrying about the CI people whining if their expectation fails) |
This sounds good to me. |
#2339 is merged and will be in 1.22.0 |
Problem
(h/t @mcclure for finding this rather annoying bug)
If you are on a system with an older
.rustup
folder, with or without a.cargo
, rustup will declare success unconditionally, without updating anything. This also happens even if you request a default profile that is not installed, and, curiously, if you attempt to install rustup with a different default toolchain.Steps
stable
. I did this by installing rust 1.8, and then moving the toolchain and update-hashes folder. Make surerustup update
attempts to update, but don't let it finish.info: updating existing rustup installation
, but nothing will actually be updated..cargo
. Try again. It will recreate the.cargo
folder, but again not update anything.The last step can be tested on an existing rustup install pretty easily (for testing the others I had to create a docker container), by trying to install rustup with the default set as beta (if you don't have beta installed).
Possible Solution(s)
I feel like we should have some additional prompts for the user here.
If the user is attempting to install a default toolchain that does exist in
.rustup
but is out of date, we should tell them: "You have an existing outdated toolchain installed in~/.rustup
. Would you like to update it? y/n"If the user is attempting to install a default toolchain that does not exist in
.rustup
, we should just install it, and set it as default.Notes
Output of
rustup --version
: rustup 1.21.1 (7832b2e 2019-12-20)Output of
rustup show
:cc @kinnison
The text was updated successfully, but these errors were encountered: