From caf56ccd3549884195bc2b34e0135cb8ce3068b5 Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Fri, 8 Dec 2023 10:36:11 +0000 Subject: [PATCH] Update format of `show` and `show active-toolchain` This changes the format of `rustup show` to be in a more logical order, and changes the format of `rustup show active-toolchain` to match. Also, as suggested in a comment, these commands will no longer install the active toolchain if it is not already installed, as they now call `cfg.find_active_toolchain()` instead of `cfg.find_or_install_active_toolchain()`. This fixes https://github.com/rust-lang/rustup/issues/1397 --- doc/user-guide/src/overrides.md | 5 +- src/cli/rustup_mode.rs | 237 ++++++++++++++--------------- src/config.rs | 4 +- tests/suite/cli_misc.rs | 2 +- tests/suite/cli_rustup.rs | 255 +++++++++++++++++++++----------- 5 files changed, 284 insertions(+), 219 deletions(-) diff --git a/doc/user-guide/src/overrides.md b/doc/user-guide/src/overrides.md index d3cfa564808..bfeef3922e3 100644 --- a/doc/user-guide/src/overrides.md +++ b/doc/user-guide/src/overrides.md @@ -19,10 +19,7 @@ the directory tree toward the filesystem root, and a `rust-toolchain.toml` file that is closer to the current directory will be preferred over a directory override that is further away. -To verify which toolchain is active, you can use `rustup show`, -which will also try to install the corresponding -toolchain if the current one has not been installed according to the above rules. -(Please note that this behavior is subject to change, as detailed in issue [#1397].) +To verify which toolchain is active, you can use `rustup show`. [toolchain]: concepts/toolchains.md [toolchain override shorthand]: #toolchain-override-shorthand diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index 247db71de5f..68a4c94bdc3 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -11,6 +11,7 @@ use clap::{ }; use clap_complete::Shell; +use crate::config::new_toolchain_with_reason; use crate::{ cli::{ common::{self, PackageUpdate}, @@ -39,8 +40,9 @@ use crate::{ names::{ custom_toolchain_name_parser, maybe_resolvable_toolchainame_parser, partial_toolchain_desc_parser, resolvable_local_toolchainame_parser, - resolvable_toolchainame_parser, CustomToolchainName, MaybeResolvableToolchainName, - ResolvableLocalToolchainName, ResolvableToolchainName, ToolchainName, + resolvable_toolchainame_parser, CustomToolchainName, LocalToolchainName, + MaybeResolvableToolchainName, ResolvableLocalToolchainName, ResolvableToolchainName, + ToolchainName, }, toolchain::Toolchain, }, @@ -1081,127 +1083,112 @@ fn show(cfg: &Cfg, m: &ArgMatches) -> Result { let cwd = utils::current_dir()?; let installed_toolchains = cfg.list_toolchains()?; - // XXX: we may want a find_without_install capability for show. - let active_toolchain = cfg.find_or_install_active_toolchain(&cwd); - - // active_toolchain will carry the reason we don't have one in its detail. - let active_targets = if let Ok(ref at) = active_toolchain { - if let Ok(distributable) = DistributableToolchain::try_from(&at.0) { - let components = (|| { - let manifestation = distributable.get_manifestation()?; - let config = manifestation.read_config()?.unwrap_or_default(); - let manifest = distributable.get_manifest()?; - manifest.query_components(distributable.desc(), &config) - })(); - - match components { - Ok(cs_vec) => cs_vec - .into_iter() - .filter(|c| c.component.short_name_in_manifest() == "rust-std") - .filter(|c| c.installed) - .collect(), - Err(_) => vec![], - } + let active_toolchain_and_reason: Option<(ToolchainName, ActiveReason)> = + if let Ok(Some((LocalToolchainName::Named(toolchain_name), reason))) = + cfg.find_active_toolchain(&cwd) + { + Some((toolchain_name, reason)) } else { - // These three vec![] could perhaps be reduced with and_then on active_toolchain. - vec![] - } - } else { - vec![] - }; + None + }; + + let (active_toolchain_name, _active_reason) = active_toolchain_and_reason + .as_ref() + .map(|atar| (&atar.0, &atar.1)) + .unzip(); - let show_installed_toolchains = installed_toolchains.len() > 1; - let show_active_targets = active_targets.len() > 1; - let show_active_toolchain = true; - - // Only need to display headers if we have multiple sections - let show_headers = [ - show_installed_toolchains, - show_active_targets, - show_active_toolchain, - ] - .iter() - .filter(|x| **x) - .count() - > 1; - - if show_installed_toolchains { + let active_targets: Vec = active_toolchain_name + .and_then(|atn| match atn { + ToolchainName::Official(desc) => DistributableToolchain::new(cfg, desc.clone()).ok(), + _ => None, + }) + .and_then(|distributable| { + let manifestation = distributable.get_manifestation().ok()?; + let config = manifestation.read_config().ok()?.unwrap_or_default(); + let manifest = distributable.get_manifest().ok()?; + manifest + .query_components(distributable.desc(), &config) + .ok() + }) + .map(|cs_vec| { + cs_vec + .into_iter() + .filter(|c| c.component.short_name_in_manifest() == "rust-std") + .filter(|c| c.installed) + .collect() + }) + .unwrap_or_default(); + + // show installed toolchains + { let mut t = process().stdout().terminal(); - if show_headers { - print_header::(&mut t, "installed toolchains")?; - } - let default_name = cfg - .get_default()? - .ok_or_else(|| anyhow!("no default toolchain configured"))?; - for it in installed_toolchains { - if default_name == it { - writeln!(t.lock(), "{it} (default)")?; - } else { - writeln!(t.lock(), "{it}")?; - } + print_header::(&mut t, "installed toolchains")?; + + let default_toolchain_name = cfg.get_default()?; + + let last_index = installed_toolchains.len().wrapping_sub(1); + for (n, toolchain_name) in installed_toolchains.into_iter().enumerate() { + let is_default_toolchain = default_toolchain_name.as_ref() == Some(&toolchain_name); + let is_active_toolchain = active_toolchain_name == Some(&toolchain_name); + + let status_str = match (is_default_toolchain, is_active_toolchain) { + (true, true) => " (default, active)", + (true, false) => " (default)", + (false, true) => " (active)", + (false, false) => "", + }; + + writeln!(t.lock(), "{toolchain_name}{status_str}")?; + if verbose { - let toolchain = Toolchain::new(cfg, it.into())?; - writeln!(process().stdout().lock(), "{}", toolchain.rustc_version())?; - // To make it easy to see what rustc that belongs to what - // toolchain we separate each pair with an extra newline - writeln!(process().stdout().lock())?; + let toolchain = Toolchain::new(cfg, toolchain_name.into())?; + writeln!(process().stdout().lock(), " {}", toolchain.rustc_version())?; + // To make it easy to see which rustc belongs to which + // toolchain, we separate each pair with an extra newline. + if n != last_index { + writeln!(process().stdout().lock())?; + } } } - if show_headers { - writeln!(t.lock())? - }; } - if show_active_targets { + // show active toolchain + { let mut t = process().stdout().terminal(); - if show_headers { - print_header::(&mut t, "installed targets for active toolchain")?; - } - for at in active_targets { - writeln!( - t.lock(), - "{}", - at.component - .target - .as_ref() - .expect("rust-std should have a target") - )?; - } - if show_headers { - writeln!(t.lock())?; - }; - } - - if show_active_toolchain { - let mut t = process().stdout().terminal(); + writeln!(t.lock())?; - if show_headers { - print_header::(&mut t, "active toolchain")?; - } + print_header::(&mut t, "active toolchain")?; - match active_toolchain { - Ok((ref toolchain, ref reason)) => { - writeln!(t.lock(), "{} ({})", toolchain.name(), reason)?; - writeln!(t.lock(), "{}", toolchain.rustc_version())?; - } - Err(err) => { - let root_cause = err.root_cause(); - if let Some(RustupError::ToolchainNotSelected) = - root_cause.downcast_ref::() - { - writeln!(t.lock(), "no active toolchain")?; - } else if let Some(cause) = err.source() { - writeln!(t.lock(), "(error: {err}, {cause})")?; - } else { - writeln!(t.lock(), "(error: {err})")?; + match active_toolchain_and_reason { + Some((active_toolchain_name, active_reason)) => { + let active_toolchain = new_toolchain_with_reason( + cfg, + active_toolchain_name.clone().into(), + &active_reason, + )?; + writeln!(t.lock(), "name: {}", active_toolchain.name())?; + writeln!(t.lock(), "compiler: {}", active_toolchain.rustc_version())?; + writeln!(t.lock(), "active because: {}", active_reason.to_string())?; + + // show installed targets for the active toolchain + writeln!(t.lock(), "installed targets:")?; + + for at in active_targets { + writeln!( + t.lock(), + " {}", + at.component + .target + .as_ref() + .expect("rust-std should have a target") + )?; } } - } - - if show_headers { - writeln!(t.lock())? + None => { + writeln!(t.lock(), "no active toolchain")?; + } } } @@ -1210,9 +1197,11 @@ fn show(cfg: &Cfg, m: &ArgMatches) -> Result { E: From, { t.attr(terminalsource::Attr::Bold)?; - writeln!(t.lock(), "{s}")?; - writeln!(t.lock(), "{}", "-".repeat(s.len()))?; - writeln!(t.lock())?; + { + let mut term_lock = t.lock(); + writeln!(term_lock, "{s}")?; + writeln!(term_lock, "{}", "-".repeat(s.len()))?; + } // drop the term_lock t.reset()?; Ok(()) } @@ -1224,27 +1213,27 @@ fn show(cfg: &Cfg, m: &ArgMatches) -> Result { fn show_active_toolchain(cfg: &Cfg, m: &ArgMatches) -> Result { let verbose = m.get_flag("verbose"); let cwd = utils::current_dir()?; - match cfg.find_or_install_active_toolchain(&cwd) { - Err(e) => { - let root_cause = e.root_cause(); - if let Some(RustupError::ToolchainNotSelected) = - root_cause.downcast_ref::() - { - } else { - return Err(e); - } - } - Ok((toolchain, reason)) => { + match cfg.find_active_toolchain(&cwd)? { + Some((toolchain_name, reason)) => { + let toolchain = new_toolchain_with_reason(cfg, toolchain_name.clone().into(), &reason)?; writeln!( process().stdout().lock(), - "{} ({})", + "{}\nactive because: {}", toolchain.name(), reason )?; if verbose { - writeln!(process().stdout().lock(), "{}", toolchain.rustc_version())?; + writeln!( + process().stdout().lock(), + "compiler: {}", + toolchain.rustc_version() + )?; } } + None => writeln!( + process().stdout().lock(), + "no default toolchain is configured" + )?, } Ok(utils::ExitCode(0)) } diff --git a/src/config.rs b/src/config.rs index 2b2e54b5c07..5cf3bdf6b4e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -109,8 +109,8 @@ pub(crate) enum ActiveReason { impl Display for ActiveReason { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::result::Result<(), fmt::Error> { match self { - Self::Default => write!(f, "default"), - Self::Environment => write!(f, "environment override by RUSTUP_TOOLCHAIN"), + Self::Default => write!(f, "it's the default toolchain"), + Self::Environment => write!(f, "overriden by environment variable RUSTUP_TOOLCHAIN"), Self::CommandLine => write!(f, "overridden by +toolchain on the command line"), Self::OverrideDB(path) => write!(f, "directory override for '{}'", path.display()), Self::ToolchainFile(path) => write!(f, "overridden by '{}'", path.display()), diff --git a/tests/suite/cli_misc.rs b/tests/suite/cli_misc.rs index 49982fd2236..ae5f8303e90 100644 --- a/tests/suite/cli_misc.rs +++ b/tests/suite/cli_misc.rs @@ -930,7 +930,7 @@ fn override_by_toolchain_on_the_command_line() { config.expect_stdout_ok(&["rustup", "+nightly", "which", "rustc"], "/bin/rustc"); config.expect_stdout_ok( &["rustup", "+nightly", "show"], - "(overridden by +toolchain on the command line)", + "active because: overridden by +toolchain on the command line", ); config.expect_err( &["rustup", "+foo", "which", "rustc"], diff --git a/tests/suite/cli_rustup.rs b/tests/suite/cli_rustup.rs index 9caf598d627..7692980ec12 100644 --- a/tests/suite/cli_rustup.rs +++ b/tests/suite/cli_rustup.rs @@ -505,7 +505,7 @@ fn link() { config.expect_ok(&["rustup", "toolchain", "link", "custom", &path]); config.expect_ok(&["rustup", "default", "custom"]); config.expect_stdout_ok(&["rustc", "--version"], "hash-c-1"); - config.expect_stdout_ok(&["rustup", "show"], "custom (default)"); + config.expect_stdout_ok(&["rustup", "show"], "custom (default, active)"); config.expect_ok(&["rustup", "update", "nightly"]); config.expect_ok(&["rustup", "default", "nightly"]); config.expect_stdout_ok(&["rustup", "show"], "custom"); @@ -630,6 +630,11 @@ fn show_toolchain_none() { r"Default host: {0} rustup home: {1} +installed toolchains +-------------------- + +active toolchain +---------------- no active toolchain " ), @@ -650,8 +655,17 @@ fn show_toolchain_default() { r"Default host: {0} rustup home: {1} -nightly-{0} (default) -1.3.0 (hash-nightly-2) +installed toolchains +-------------------- +nightly-{0} (default, active) + +active toolchain +---------------- +name: nightly-{0} +compiler: 1.3.0 (hash-nightly-2) +active because: it's the default toolchain +installed targets: + {0} " ), r"", @@ -660,6 +674,50 @@ nightly-{0} (default) }); } +#[test] +fn show_no_default() { + test(&|config| { + config.with_scenario(Scenario::SimpleV2, &|config| { + config.expect_ok(&["rustup", "install", "nightly"]); + config.expect_ok(&["rustup", "default", "none"]); + config.expect_stdout_ok( + &["rustup", "show"], + for_host!( + "\ +installed toolchains +-------------------- +nightly-{0} + +active toolchain +" + ), + ); + }) + }); +} + +#[test] +fn show_no_default_active() { + test(&|config| { + config.with_scenario(Scenario::SimpleV2, &|config| { + config.expect_ok(&["rustup", "install", "nightly"]); + config.expect_ok(&["rustup", "default", "none"]); + config.expect_stdout_ok( + &["rustup", "+nightly", "show"], + for_host!( + "\ +installed toolchains +-------------------- +nightly-{0} (active) + +active toolchain +" + ), + ); + }) + }); +} + #[test] fn show_multiple_toolchains() { test(&|config| { @@ -675,16 +733,16 @@ rustup home: {1} installed toolchains -------------------- - stable-{0} -nightly-{0} (default) +nightly-{0} (default, active) active toolchain ---------------- - -nightly-{0} (default) -1.3.0 (hash-nightly-2) - +name: nightly-{0} +compiler: 1.3.0 (hash-nightly-2) +active because: it's the default toolchain +installed targets: + {0} " ), r"", @@ -709,18 +767,18 @@ fn show_multiple_targets() { r"Default host: {2} rustup home: {3} -installed targets for active toolchain --------------------------------------- - -{1} -{0} +installed toolchains +-------------------- +nightly-{0} (default, active) active toolchain ---------------- - -nightly-{0} (default) -1.3.0 (xxxx-nightly-2) - +name: nightly-{0} +compiler: 1.3.0 (xxxx-nightly-2) +active because: it's the default toolchain +installed targets: + {1} + {0} ", clitools::MULTI_ARCH1, clitools::CROSS_ARCH2, @@ -760,22 +818,17 @@ rustup home: {3} installed toolchains -------------------- - stable-{0} -nightly-{0} (default) - -installed targets for active toolchain --------------------------------------- - -{1} -{0} +nightly-{0} (default, active) active toolchain ---------------- - -nightly-{0} (default) -1.3.0 (xxxx-nightly-2) - +name: nightly-{0} +compiler: 1.3.0 (xxxx-nightly-2) +active because: it's the default toolchain +installed targets: + {1} + {0} ", clitools::MULTI_ARCH1, clitools::CROSS_ARCH2, @@ -845,32 +898,35 @@ fn heal_damaged_toolchain() { test(&|config| { config.with_scenario(Scenario::SimpleV2, &|config| { config.expect_ok(&["rustup", "default", "nightly"]); - config.expect_not_stderr_ok( - &["rustup", "show", "active-toolchain"], - "syncing channel updates", - ); - let path = format!( + config.expect_not_stderr_ok(&["rustup", "which", "rustc"], "syncing channel updates"); + let manifest_path = format!( "toolchains/nightly-{}/lib/rustlib/multirust-channel-manifest.toml", this_host_triple() ); - fs::remove_file(config.rustupdir.join(path)).unwrap(); + + let mut rustc_path = config.rustupdir.join( + [ + "toolchains", + &format!("nightly-{}", this_host_triple()), + "bin", + "rustc", + ] + .iter() + .collect::(), + ); + + if cfg!(windows) { + rustc_path.set_extension("exe"); + } + + fs::remove_file(config.rustupdir.join(manifest_path)).unwrap(); config.expect_ok_ex( - &["rustup", "show", "active-toolchain"], - &format!( - r"nightly-{0} (default) -", - this_host_triple() - ), - for_host!( - r"info: syncing channel updates for 'nightly-{0}' -" - ), + &["rustup", "which", "rustc"], + &format!("{}\n", rustc_path.to_str().unwrap()), + for_host!("info: syncing channel updates for 'nightly-{0}'\n"), ); config.expect_ok(&["rustup", "default", "nightly"]); - config.expect_stderr_ok( - &["rustup", "show", "active-toolchain"], - "syncing channel updates", - ); + config.expect_stderr_ok(&["rustup", "which", "rustc"], "syncing channel updates"); }) }); } @@ -1023,15 +1079,12 @@ fn show_toolchain_override_not_installed() { config.with_scenario(Scenario::SimpleV2, &|config| { config.expect_ok(&["rustup", "override", "add", "nightly"]); config.expect_ok(&["rustup", "toolchain", "remove", "nightly"]); - let mut cmd = clitools::cmd(config, "rustup", ["show"]); - clitools::env(config, &mut cmd); - let out = cmd.output().unwrap(); - assert!(out.status.success()); - let stdout = String::from_utf8(out.stdout).unwrap(); - let stderr = String::from_utf8(out.stderr).unwrap(); - assert!(!stdout.contains("not a directory")); - assert!(!stdout.contains("is not installed")); - assert!(stderr.contains("info: installing component 'rustc'")); + let out = config.run("rustup", ["show"], &[]); + assert!(!out.ok); + assert!(out + .stderr + .contains("is not installed: the directory override for")); + assert!(!out.stderr.contains("info: installing component 'rustc'")); }) }); } @@ -1078,8 +1131,17 @@ fn show_toolchain_env() { r"Default host: {0} rustup home: {1} -nightly-{0} (environment override by RUSTUP_TOOLCHAIN) -1.3.0 (hash-nightly-2) +installed toolchains +-------------------- +nightly-{0} (default, active) + +active toolchain +---------------- +name: nightly-{0} +compiler: 1.3.0 (hash-nightly-2) +active because: overriden by environment variable RUSTUP_TOOLCHAIN +installed targets: + {0} " ) ); @@ -1091,15 +1153,31 @@ nightly-{0} (environment override by RUSTUP_TOOLCHAIN) fn show_toolchain_env_not_installed() { test(&|config| { config.with_scenario(Scenario::SimpleV2, &|config| { - let mut cmd = clitools::cmd(config, "rustup", ["show"]); - clitools::env(config, &mut cmd); - cmd.env("RUSTUP_TOOLCHAIN", "nightly"); - let out = cmd.output().unwrap(); - assert!(out.status.success()); - let stdout = String::from_utf8(out.stdout).unwrap(); - let stderr = String::from_utf8(out.stderr).unwrap(); - assert!(!stdout.contains("is not installed")); - assert!(stderr.contains("info: installing component 'rustc'")); + let out = config.run("rustup", ["show"], &[("RUSTUP_TOOLCHAIN", "nightly")]); + + assert!(!out.ok); + + let expected_out = for_host_and_home!( + config, + r"Default host: {0} +rustup home: {1} + +installed toolchains +-------------------- + +active toolchain +---------------- +" + ); + assert!(&out.stdout == expected_out); + assert!( + out.stderr + == format!( + "error: override toolchain 'nightly-{}' is not installed: \ + the RUSTUP_TOOLCHAIN environment variable specifies an uninstalled toolchain\n", + this_host_triple() + ) + ); }) }); } @@ -1111,10 +1189,7 @@ fn show_active_toolchain() { config.expect_ok(&["rustup", "default", "nightly"]); config.expect_ok_ex( &["rustup", "show", "active-toolchain"], - for_host!( - r"nightly-{0} (default) -" - ), + for_host!("nightly-{0}\nactive because: it's the default toolchain\n"), r"", ); }) @@ -1138,20 +1213,19 @@ rustup home: {1} installed toolchains -------------------- - nightly-2015-01-01-{0} -1.2.0 (hash-nightly-1) - -nightly-{0} (default) -1.3.0 (hash-nightly-2) + 1.2.0 (hash-nightly-1) +nightly-{0} (default, active) + 1.3.0 (hash-nightly-2) active toolchain ---------------- - -nightly-{0} (default) -1.3.0 (hash-nightly-2) - +name: nightly-{0} +compiler: 1.3.0 (hash-nightly-2) +active because: it's the default toolchain +installed targets: + {0} " ), r"", @@ -1168,8 +1242,9 @@ fn show_active_toolchain_with_verbose() { config.expect_ok_ex( &["rustup", "show", "active-toolchain", "--verbose"], for_host!( - r"nightly-{0} (default) -1.3.0 (hash-nightly-2) + r"nightly-{0} +active because: it's the default toolchain +compiler: 1.3.0 (hash-nightly-2) " ), r"", @@ -1187,7 +1262,7 @@ fn show_active_toolchain_with_override() { config.expect_ok(&["rustup", "override", "set", "stable"]); config.expect_stdout_ok( &["rustup", "show", "active-toolchain"], - for_host!("stable-{0} (directory override for"), + for_host!("stable-{0}\nactive because: directory override for"), ); }) }); @@ -1196,7 +1271,11 @@ fn show_active_toolchain_with_override() { #[test] fn show_active_toolchain_none() { test(&|config| { - config.expect_ok_ex(&["rustup", "show", "active-toolchain"], r"", r""); + config.expect_ok_ex( + &["rustup", "show", "active-toolchain"], + "no default toolchain is configured\n", + "", + ); }); } @@ -1968,7 +2047,7 @@ fn override_order() { config.expect_ok(&["rustup", "default", "none"]); config.expect_stdout_ok( &["rustup", "show", "active-toolchain"], - "", + "no default toolchain is configured\n", ); // Default