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

Find and correct all places recommending rustup install and rustup uninstall #2148

Closed
3 tasks done
kinnison opened this issue Dec 8, 2019 · 11 comments
Closed
3 tasks done
Labels
E-easy help wanted tracking This is a tracking issue
Milestone

Comments

@kinnison
Copy link
Contributor

kinnison commented Dec 8, 2019

We want to deprecate rustup install and rustup uninstall in 1.21.0 -- as such as indicated in #2096 (comment) we need to find and correct anywhere major which recommends the use of these deprecated interfaces.

NOTE Our ideal outcome is that the rustup install and rustup uninstall CLI API remains available into the future as a pure alias to rustup toolchain XXX which currently due to limitations in clap it is not.

ALSO NOTE We want the "correct" CLI API documented, so this work remains wanted.

FINALLY With #2149 we intend to not emit a warning but rather only verbose indicators that the current rustup install and rustup uninstall may not behave exactly as the rustup toolchain XXX interface and that will change when the alias work is done.

This issue is meant to track that documentation fixup work:

  • The Book, for example here
  • The edition guide, for example here
  • Miri's README, for example here

Until all those (and any further high profile instances) are resolved, we should not release 1.21.0

If you find new instances of rustup install and rustup uninstall in high profile documentation, please comment below so we can track it. Ditto if you file a PR to get that fixed to use rustup toolchain install etc, then please comment so that we can track the PR.

@trolleyman
Copy link
Contributor

trolleyman commented Dec 8, 2019

Please don't do this -- this makes rustup less easy to use! Just check the comments of https://www.reddit.com/r/rust/comments/e7rer9/we_need_your_help_before_rustup_1210_can_be/

Can someone explain what the main reason is for this change?

@AxelMontini
Copy link

IMO it doesn't make any sense.

  • rustup override overrides the default toolchain.
  • rustup check checks for toolchain updates.
  • and so on

Either:

  1. We group every toolchain-related command under rustup toolchain
  2. We remove rustup toolchain and keep everything as a rustup command.

I find that separating toolchain-related commands (some under rustup toolchain, others not) just makes the tool less intuitive to use.
I prefer the current situation (or the second option I listed) over this.

bors added a commit to rust-lang/miri that referenced this issue Dec 8, 2019
Updated README to reflect deprecation of rustup install

This PR from the rustup repository brought me here: rust-lang/rustup#2148

TL;DR With rustup 1.21.0 `rustup install` and `rustup uninstall` are being deprecated in favor of `rustup toolchain install` and `rustup toolchain uninstall`. There's plenty of code/documentation out there that needs to be updated to reflect this.

Thought It would be cool to help, however small the change may be. :)

Keep up the great work!
bors added a commit to rust-lang/miri that referenced this issue Dec 8, 2019
Updated CI config to reflect deprecation of rustup uninstall

In the same spirit as #1110 😄

This PR from the rustup repository brought me here: rust-lang/rustup#2148

TL;DR With rustup 1.21.0 `rustup install` and `rustup uninstall` are being deprecated in favor of `rustup toolchain install` and `rustup toolchain uninstall`. There's plenty of code/documentation out there that needs to be updated to reflect this.

Thought It would be cool to help, however small the change may be. :)

Keep up the great work!
@brson
Copy link
Contributor

brson commented Dec 8, 2019

I would encourage careful deliberation about actually removing these commands. Maybe it is best to actively discourage their use, in the hopes of removing them someday, though I am not sure. I am doubtful about removing them completely in the near term.

These commands are commonly used, are possibly more intuitive than the alternative, and have effectively no maintenance burden. Since rustup is used by nearly the entire rust community, any disruption it causes will have a negative effect on goodwill.

Some tools need to "just work", and sometimes have to carry backwards-compatibility baggage to do so.

@pantsman0
Copy link

A tool should never never take away a feature that isn't a significant bug.

It is very simple. If you teach users that your tool breaks when you update, they will not update.

@kennytm
Copy link
Member

kennytm commented Dec 8, 2019

Rather than implementing rustup install and rustup uninstall as hard-coded commands like #2096, perhaps rustup could adapt cargo's "[alias]" system and handle install/uninstall as a part of this.

@kinnison
Copy link
Contributor Author

kinnison commented Dec 8, 2019

