Skip to content
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

Closed
Manishearth opened this issue May 14, 2020 · 25 comments
Closed

rustup.rs install should update old toolchains #2330

Manishearth opened this issue May 14, 2020 · 25 comments
Labels
Milestone

Comments

@Manishearth
Copy link
Member

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

  1. Get a rustup install on a fresh system with an old stable. I did this by installing rust 1.8, and then moving the toolchain and update-hashes folder. Make sure rustup update attempts to update, but don't let it finish.
  2. Run the rustup string from https://rustup.rs with the default install. It will say info: updating existing rustup installation, but nothing will actually be updated.
  3. Delete .cargo. Try again. It will recreate the .cargo folder, but again not update anything.
  4. Try installing it with a beta or nightly install, a toolchain that is definitely not installed. It will again not install anything. It will also not update the default!

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:

manishearth@bb1876555c6d:~$ rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/manishearth/.rustup

installed toolchains
--------------------

stable-x86_64-unknown-linux-gnu (default)
1.8.0-x86_64-unknown-linux-gnu

active toolchain
----------------

stable-x86_64-unknown-linux-gnu (default)
rustc 1.8.0 (db2939409 2016-04-11)

cc @kinnison

@mcclure
Copy link

mcclure commented May 14, 2020

For the record, here is how I encountered this bug "in the wild":

  • I fiddled with Rust briefly in 2018 but didn't get beyond building someone else's code. I don't know anything about Rust.
  • This week I attempted to build the wgpu-rs examples. There was a cryptic error because my cargo was too old to understand the dependencies.
  • I thought: Cargo is printing errors, maybe something's wrong with my configuration. I deleted ~/.cargo. I now understand this is a bad idea but again I do not know anything about Rust so it seemed like a good idea. Now of course cargo and rustup can no longer be run because the symlinks are gone.
  • A friend told me the original problem looked like my Rust was too old. Well, no problem, it looks like I need to reinstall Rust anyway.
  • I ran the sh script from https://rustup.rs . It showed me a screen that looked much like this https://twitter.com/Argorak/status/1260939388445491201 but it didn't tell me what version it was installing. Because it said it was the default installation I assumed it was installing the newest.
  • It wasn't the newest, it was my version from 2018, and wgpu-rs still does not build. But I didn't know that until a friend told me to check --version. (Then I ran rustup update and everything worked.)

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 ~/.cargo broke my copy of Rust.)

@kinnison
Copy link
Contributor

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.

@Manishearth
Copy link
Member Author

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 info: updating existing rustup installation - leaving toolchains alone. We should check if things are out of date and offer to update in that case.

@kinnison
Copy link
Contributor

So part of this is the semantics of the fact that rustup wasn't always the official installer for rust, so installing rustup and installing rust were not considered synonymous. 2201 fixed some of that but perhaps didn't take it far enough.

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 rustup update --no-self-update once everything else is done. Offering a --no-update-all-toolchains installer option to prevent that. This would be mildly surprising for some CI installs so an option for dealing with that could be that if the installer is running -y (non-interactive) we default to not updating all toolchains, but allow the update to be requested, and if the installer is interactive, we default to updating all toolchains.

How do you feel about that as a possible approach?

@Manishearth
Copy link
Member Author

I like this approach.

A slight issue I have with not doing this on -y installs (despite having suggested this as a fix for the CI issue) is that this makes -ys behavior differ from a default interactive mode. This seems weird to me. If we pick this option we should at least print out "old toolchains found, but did not update, please run rustup update".

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 -y has the behavior I describe for a while and says that this will eventually go away.

@kinnison
Copy link
Contributor

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 rustup install because several whiny internetbabies were sad I might officially deprecate it and make it output a warning if you used it because "A warning might break my CI"

@kinnison
Copy link
Contributor

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.

@Manishearth
Copy link
Member Author

haha.

If we're concerned we can go with the behavior you suggested for -y, with a red warning when it detects an out of date toolchain with a note that this may change in the future. We can also PSA it on the next release.

@kinnison
Copy link
Contributor

Perhaps the answer instead then is to run rustup check after install is completed -- that would list all installed toolchains, what they're at, and whether there's updates available.

@Manishearth
Copy link
Member Author

Hmmm. I would prefer -y have differing behavior over the default case not automatically fixing this. We can have -y still run rustup check and print a red notification (and perhaps suggest that this might be changed in the future (maybe with a link to an issue if people wish to share use cases where they don't want this) with no concrete plans to change it)

@kinnison
Copy link
Contributor

Okay so current total proposal is:

Interactive mode (no -y): Upon completion of installation, run equivalent of rustup update --no-self-update unless user supplies "--no-update-everything" or selects such an option from the menu.

Non-interactive mode (-y): Upon completion of installation, run equivalent of rustup check *unless --update-everything was provided on the CLI in which case run equivalent of rustup update

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?

@Manishearth
Copy link
Member Author

or selects such an option from the menu

and that includes prompting for users anyway

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.

@kinnison
Copy link
Contributor

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?"

@Manishearth
Copy link
Member Author

Ah. This seems fine, though I kinda feel like we should just do it (and perhaps mention it is being done).

@kinnison
Copy link
Contributor

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.

@mcclure
Copy link

mcclure commented May 19, 2020

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.

@kinnison kinnison added this to the 1.22.0 milestone May 19, 2020
@Manishearth
Copy link
Member Author

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.

@kinnison
Copy link
Contributor

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 --post-install flag to rustup-init which takes update check or none and defaults to update on interactive and check on non interactive, unless you explicitly pass it in which case it takes the option you say.

@kinnison
Copy link
Contributor

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.

@rbtcollins
Copy link
Contributor

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:

  • running an installer should install the thing
  • rustup's install is /well known/ as installing both rustup and a default usable rust (that is, you install rustup, you have working rust); and we explicitly support accepting options to the install stage to control the outcome of that process - profiles, target, components.

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.

@kinnison
Copy link
Contributor

kinnison commented Jun 7, 2020

I'm happy with the idea that we do the update without prompting, and use --default-toolchain none to suppress the update, but if we're doing an update at all @rbtcollins should we update everything or just the toolchain we're "installing"?

@rbtcollins
Copy link
Contributor

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'.

@kinnison
Copy link
Contributor

kinnison commented Jun 8, 2020

Okay, so the approach will be:

  1. After installation, if the desired default toolchain was already present, we will proceed as though rustup update $TOOLCHAIN --no-self-update was run.
  2. Users may specify --default-toolchain none to prevent this, or --no-update-default-toolchain if they want to set a default toolchain but do not want to risk it being updated during a rustup installation.

Does that sound good to all concerned? (i.e. stop worrying about the CI people whining if their expectation fails)

@Manishearth
Copy link
Member Author

This sounds good to me.

@kinnison
Copy link
Contributor

#2339 is merged and will be in 1.22.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants