Make `rustup show` output info in a more logical order

[majaha]

I ran rustup show and noticed that its output is in the order: installed toolchains -> installed targets for active toolchain -> active toolchain I thought it would make sense to put "active toolchain" before " installed targets for active toolchain" so that they in order of decreasing scope: installed toolchains -> active toolchain -> installed targets for active toolchain so that is what this patch does 🙂 Thoughts?

Before:

Matt@Matts-PC /c/d/p/r/rustup (show-order)> rustup show
Default host: x86_64-pc-windows-msvc
rustup home:  C:\Users\Matt\.rustup

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

stable-x86_64-pc-windows-msvc (default)
nightly-x86_64-pc-windows-msvc

installed targets for active toolchain
--------------------------------------

wasm32-unknown-unknown
x86_64-pc-windows-msvc

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

stable-x86_64-pc-windows-msvc (default)
rustc 1.67.1 (d5a82bbd2 2023-02-07)

Matt@Matts-PC /c/d/p/r/rustup (show-order)>

After:

Matt@Matts-PC /c/d/p/r/rustup (show-order)> RUSTUP_HOME=home CARGO_HOME=home home/bin/rustup show
Default host: x86_64-pc-windows-msvc
rustup home:  D:\programming\repos\rustup\home

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

stable-x86_64-pc-windows-msvc (default)
nightly-x86_64-pc-windows-msvc

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

stable-x86_64-pc-windows-msvc (default)
rustc 1.67.1 (d5a82bbd2 2023-02-07)

installed targets for active toolchain
--------------------------------------

wasm32-unknown-unknown
x86_64-pc-windows-msvc

Matt@Matts-PC /c/d/p/r/rustup (show-order)>

Fixes https://github.com/rust-lang/rustup/issues/1397

[rbtcollins]

My gut reaction is that folk usually want the active toolchain information, so having it first or last is best; and last means they don't need to scan far for it or use head.

Neither first nor last will match your concept though.

Thanks for demonstrating the idea clearly though!

[majaha]

Thanks for considering it!

I've fixed the tests I broke, amended the commit and commit message and force pushed. Apologies to anyone who pulled the original commit.

[majaha]

There's always rustup show active-toolchain [-v], although it is admittedly longer

Matt@Matts-PC /c/d/p/r/rustup (show-order)> rustup show active-toolchain
stable-x86_64-pc-windows-msvc (default)
Matt@Matts-PC /c/d/p/r/rustup (show-order)> rustup show active-toolchain -v
stable-x86_64-pc-windows-msvc (default)
rustc 1.67.1 (d5a82bbd2 2023-02-07)

[majaha]

As an aside, would it be possible to reformat the reason the active toolchain was chosen? At the moment the floating (default) is a little misleading and mysterious if you don't already know what it means. And it visually matches how it looks in "installed toolchains" even though it means something different. (My initial reaction to it was "Why is there a default active toolchain if there can only be one active toolchain? 🤔" but of course, that's not what it means 🙂).

active toolchain
----------------
stable-x86_64-pc-windows-msvc (default)
rustc 1.67.1 (d5a82bbd2 2023-02-07)

->

active toolchain
----------------
stable-x86_64-pc-windows-msvc
rustc 1.67.1 (d5a82bbd2 2023-02-07)
Reason: is the default toolchain
/or/
Reason: overridden by +toolchain on the command line

Also, there's some scope for code deduplication between show_active_toolchain() and show()

[workingjubilee]

