rustup icon indicating copy to clipboard operation
rustup copied to clipboard

`rustup show` should not force-install the default toolchain if it is not installed.

Open johnthagen opened this issue 6 years ago • 31 comments

This behavior confused me:

$ rustup uninstall nightly-2018-04-01-x86_64-apple-darwin
info: uninstalling toolchain 'nightly-2018-04-01-x86_64-apple-darwin'
info: toolchain 'nightly-2018-04-01-x86_64-apple-darwin' uninstalled
$ rustup show
Default host: x86_64-apple-darwin

info: syncing channel updates for 'nightly-2018-04-01-x86_64-apple-darwin'
info: latest update on 2018-04-01, rust version 1.26.0-nightly (517f24025 2018-03-31)
info: downloading component 'rustc'
 57.3 MiB /  57.3 MiB (100 %)   7.8 MiB/s ETA:   0 s                
info: downloading component 'rust-std'
 46.2 MiB /  46.2 MiB (100 %)   7.4 MiB/s ETA:   0 s                
info: downloading component 'cargo'
  3.0 MiB /   3.0 MiB (100 %)   1.5 MiB/s ETA:   0 s                
info: downloading component 'rust-docs'
  6.8 MiB /   6.8 MiB (100 %)   3.1 MiB/s ETA:   0 s                
info: installing component 'rustc'
info: installing component 'rust-std'
info: installing component 'cargo'
info: installing component 'rust-docs'
installed toolchains
--------------------

stable-x86_64-apple-darwin
nightly-i686-apple-darwin
nightly-x86_64-apple-darwin (default)

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

nightly-2018-04-01-x86_64-apple-darwin (directory override for '/Users/user/GitHub/unrust')
rustc 1.26.0-nightly (517f24025 2018-03-31)

Is rustup show supposed to actually start installing components? I was under the impression that it was just for listing out what you have installed.

I'm on a macOS 10.12.6 host.

johnthagen avatar Apr 17 '18 14:04 johnthagen

The behaviour here comes from the fact that the default toolchain was uninstalled. Rustup forces the default toolchain to exist in many places semi-automatically. This is done to reduce surprise in a number of places, but has the side-effect of being surprising here. Perhaps we need an info: Default toolchain not installed, attempting to install... which would make it clear why it's happening?

kinnison avatar Nov 10 '19 09:11 kinnison

@kinnison Personally I think a command like show should never actually install something and change the user's environment. That just seems very odd to me and I can't think of another example of a tool that does something like that.

johnthagen avatar Nov 16 '19 20:11 johnthagen

@johnthagen That's fair, I have retitled the issue and I'm happy to mentor yourself or anyone else through the process of determining why this happens, and what we can do to improve the UX and ensure it doesn't happen in future. This is nominally a breaking change in how rustup show works, but frankly I think it's right to do it.

If anyone wants help with working this one through to a PR, please comment here or find me on #wg-rustup on our Discord.

kinnison avatar Nov 17 '19 10:11 kinnison

@kinnison anyone doing this?

SummerGram avatar Mar 21 '20 17:03 SummerGram

@SummerGram I think that noone is working on this per-se. It should be fairly easy to work out what's going on after recent refactoring done by Robert.

kinnison avatar Mar 21 '20 17:03 kinnison

What about rustup --version or rustup +toolchain --version ?

kellda avatar Feb 11 '21 10:02 kellda

I think if you explicitly say +toolchain then you're saying you want that to be installed.

kinnison avatar Feb 12 '21 19:02 kinnison

I'm still unsure what is the right behaviour: which commands of the following should install a toolchain ?

  • rustup --version
  • rustup +toolchain --version
  • rustup show
  • rustup +toolchain show
  • rustup show active-toolchain
  • rustup +toolchain show active-toolchain

What about healing damaged toolchains ?

Furthermore, there are actually tests (*_not_installed in cli-rustup.rs) that check that rustup show installs the toolchain.

kellda avatar Feb 13 '21 09:02 kellda

cc #1250 for the tests cc #2252 for always installing toolchains

kellda avatar Feb 13 '21 09:02 kellda

I think, for each case we need to think about whether or not having the toolchain absent would cause confusion or breakage in scripting which may already exist and rely on behaviour. I'm prepared to introduce some breaking change so long as it's very well thought out.

