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#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-toolchainnightly-x86_64-pc-windows-msvcactive because: environment override by RUSTUP_TOOLCHAIN

(From #3225 (comment))

---

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

$ rustup showDefault host: x86_64-pc-windows-msvcrustup home: C:\Users\Matt\.rustupinstalled toolchains--------------------stable-x86_64-pc-windows-msvc (active)nightly-x86_64-pc-windows-msvc (default)active toolchain----------------name: stable-x86_64-pc-windows-msvccompiler: rustc 1.73.0 (cc66ad468 2023-10-03)active because: overridden by +toolchain on the command linetargets: wasm32-unknown-unknown x86_64-pc-windows-msvc

(From #3225 (comment))

---

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

(From #3225 (comment))

[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 rustup show should not force-install the default toolchain if it is not installed. #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: 7bfbd3b)

[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]

Nice, well done on merging this guys!