I feel like "\nReason:" is nondescript (reason for what? it's dissociated from the phrase "active toolchain" by multiple lines by that point) and the " (inline comment)" style is better, but I don't think " (default)" is a good explanation either.

[majaha]

It's a little terse, I'll admit. I think because it's right next to the other parts of the active toolchain, you can work out what it's the reason for using context from the reason message. But I would be open to:

active toolchain
----------------
stable-x86_64-pc-windows-msvc
rustc 1.67.1 (d5a82bbd2 2023-02-07)
Reason it's active: it's the default toolchain

Either way, it's certainly more descriptive than the other lines! Perhaps it should really be:

active toolchain
----------------
Toolchain name: stable-x86_64-pc-windows-msvc
Compiler version: rustc 1.67.1 (d5a82bbd2 2023-02-07)
Reason it's active: it's the default toolchain

[workingjubilee]

Hm. Why not reframe it and instead

active toolchain: default
----------------
stable-x86_64-pc-windows-msvc
rustc 1.67.1 (d5a82bbd2 2023-02-07)

[rbtcollins]

What would happen if we put the reason on the same line as Active toolchain.

Active toolchain (the default) Active toolchain (selected by RUSTUP_TOOLCHAIN) ...

(or something similar).

Having thought more about this I do like the idea of making targets subordinate to the toolchain. Perhaps like this:

RUSTUP_HOME=home CARGO_HOME=home home/bin/rustup show
Default host: x86_64-pc-windows-msvc
rustup home:  D:\programming\repos\rustup\home

installed toolchains
===================

stable-x86_64-pc-windows-msvc (default)
nightly-x86_64-pc-windows-msvc

active toolchain
================

channel: stable-x86_64-pc-windows-msvc
version: rustc 1.67.1 (d5a82bbd2 2023-02-07)
reason: default
targets: 
--------

wasm32-unknown-unknown
x86_64-pc-windows-msvc

[majaha]

Yes, that's definitely an improvement!

Although I think we could do away with the blank lines after the ===== and ----- lines.

[djc]

@majaha would you be able to rebase this and follow up on the discussion so far?

[majaha]

Okay, I've rebased the old patch.

We're probably not ready to merge yet though, as we were still discussing and iterating on what we wanted to do. I'm going to have another think about what we want here.

[majaha]

Okay, I've spent some more time thinking about the output format, incorporating some other's ideas, and this is what I've come up with:

$ rustup show
Default host: x86_64-pc-windows-msvc
rustup home:  C:\Users\Matt\.rustup

installed toolchains
====================
stable-x86_64-pc-windows-msvc (default)
nightly-x86_64-pc-windows-msvc

active toolchain
================
name: stable-x86_64-pc-windows-msvc
compiler: rustc 1.73.0 (cc66ad468 2023-10-03)
active because: overridden by +toolchain on the command line
targets:
--------
wasm32-unknown-unknown
x86_64-pc-windows-msvc

I thought about putting the active reason in the header like this:

active toolchain [by default]
================

but decided against it as the rust-toolchain file override reason can be arbitrarily long, and will probably wrap for some people, causing some headache with what to prevent the wrapped ==== line being ugly:

active toolchain [overridden by 'D:\foo\bar\baz\...\rust-toolchain']
================

We should also change the format of rustup show active-toolchain and split it into two lines:

$ RUSTUP_TOOLCHAIN=nightly rustup show active-toolchain
nightly-x86_64-pc-windows-msvc (environment override by RUSTUP_TOOLCHAIN)

⬇

$ RUSTUP_TOOLCHAIN=nightly rustup show active-toolchain
nightly-x86_64-pc-windows-msvc
active because: environment override by RUSTUP_TOOLCHAIN

[rami3l]

$ RUSTUP_TOOLCHAIN=nightly rustup show active-toolchain
nightly-x86_64-pc-windows-msvc
active because: environment override by RUSTUP_TOOLCHAIN

Indeed, breaking that into two lines might be a good idea. @majaha, it looks like we are moving in the right direction!

active toolchain [overridden by 'D:\foo\bar\baz\...\rust-toolchain']
================

I still prefer using active because: here.

However, I'd like to add an (active) in the first section, which is a deliberate duplication but will hopefully make things clearer:

$ rustup show
Default host: x86_64-pc-windows-msvc
rustup home:  C:\Users\Matt\.rustup

installed toolchains
====================
stable-x86_64-pc-windows-msvc (active)
nightly-x86_64-pc-windows-msvc (default)

active toolchain
================
name: stable-x86_64-pc-windows-msvc
compiler: rustc 1.73.0 (cc66ad468 2023-10-03)
active because: overridden by +toolchain on the command line
targets:
--------
wasm32-unknown-unknown
x86_64-pc-windows-msvc

... and we might want to use (active, default) for the non-overridden state.

[rami3l]

@rbtcollins @djc Instead of changing to = and - combined as proposed above, what if we keep the -, and add indentation to the mix (IIRC it should be 2 spaces to be coherent)?

<snip>

active toolchain
----------------
name: stable-x86_64-pc-windows-msvc
compiler: rustc 1.73.0 (cc66ad468 2023-10-03)
active because: overridden by +toolchain on the command line
targets:
  wasm32-unknown-unknown
  x86_64-pc-windows-msvc

Other than this I have no outstanding concerns at the moment.

[majaha]

I still prefer using active because: here.

Sorry, do you mean that you want it to be on the same line as the header next to active toolchain, or that you want it to be on it's own line underneath?

[rami3l]

I still prefer using active because: here.

Sorry, do you mean that you want it to be on the same line as the header next to active toolchain, or that you want it to be on it's own line underneath?

A separate line underneath, just like in my snippets above.

[majaha]

Ah, gotcha. Yeah, I think you're version looks quite clean!

[djc]

This looks good to me. So can we get the code updated to match the proposed style?

[majaha]

Okay, I'm working on this.

Here's a thought I had: Should we always have the installed toolchains and active toolchain -> installed targets lists, even if they have only a single entry? I think that would be clearer and more intuitive. If you haven't seen them before, it's not obvious that their non-presence is giving you information. It's too implicit, you have to go and carefully read the help page to pick up on it.

[rami3l]

@majaha As a quick reminder, with the currently proposed style, the output should be like:

$ RUSTUP_TOOLCHAIN=nightly rustup show active-toolchain
nightly-x86_64-pc-windows-msvc
active because: environment override by RUSTUP_TOOLCHAIN

(From https://github.com/rust-lang/rustup/pull/3225#issuecomment-1782058807)

---

... with (active) and (default) in installed toolchains:

$ rustup show
Default host: x86_64-pc-windows-msvc
rustup home:  C:\Users\Matt\.rustup

installed toolchains
--------------------
stable-x86_64-pc-windows-msvc (active)
nightly-x86_64-pc-windows-msvc (default)

active toolchain
----------------
name: stable-x86_64-pc-windows-msvc
compiler: rustc 1.73.0 (cc66ad468 2023-10-03)
active because: overridden by +toolchain on the command line
targets:
  wasm32-unknown-unknown
  x86_64-pc-windows-msvc

(From https://github.com/rust-lang/rustup/pull/3225#issuecomment-1793316750)

---

Also, (active, default) should be used if the default toolchain is active.

(From https://github.com/rust-lang/rustup/pull/3225#issuecomment-1793314607)

[majaha]

I've uploaded the new patch, which includes the changes and a bit of a refactoring:

1. Change the output format of rustup show, to be in a more logical order and have more pleasing formatting
2. Update the formatting of rustup show active-toolchain and rustup toolchain list to match the new rustup show format.
3. rustup +nightly show will no longer install the nightly toolchain if it isn't already installed. Ditto for rustup show active-toolchain. Fixes https://github.com/rust-lang/rustup/issues/1397
4. Fix a bug where the RUSTUP_TOOLCHAIN environment variable took priority over the +toolchain command line option, and add a test for override priority.
5. rustup default no longer errors when there is no default toolchain configured, it prints a message instead.
6. Update the docs, and fix an issue where they incorrectly stated that rust-toolchain.toml config files couldn't specify custom names. (Text last touched here, see commit message: https://github.com/rust-lang/rustup/commit/7bfbd3b96c29898de001cbda7730fa440d35bd3b)

[djc]

@majaha this sounds great, except I'd like to request basically one commit per bullet point, which should make it a lot easier to review this. Do you think that would be feasible?

[majaha]

Ok, I've split out as best as I can. The first one is still quite large, but I can't really split it much further without meticulously reimplementing the bugs that were fixed. The commit message should help though; start by reading through the changes to OverrideCfg, and noting that OverrideReason has turned into ActiveReason, and the rest mostly follows from there.

[majaha]

Looks like a random test failure. Can we run it again?

[rami3l]

@majaha Sorry for the late review! I'm finally gaining bandwidth since a few days ago 💦

[majaha]

No worries, hope you enjoyed your break! I've addressed your comments and uploaded the new version.

[rami3l]

@majaha Looks like the cargo fmt check has failed, would you mind updating your code again?

[majaha]

Ah sorry, there's always something. Should be fixed now.

[rami3l]

~~@djc I've reviewed this PR and it looks ready in its current state.~~ ~~I'd like to merge this PR soon and return to #3501, if that sounds okay to you.~~

Update: After my discussion with @rbtcollins last night we decided to postpone this change to the next milestone to better integrate this PR with the massive changes to be made in #3367 and #3596.

[rbtcollins]

Hi, I know I'm coming to the code aspects of this late.

This PR is not focused on changing the output; it is instead:

• changing how we model toolchain selection internally
• changing the semantics for config setting file operations
• refactoring the show function to decompose it somewhat
• and changing the ordering

And perhaps more I've missed.

I'd like to take each of these separately to allow decent discussion about the merits of each, please.