In my view, if you are explicitly stating a toolchain via +toolchain or via a rust-toolchain file then we should always install that since I know some people rely on that behaviour in CI (i.e. running rustup show as part of their CI to both ensure the toolchain is present, and to display the toolchain information in the CI logs, but IIRC the projects I know doing this all have rust-toolchain files).

I also think if you're explicitly asking questions of the toolchain then it should be installed if absent (rustup show active-toolchain).

Of the examples you gave, I think that means a plain rustup --version and a plain rustup show (plain in the sense of no +toolchain and no rust-toolchain override file) are the only ones I'd entertain not force-installing the toolchain for. And they should emit a warning if the toolchain is absent.

All in all this is a somewhat confusing situation to be in, made worse because people are relying on the theoretically 'odd' behaviour.

kinnison avatar Feb 16 '21 09:02 kinnison

I think we should break things. Not cavalierly, but carefully, perhaps even with an RFC. But we have 'install', and we can make that idempotent. We can make a version of install that honours toolchain files etc. But (ab)using show to trigger an install is just counter intuitive and will be an ongoing source of WTFs forever.

rbtcollins avatar Feb 16 '21 20:02 rbtcollins

If we are going to make such a breaking change I agree that an RFC would be best. If we're to file an RFC we need to think through all the implications of disabling implicit installation for the various commands and decide which retain implicit installation and which will no longer support it.

As a starter for ten, I think the following must continue to support implicit installation:

  • Any invocation of rustup or one of the proxies (rustc cargo etc) which has a user-provided +toolchain argument
  • Any invocation of rustup or one of the proxies (rustc cargo etc) which is done in a directory with an explicitly set override (whether that be by rustup set override or by rust-toolchain)
  • rustup default $foo

I'm sure there's other places where implicit install is still preferable, but as a starting point, would people agree that the above should include it?

kinnison avatar Feb 17 '21 09:02 kinnison

It doesn't make intrinsic sense to me that merely adding +toolchain to a command causes an install.

I think of rustup as an API, and there are metadata operations such as show and version which should strictly show what the state of the system is without mutating it; operations such as as 'default' that mutate metadata and also shouldn't obviously install, but perhaps the ergonomics of ease of ease make 'default stable' doing an install desirable - I'm not a purist on this model - and then operations like install /update/target add, where the operation is doing the install, and finally anything via a proxy where the implicit install is super useful, but not the goal of the command.

And the implicit install in that last case bites us in some places such as the docker bugs we get from time to time where implicit toolchain installation will break rather than erroring with a 'not installed' error which would allow direct debugging of the situation (obviously supporting docker layers would also help :P).

Having used goenv/pyenv etc a lot, I really do quite like implicit install, so I think even with the occasional bite we probably should keep it; but it should be very clear and reasoned where it triggers.

I would suggest:

  • any proxy invocation, regardless of toolchain file or arguments
  • nothing else

rbtcollins avatar Feb 20 '21 16:02 rbtcollins

I think that's a reasonable compromise. I will note though that not autoinstalling on rustup default XXX would result in a substantial sweeping change (though not a complex one) across the entire test codebase.

kinnison avatar Feb 20 '21 16:02 kinnison

It might be nice for instance to have a 'please install whatever the current toolchain is' command, maybe via options to install?

Rather than having folk run 'rustup rustc' to trigger autoinstalls when using toolchain files.

rbtcollins avatar Feb 20 '21 17:02 rbtcollins

