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

Make rustup install a true alias of rustup toolchain install. #2096

Closed
wants to merge 1 commit into from

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Oct 29, 2019

This makes rustup install exactly the same as rustup toolchain install, adding the --component and --target flags.

@ehuss
Copy link
Contributor Author

ehuss commented Oct 29, 2019

@lzutao and I were a bit confused when rustup install did not have the same options as rustup toolchain install (rust-lang/reference#705 (comment)). This is a proposal to make them the same.

Another idea is to make rustup update the same as well (with an optional toolchain), so that it can gain the extra flags (profile, components, and targets), but I'm not sure if "rustup update" is intentionally hobbled.

@kinnison
Copy link
Contributor

rustup install is deprecated and may be removed in a future version of rustup I'd rather see an MR which makes rustup install direct people to rustup toolchain install instead.

@bors
Copy link
Contributor

bors commented Oct 29, 2019

☔ The latest upstream changes (presumably #2097) made this pull request unmergeable. Please resolve the merge conflicts.

@kinnison
Copy link
Contributor

I'm tempted to make it so that rustup update is also not an alias for rustup toolchain install as such because the problems with maintaining so many aliases for the same functionality are manifest in that you had this confusion in the first place.

@tesuji
Copy link
Contributor

tesuji commented Oct 30, 2019

I think for each deprecated commands, a warning should be display whenver it is used
and warn that this command will be removed in future version. And if possible,
suggest which command to use instead.

@kinnison
Copy link
Contributor

@lzutao I agree, I'm going to see if I can write such a thing tonight, and go through our CLI and annotate any deprecated stuff so that we get a good warning with help for the user.

@kinnison
Copy link
Contributor

I've pushed #2100 which I'd like to see used instead of this. Are you OK with that?

@ehuss
Copy link
Contributor Author

ehuss commented Nov 1, 2019

I've pushed #2100 which I'd like to see used instead of this. Are you OK with that?

Not sure if this question is directed at me. I think with deprecating, I would be concerned that these are often used in CI where nobody will see the warnings.

Also, rustup install is often used in documentation and web pages all over. For example, in TRPL, the Edition Guide, Clippy, Miri, and RLS. I think at least updating all those should be done if it is to be deprecated.

I'm more swayed by the argument that "rustup install" can be confusing or ambiguous (does it install a toolchain, component, or target?), and less with the maintenance burden of keeping them in sync (which this PR does).

I don't have a strong preference either way, they both have their pros and cons. I just suggest, if it is deprecated, to make a good effort to scrub references to it.

@kinnison
Copy link
Contributor

kinnison commented Nov 1, 2019

Scrubbing references makes sense. I will set-to looking at what needs to be done to amend all of those. The ambiguity point also stands, I'd not though in that mode because I approach rustup from a maintainer perspective rather than from a user perspective -- that's handy for me, thank you.

@tesuji
Copy link
Contributor

tesuji commented Nov 1, 2019

Or we could totally remove these deprecated aliases, and just suggest which
to use then return non-zero status code whenever these aliases used.
I think that better because it would make any uses of these aliases fail intermediately
so that people use it on CI will notice this change.

@kinnison
Copy link
Contributor

kinnison commented Nov 3, 2019

@lzutao I think the rough approach should be:

  1. Merge the deprecation warnings
  2. Make PRs to get all places still recommending the deprecated approaches updated
  3. Release a version of rustup with the deprecation warnings (Release notes should explain this)
  4. Wait at least a little while
  5. Merge something turning the warnings into errors, release a rustup with that
  6. Wait a while
  7. Merge something to remove the unwanted CLI content etc.

I'd rather not break people's CI without giving them at least a little while to update first.

@kinnison
Copy link
Contributor

kinnison commented Dec 8, 2019

With the tracking issue open, I'm closing this PR. Thanks again everyone, and hopefully we can work together to find and correct all the bad examples before we release 1.21.0

@kinnison kinnison closed this Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants