From b28c184d6ab805996dd258d8ec98900765973383 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 | 227 +++++++++++++--------------- src/config.rs | 4 +- tests/suite/cli_misc.rs | 2 +- tests/suite/cli_rustup.rs | 255 +++++++++++++++++++++----------- 5 files changed, 274 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 590ef384180..f0fe3da3147 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -20,7 +20,7 @@ use crate::{ topical_doc, }, command, - config::ActiveReason, + config::{new_toolchain_with_reason, ActiveReason}, currentprocess::{ argsource::ArgSource, filesource::{StderrSource, StdoutSource}, @@ -38,8 +38,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 +1082,106 @@ 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_toolchain_targets: Vec = active_toolchain_name + .and_then(|atn| match atn { + ToolchainName::Official(desc) => DistributableToolchain::new(cfg, desc.clone()).ok(), + ToolchainName::Custom(_) => 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) + .map(|c| c.component.target.expect("rust-std should have a target")) + .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) => " (active, default)", + (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())?; - }; - } + writeln!(t.lock())?; - if show_active_toolchain { - let mut t = process().stdout().terminal(); + print_header::(&mut t, "active toolchain")?; - if show_headers { - print_header::(&mut t, "active toolchain")?; - } + 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())?; - 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})")?; + // show installed targets for the active toolchain + writeln!(t.lock(), "installed targets:")?; + + for target in active_toolchain_targets { + writeln!(t.lock(), " {}", target)?; } } - } - - if show_headers { - writeln!(t.lock())? + None => { + writeln!(t.lock(), "no active toolchain")?; + } } } @@ -1210,9 +1190,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 +1206,24 @@ 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(), "There isn't an active toolchain")?, } Ok(utils::ExitCode(0)) } diff --git a/src/config.rs b/src/config.rs index 144867a4b62..b72ad39d6d8 100644 --- a/src/config.rs +++ b/src/config.rs @@ -108,8 +108,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 b1a2c358c4f..1a3e60dcf35 100644 --- a/tests/suite/cli_misc.rs +++ b/tests/suite/cli_misc.rs @@ -1002,7 +1002,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 f115289acac..c8ce12164dd 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 (active, default)"); config.expect_ok(&["rustup", "update", "nightly"]); config.expect_ok(&["rustup", "default", "nightly"]); config.expect_stdout_ok(&["rustup", "show"], "custom"); @@ -616,6 +616,11 @@ fn show_toolchain_none() { r"Default host: {0} rustup home: {1} +installed toolchains +-------------------- + +active toolchain +---------------- no active toolchain " ), @@ -636,8 +641,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} (active, default) + +active toolchain +---------------- +name: nightly-{0} +compiler: 1.3.0 (hash-nightly-2) +active because: it's the default toolchain +installed targets: + {0} " ), r"", @@ -646,6 +660,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| { @@ -661,16 +719,16 @@ rustup home: {1} installed toolchains -------------------- - stable-{0} -nightly-{0} (default) +nightly-{0} (active, default) 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"", @@ -695,18 +753,18 @@ fn show_multiple_targets() { r"Default host: {2} rustup home: {3} -installed targets for active toolchain --------------------------------------- - -{1} -{0} +installed toolchains +-------------------- +nightly-{0} (active, default) 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, @@ -746,22 +804,17 @@ rustup home: {3} installed toolchains -------------------- - stable-{0} -nightly-{0} (default) - -installed targets for active toolchain --------------------------------------- - -{1} -{0} +nightly-{0} (active, default) 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, @@ -831,32 +884,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"); }) }); } @@ -1009,15 +1065,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'")); }) }); } @@ -1064,8 +1117,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} (active, default) + +active toolchain +---------------- +name: nightly-{0} +compiler: 1.3.0 (hash-nightly-2) +active because: overriden by environment variable RUSTUP_TOOLCHAIN +installed targets: + {0} " ) ); @@ -1077,15 +1139,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() + ) + ); }) }); } @@ -1097,10 +1175,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"", ); }) @@ -1124,20 +1199,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} (active, default) + 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"", @@ -1154,8 +1228,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"", @@ -1173,7 +1248,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"), ); }) }); @@ -1182,7 +1257,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"], + "There isn't an active toolchain\n", + "", + ); }); } @@ -2050,7 +2129,7 @@ fn override_order() { config.expect_ok(&["rustup", "default", "none"]); config.expect_stdout_ok( &["rustup", "show", "active-toolchain"], - "", + "There isn't an active toolchain\n", ); // Default