I can almost imagine that as rustup toolchain install . (effectively use . to mean 'whatever I need to build this here'.

kinnison avatar Feb 20 '21 17:02 kinnison

My only concern about that is that what-if we wanted to permit installing a toolchain from a dir/tarball etc in future. We'd need to make sure that we can discriminate effectively. Probably we can but lets think it through.

rbtcollins avatar Feb 20 '21 20:02 rbtcollins

I think installing from a dir is more rustup toolchain link and we could always do tarball installations from rustup toolchain unpack or similar. The verb doesn't always have to be install IMO.

kinnison avatar Feb 21 '21 10:02 kinnison

We could look for a new verb for this though like 'ensure' or something.

link doesn't copy a toolchain in, so its quite different. I agree that tarballs can use different verbs.

rbtcollins avatar Feb 21 '21 11:02 rbtcollins

Hmm I like ensure that's nice.

kinnison avatar Feb 21 '21 12:02 kinnison

I can almost imagine that as rustup toolchain install .

I think rustup toolchain install . may be confusing, as I would think . should be a toolchain or point to a toolchain. Also I would think I could use whatever path in place of ., e.g. git clone [someproject] && rustup toolchain install [someproject] instead of git clone [someproject] && cd [someproject] && rustup toolchain install .

We could look for a new verb for this though like 'ensure' or something.

I've thought about 'auto-install' although ensure is also good.

Rather than having folk run 'rustup rustc' to trigger autoinstalls when using toolchain files.

rustc --version or cargo --version would also work and is maybe more easy / common. That said it still would be nice to have an 'ensure' or 'auto-install' rustup command

kellda avatar Feb 22 '21 11:02 kellda

Regarding implicit installs, as a user it seems obvious to me that commands that I expect be purely informational wouldn't modify my system under any circumstances: rustup show rustup check rustup which rustup --version rustup --help

It wouldn't really bother me to have commands that already modify the behaviour of my system in some way automatically install things, and would often be a pleasant convenience: rustup run rustup ensure? rustup update rustup target rustup default rustup toolchain rustup component rustup override

I think rustup doc could go either way.

ickk avatar Apr 11 '21 10:04 ickk

Is anyone actually working on this? This is very surprising behavior and I would have expected three and a half years to be plenty of time to address it. I realize that everyone is busy, is there anything I can do to assist?

Is there agreement on @ickk's list of commands that should not modify the environment? Does anyone have comments on @kellda's PR #2658?

couchand avatar Nov 24 '21 16:11 couchand

@couchand This is quite a controversial and intricate thing which needs careful thought to address because unlike other places where breaking changes are version controlled and can be handled more easily, if we break rustup's behaviour, anyone using rustup has to change their systems to cope. Rustup's purpose is to ensure that toolchains are available so while I personally agree that it's slightly surprising that rustup show or even rustup --version might install a toolchain, it's not entirely surprising to everyone.

We move very slowly on UI changes in Rustup where they would run the risk of breaking others. At a personal level I feel pretty good about @ickk's suggested list, but we do need to think and time for thinking is quite sparse right now :(

As for #2658 I don't think we should be going ahead with the changes until we've decided what the right changes are.

kinnison avatar Dec 27 '21 10:12 kinnison

I'm entirely sysmpathetic @kinnison about the need to think through the implications for many of the commands. The above discussion makes clear that there are a great many different edge cases.

I'd propose moving the ball forward by focusing on a non-controversial subset. Personally, @ickk's first group seems to fit the bill, though I can see how changing show, check, or which are possibly controversial -- a script could conceivably rely on one of those, despite the strangeness.

How about just rustup --version and rustup --help? I can't see how those could possibly be controversial, so perhaps we can make some concrete progress on this issue by first just fixing rustup --version (it appears --help works fine already)?


As an aside, from that PR I landed on this comment in the implementation of rustup show:

https://github.com/rust-lang/rustup/blob/a9d0c67d4aaa74863fbd56962cb83e2e207f607d/src/cli/rustup_mode.rs#L1055

git blame dates that line to #2252, which changed the call in show from find_override_toolchain_or_default to find_or_install_override_toolchain_or_default. Now, since that PR post-dates this issue, presumably that didn't change the behavior, but I'm not immediately seeing why not.

In any case I'd agree with the comment :smile:


@kinnison regarding your comment here: https://github.com/rust-lang/rustup/issues/1397#issuecomment-779702076

In my view, if you are explicitly stating a toolchain via +toolchain or via a rust-toolchain file then we should always install that since I know some people rely on that behaviour in CI (i.e. running rustup show as part of their CI to both ensure the toolchain is present, and to display the toolchain information in the CI logs, but IIRC the projects I know doing this all have rust-toolchain files).

I also think if you're explicitly asking questions of the toolchain then it should be installed if absent (rustup show active-toolchain).

You're somewhat describing how I landed on this issue. I was trying to debug a toolchain issue in a deeply-nested directory of an unfamiliar project. I was unaware of the presence of the rust-toolchain file many directories higher, and was surprised that the query of rustup show started trying to reach the internet.

If rustup show active-toolchain isn't the right way to ask my question, I'm happy to hear that there's another. But it seems to me that there should be some command to ask rustup, "what toolchain do you want to use to compile this project" without asking for it to be installed.

couchand avatar Dec 27 '21 16:12 couchand

We should also make sure that ~/.rustup is not written to by show etc.

rbtcollins avatar Mar 11 '22 06:03 rbtcollins