For those coming here to ask about stopping the removal of the commands -- it's very likely they will remain deprecated (and emitting a warning) for some time because yes, it would be madness to remove them too quickly. I quite like @kennytm 's idea of aliases, that'd be a very neat way to support these without the complexity inherent in multiple pathways to the same internal behaviour which is what we have right now.

Part of the rationale behind moving away from toolchain management commands not being under toolchain is that we may want to integrate some of rustup and cargo's behaviour in the future. To do that, we need to begin the process of cleaning up rustup's CLI. This is a start of that.

bors bot added a commit to rust-lang/rust-analyzer that referenced this issue Dec 8, 2019
2504: Use rustup toolchain install instead of rustup install r=matklad a=edwin0cheng

`rustup install` and `rustup uninstall` are going to be deprecated in rustup 1.21.0

See rust-lang/rustup#2148

and https://www.reddit.com/r/rust/comments/e7rer9/we_need_your_help_before_rustup_1210_can_be/



Co-authored-by: Edwin Cheng <[email protected]>
@ChrisJefferson
Copy link

While I see why this would have been a good idea from the start, it is I think too late now.

As you say, huge numbers of physical books have been sold with "rustup install". They can't be "fixed".

Google for "rustup install" and you can find thousands of links that need fixing, while "rustup toolchain install" is on only a tiny fraction of pages.

Is not maintaining "rustup install" worth fixing all these guides, and breaking the ones which inevitably won't be fixed, including physical books?

@CAD97
Copy link

CAD97 commented Dec 9, 2019

Even if we want cargo to adopt some of rustup, rustup's primary purpose remains toolchain management. It's for that reason that the rustup install -> rustup toolchain install alias makes sense. If cargo adopts toolchain management, it would go under cargo toolchain install, of course.

Of the currently documented by --help commands, update, default, and override all would pretty clearly make sense under rustup toolchain. Even e.g. rustup target could fit under rustup toolchain as e.g. rustup toolchain nightly target add all or similar. I hate to argue a slippery slope here, but the point I think the pushback is trying to articulate is that manipulating the toolchain installs is the "default" mode of rustup. Someone could theoretically publish cargo-toolchain and just forward to rustup, and it would make sense. (As in cargo toolchain --- => rustup ---.)

I fully agree that rustup install should "just" be an alias for rustup toolchain install, and not be a fully separate entry point. But it makes sense for the alias to exist, and for the primary function of rustup to be at the first level of functionality.

So, I guess I'd argue this isn't a technical issue, but a social one. Especially with the r/rust post, it kind of felt like "hey, this thing everyone's learned to use? It was a mistake and we're removing it." I think the response clearly indicates there's a lot of desire for the alias to stay, so it'd "just" be a matter of saying "ok, it stays, as an alias for the proper rustup toolchain install," and then everyone can be happy with the situation.

Even if cargo adopts (some or all of) the capabilities of rustup, there's nothing saying that it needs to adopt the CLI verbatim, either. It's a different interface, with a different primary purpose, so it makes sense if how you get to the functionality differs a bit.

@kinnison
Copy link
Contributor Author

kinnison commented Dec 9, 2019

@ChrisJefferson your point about printed books is good. I was not proposing removing the command at all, but I can see that new users might be put off by a warning when they use it. This point, along with the surprising (to me, but likely not to others) pushback on the deprecation means that I'm considering changing the warning to a verbose info that things might not behave like the toolchain install subcommand. That can be removed once we have true alias support properly set up.

@CAD97 Thank you for opening clap-rs/clap#1603 -- that would indeed solve the aliasing issue nicely for us.

I still want all the online docs updating to the true command.

I've opened #2149 to track dealing with the above warning demotion if anyone would like to submit a PR against it.

@trolleyman
Copy link
Contributor

@kinnison Thank you for changing your mind

zakaluka added a commit to zakaluka/book that referenced this issue Dec 15, 2019
zakaluka added a commit to zakaluka/edition-guide that referenced this issue Dec 15, 2019
@kinnison kinnison removed this from the 1.21.0 milestone Dec 18, 2019
@kinnison
Copy link
Contributor Author

Because of #2149 I've taken this out of the 1.21.0 milestone, unblocking the release.

@kinnison kinnison added this to the 1.22.0 milestone Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy help wanted tracking This is a tracking issue
Projects
None yet
Development

No branches or pull requests

8 participants