From bc907aa3f4a98496378f3219c778ccf36b7e3e85 Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Thu, 14 Dec 2023 17:01:08 +0000 Subject: [PATCH 01/11] Pull out `ensure_installed()` --- src/config.rs | 67 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 40 insertions(+), 27 deletions(-) diff --git a/src/config.rs b/src/config.rs index b6aa81201b..a80034987a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -684,38 +684,51 @@ impl Cfg { Ok((Toolchain::new(self, toolchain)?, reason)) } Some(LocalToolchainName::Named(ToolchainName::Official(desc))) => { - let components: Vec<_> = components.iter().map(AsRef::as_ref).collect(); - let targets: Vec<_> = targets.iter().map(AsRef::as_ref).collect(); - let toolchain = match DistributableToolchain::new(self, desc.clone()) { - Err(RustupError::ToolchainNotInstalled(_)) => { - DistributableToolchain::install( - self, - &desc, - &components, - &targets, - profile.unwrap_or(Profile::Default), - false, - )? - .1 - } - Ok(mut distributable) => { - if !distributable.components_exist(&components, &targets)? { - distributable.update( - &components, - &targets, - profile.unwrap_or(Profile::Default), - )?; - } - distributable - } - Err(e) => return Err(e.into()), - } - .into(); + let toolchain = self.ensure_installed(desc, components, targets, profile)?; Ok((toolchain, reason)) } } } + // Returns a Toolchain matching the given ToolchainDesc, installing it and + // the given components and targets if they aren't already installed. + fn ensure_installed( + &self, + toolchain: ToolchainDesc, + components: Vec, + targets: Vec, + profile: Option, + ) -> Result> { + let components: Vec<_> = components.iter().map(AsRef::as_ref).collect(); + let targets: Vec<_> = targets.iter().map(AsRef::as_ref).collect(); + let toolchain = match DistributableToolchain::new(self, toolchain.clone()) { + Err(RustupError::ToolchainNotInstalled(_)) => { + DistributableToolchain::install( + self, + &toolchain, + &components, + &targets, + profile.unwrap_or(Profile::Default), + false, + )? + .1 + } + Ok(mut distributable) => { + if !distributable.components_exist(&components, &targets)? { + distributable.update( + &components, + &targets, + profile.unwrap_or(Profile::Default), + )?; + } + distributable + } + Err(e) => return Err(e.into()), + } + .into(); + Ok(toolchain) + } + /// Get the configured default toolchain. /// If none is configured, returns None /// If a bad toolchain name is configured, errors. From 90ba7d493a5ea9235fb9238492dbae16b93e728f Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Thu, 14 Dec 2023 17:29:16 +0000 Subject: [PATCH 02/11] Pull out `new_toolchain_with_reason()` --- src/config.rs | 67 +++++++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 31 deletions(-) diff --git a/src/config.rs b/src/config.rs index a80034987a..c74eaae295 100644 --- a/src/config.rs +++ b/src/config.rs @@ -114,6 +114,41 @@ impl Display for OverrideReason { } } +/// Calls Toolchain::new(), but augments the error message with more context +/// from the OverrideReason if the toolchain isn't installed. +pub(crate) fn new_toolchain_with_reason<'a>( + cfg: &'a Cfg, + name: LocalToolchainName, + reason: &OverrideReason, +) -> Result> { + match Toolchain::new(cfg, name.clone()) { + Err(RustupError::ToolchainNotInstalled(_)) => (), + result => { + return Ok(result?); + } + } + + let reason_err = match reason { + OverrideReason::Environment => { + "the RUSTUP_TOOLCHAIN environment variable specifies an uninstalled toolchain" + .to_string() + } + OverrideReason::CommandLine => { + "the +toolchain on the command line specifies an uninstalled toolchain".to_string() + } + OverrideReason::OverrideDB(ref path) => format!( + "the directory override for '{}' specifies an uninstalled toolchain", + utils::canonicalize_path(path, cfg.notify_handler.as_ref()).display(), + ), + OverrideReason::ToolchainFile(ref path) => format!( + "the toolchain file at '{}' specifies an uninstalled toolchain", + utils::canonicalize_path(path, cfg.notify_handler.as_ref()).display(), + ), + }; + + Err(anyhow!(reason_err).context(format!("override toolchain '{name}' is not installed"))) +} + #[derive(Default, Debug)] struct OverrideCfg { toolchain: Option, @@ -480,29 +515,6 @@ impl Cfg { } if let Some((file, reason)) = override_ { - // This is hackishly using the error chain to provide a bit of - // extra context about what went wrong. The CLI will display it - // on a line after the proximate error. - - let reason_err = match reason { - OverrideReason::Environment => { - "the RUSTUP_TOOLCHAIN environment variable specifies an uninstalled toolchain" - .to_string() - } - OverrideReason::CommandLine => { - "the +toolchain on the command line specifies an uninstalled toolchain" - .to_string() - } - OverrideReason::OverrideDB(ref path) => format!( - "the directory override for '{}' specifies an uninstalled toolchain", - utils::canonicalize_path(path, self.notify_handler.as_ref()).display(), - ), - OverrideReason::ToolchainFile(ref path) => format!( - "the toolchain file at '{}' specifies an uninstalled toolchain", - utils::canonicalize_path(path, self.notify_handler.as_ref()).display(), - ), - }; - let override_cfg = OverrideCfg::from_file(self, file)?; // Overridden toolchains can be literally any string, but only // distributable toolchains will be auto-installed by the wrapping @@ -511,14 +523,7 @@ impl Cfg { match &override_cfg.toolchain { Some(t @ LocalToolchainName::Named(ToolchainName::Custom(_))) | Some(t @ LocalToolchainName::Path(_)) => { - if let Err(RustupError::ToolchainNotInstalled(_)) = - Toolchain::new(self, t.to_owned()) - { - // Strip the confusing NotADirectory error and only mention that the - // override toolchain is not installed. - return Err(anyhow!(reason_err)) - .with_context(|| format!("override toolchain '{t}' is not installed")); - } + new_toolchain_with_reason(self, t.clone(), &reason)?; } // Either official (can auto install) or no toolchain specified _ => {} From 4a84b8a2e1ac3521d775c74cfe44dce1128cb7fd Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Thu, 14 Dec 2023 18:55:11 +0000 Subject: [PATCH 03/11] Change find_override to find_active_toolchain Changes the model from "find an override to the default toolchain" to "find the active toolchain, which may be the default toolchain". --- src/cli/common.rs | 15 ++++++++----- src/cli/rustup_mode.rs | 7 ++++-- src/config.rs | 51 +++++++++++++++++++++++++----------------- 3 files changed, 46 insertions(+), 27 deletions(-) diff --git a/src/cli/common.rs b/src/cli/common.rs index d1967417c8..a3a0020ae1 100644 --- a/src/cli/common.rs +++ b/src/cli/common.rs @@ -14,6 +14,7 @@ use once_cell::sync::Lazy; use super::self_update; use crate::cli::download_tracker::DownloadTracker; +use crate::config::ActiveReason; use crate::currentprocess::{ argsource::ArgSource, filesource::{StdinSource, StdoutSource}, @@ -466,11 +467,15 @@ pub(crate) fn list_toolchains(cfg: &Cfg, verbose: bool) -> Result None, + _ => Some(toolchain), + } + } else { + None + }; for toolchain in toolchains { let if_default = if def_toolchain_name.as_ref() == Some(&toolchain) { " (default)" diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index e724944cca..a1764ac747 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -20,6 +20,7 @@ use crate::{ topical_doc, }, command, + config::ActiveReason, currentprocess::{ argsource::ArgSource, filesource::{StderrSource, StdoutSource}, @@ -864,8 +865,10 @@ fn default_(cfg: &Cfg, m: &ArgMatches) -> Result { }; let cwd = utils::current_dir()?; - if let Some((toolchain, reason)) = cfg.find_override(&cwd)? { - info!("note that the toolchain '{toolchain}' is currently in use ({reason})"); + if let Some((toolchain, reason)) = cfg.find_active_toolchain(&cwd)? { + if !matches!(reason, ActiveReason::Default) { + info!("note that the toolchain '{toolchain}' is currently in use ({reason})"); + } } } else { let default_toolchain = cfg diff --git a/src/config.rs b/src/config.rs index c74eaae295..479d43aae3 100644 --- a/src/config.rs +++ b/src/config.rs @@ -95,17 +95,20 @@ impl> From for OverrideFile { } } +// Represents the reason why the active toolchain is active. #[derive(Debug)] -pub(crate) enum OverrideReason { +pub(crate) enum ActiveReason { + Default, Environment, CommandLine, OverrideDB(PathBuf), ToolchainFile(PathBuf), } -impl Display for OverrideReason { +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::CommandLine => write!(f, "overridden by +toolchain on the command line"), Self::OverrideDB(path) => write!(f, "directory override for '{}'", path.display()), @@ -115,11 +118,11 @@ impl Display for OverrideReason { } /// Calls Toolchain::new(), but augments the error message with more context -/// from the OverrideReason if the toolchain isn't installed. +/// from the ActiveReason if the toolchain isn't installed. pub(crate) fn new_toolchain_with_reason<'a>( cfg: &'a Cfg, name: LocalToolchainName, - reason: &OverrideReason, + reason: &ActiveReason, ) -> Result> { match Toolchain::new(cfg, name.clone()) { Err(RustupError::ToolchainNotInstalled(_)) => (), @@ -129,21 +132,24 @@ pub(crate) fn new_toolchain_with_reason<'a>( } let reason_err = match reason { - OverrideReason::Environment => { + ActiveReason::Environment => { "the RUSTUP_TOOLCHAIN environment variable specifies an uninstalled toolchain" .to_string() } - OverrideReason::CommandLine => { + ActiveReason::CommandLine => { "the +toolchain on the command line specifies an uninstalled toolchain".to_string() } - OverrideReason::OverrideDB(ref path) => format!( + ActiveReason::OverrideDB(ref path) => format!( "the directory override for '{}' specifies an uninstalled toolchain", utils::canonicalize_path(path, cfg.notify_handler.as_ref()).display(), ), - OverrideReason::ToolchainFile(ref path) => format!( + ActiveReason::ToolchainFile(ref path) => format!( "the toolchain file at '{}' specifies an uninstalled toolchain", utils::canonicalize_path(path, cfg.notify_handler.as_ref()).display(), ), + ActiveReason::Default => { + "the default toolchain does not describe an installed toolchain".to_string() + } }; Err(anyhow!(reason_err).context(format!("override toolchain '{name}' is not installed"))) @@ -478,21 +484,26 @@ impl Cfg { .transpose()?) } - pub(crate) fn find_override( + pub(crate) fn find_active_toolchain( &self, path: &Path, - ) -> Result> { - Ok(self - .find_override_config(path)? - .and_then(|(override_cfg, reason)| override_cfg.toolchain.map(|t| (t, reason)))) + ) -> Result> { + Ok( + if let Some((override_config, reason)) = self.find_override_config(path)? { + override_config.toolchain.map(|t| (t, reason)) + } else { + self.get_default()? + .map(|x| (x.into(), ActiveReason::Default)) + }, + ) } - fn find_override_config(&self, path: &Path) -> Result> { + fn find_override_config(&self, path: &Path) -> Result> { let mut override_ = None; // First check toolchain override from command if let Some(ref name) = self.toolchain_override { - override_ = Some((name.to_string().into(), OverrideReason::CommandLine)); + override_ = Some((name.to_string().into(), ActiveReason::CommandLine)); } // Check RUSTUP_TOOLCHAIN @@ -501,7 +512,7 @@ impl Cfg { // custom, distributable, and absolute path toolchains otherwise // rustup's export of a RUSTUP_TOOLCHAIN when running a process will // error when a nested rustup invocation occurs - override_ = Some((name.to_string().into(), OverrideReason::Environment)); + override_ = Some((name.to_string().into(), ActiveReason::Environment)); } // Then walk up the directory tree from 'path' looking for either the @@ -539,14 +550,14 @@ impl Cfg { &self, dir: &Path, settings: &Settings, - ) -> Result> { + ) -> Result> { let notify = self.notify_handler.as_ref(); let mut dir = Some(dir); while let Some(d) = dir { // First check the override database if let Some(name) = settings.dir_override(d, notify) { - let reason = OverrideReason::OverrideDB(d.to_owned()); + let reason = ActiveReason::OverrideDB(d.to_owned()); return Ok(Some((name.into(), reason))); } @@ -619,7 +630,7 @@ impl Cfg { } } - let reason = OverrideReason::ToolchainFile(toolchain_file); + let reason = ActiveReason::ToolchainFile(toolchain_file); return Ok(Some((override_file, reason))); } @@ -663,7 +674,7 @@ impl Cfg { pub(crate) fn find_or_install_override_toolchain_or_default( &self, path: &Path, - ) -> Result<(Toolchain<'_>, Option)> { + ) -> Result<(Toolchain<'_>, Option)> { let (toolchain, components, targets, reason, profile) = match self.find_override_config(path)? { Some(( From a94bb6f00f416b20c93d52a184fd807ba217d8f8 Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Wed, 20 Dec 2023 21:00:05 +0000 Subject: [PATCH 04/11] Pull match statement out in OverrideCfg::from_file() --- src/config.rs | 60 ++++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/src/config.rs b/src/config.rs index 479d43aae3..224bd56606 100644 --- a/src/config.rs +++ b/src/config.rs @@ -165,39 +165,41 @@ struct OverrideCfg { impl OverrideCfg { fn from_file(cfg: &Cfg, file: OverrideFile) -> Result { - Ok(Self { - toolchain: match (file.toolchain.channel, file.toolchain.path) { - (Some(name), None) => Some( - (&ResolvableToolchainName::try_from(name)? - .resolve(&cfg.get_default_host_triple()?)?) - .into(), - ), - (None, Some(path)) => { - if file.toolchain.targets.is_some() - || file.toolchain.components.is_some() - || file.toolchain.profile.is_some() - { - bail!( - "toolchain options are ignored for path toolchain ({})", - path.display() - ) - } - // We -do not- support relative paths, they permit trivial - // completely arbitrary code execution in a directory. - // Longer term we'll not support path based toolchains at - // all, because they also permit arbitrary code execution, - // though with more challenges to exploit. - Some((&PathBasedToolchainName::try_from(&path as &Path)?).into()) - } - (Some(channel), Some(path)) => { + let toolchain_name = match (file.toolchain.channel, file.toolchain.path) { + (Some(name), None) => Some( + (&ResolvableToolchainName::try_from(name)? + .resolve(&cfg.get_default_host_triple()?)?) + .into(), + ), + (None, Some(path)) => { + if file.toolchain.targets.is_some() + || file.toolchain.components.is_some() + || file.toolchain.profile.is_some() + { bail!( - "cannot specify both channel ({}) and path ({}) simultaneously", - channel, + "toolchain options are ignored for path toolchain ({})", path.display() ) } - (None, None) => None, - }, + // We -do not- support relative paths, they permit trivial + // completely arbitrary code execution in a directory. + // Longer term we'll not support path based toolchains at + // all, because they also permit arbitrary code execution, + // though with more challenges to exploit. + Some((&PathBasedToolchainName::try_from(&path as &Path)?).into()) + } + (Some(channel), Some(path)) => { + bail!( + "cannot specify both channel ({}) and path ({}) simultaneously", + channel, + path.display() + ) + } + (None, None) => None, + }; + + Ok(Self { + toolchain: toolchain_name, components: file.toolchain.components.unwrap_or_default(), targets: file.toolchain.targets.unwrap_or_default(), profile: file From 6e569ecbfa71f00a7a4984780b89c89c117aef1f Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Wed, 20 Dec 2023 21:53:51 +0000 Subject: [PATCH 05/11] Redesign OverrideCfg to be more type-driven Redesigns the `OverrideCfg` type so that validity is represented in the type system, i.e. "Parse, don't validate". Probably fixes some subtle bugs when no toolchain is named in a rust-toolchain.toml, implicitly selecting the default toolchain, but the default toolchain isn't displayed as active by `rustup list toolchain` and the like. Also fixes a bug where the `RUSTUP_TOOLCHAIN` erroneously had priority over a `+toolchain` command line override. --- src/cli/rustup_mode.rs | 38 ++---- src/config.rs | 260 +++++++++++++++++++++++--------------- tests/suite/cli_rustup.rs | 79 +++++++++--- 3 files changed, 233 insertions(+), 144 deletions(-) diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index a1764ac747..ac3bf7c6fc 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -110,7 +110,7 @@ pub fn main() -> Result { cfg.set_toolchain_override(&ResolvableToolchainName::try_from(&t[1..])?); } - let toolchain = cfg.find_or_install_override_toolchain_or_default(&cwd)?.0; + let toolchain = cfg.find_or_install_active_toolchain(&cwd)?.0; Ok(toolchain.rustc_version()) } @@ -1082,7 +1082,7 @@ 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_override_toolchain_or_default(&cwd); + 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 { @@ -1175,16 +1175,10 @@ fn show(cfg: &Cfg, m: &ArgMatches) -> Result { } match active_toolchain { - Ok(atc) => match atc { - (ref toolchain, Some(ref reason)) => { - writeln!(t.lock(), "{} ({})", toolchain.name(), reason)?; - writeln!(t.lock(), "{}", toolchain.rustc_version())?; - } - (ref toolchain, None) => { - writeln!(t.lock(), "{} (default)", toolchain.name())?; - writeln!(t.lock(), "{}", toolchain.rustc_version())?; - } - }, + 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) = @@ -1223,7 +1217,7 @@ 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_override_toolchain_or_default(&cwd) { + match cfg.find_or_install_active_toolchain(&cwd) { Err(e) => { let root_cause = e.root_cause(); if let Some(RustupError::ToolchainNotSelected) = @@ -1234,16 +1228,12 @@ fn show_active_toolchain(cfg: &Cfg, m: &ArgMatches) -> Result { } } Ok((toolchain, reason)) => { - if let Some(reason) = reason { - writeln!( - process().stdout().lock(), - "{} ({})", - toolchain.name(), - reason - )?; - } else { - writeln!(process().stdout().lock(), "{} (default)", toolchain.name())?; - } + writeln!( + process().stdout().lock(), + "{} ({})", + toolchain.name(), + reason + )?; if verbose { writeln!(process().stdout().lock(), "{}", toolchain.rustc_version())?; } @@ -1408,7 +1398,7 @@ fn explicit_or_dir_toolchain2( } None => { let cwd = utils::current_dir()?; - let (toolchain, _) = cfg.find_or_install_override_toolchain_or_default(&cwd)?; + let (toolchain, _) = cfg.find_or_install_active_toolchain(&cwd)?; Ok(toolchain) } diff --git a/src/config.rs b/src/config.rs index 224bd56606..144867a4b6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -27,8 +27,8 @@ use crate::{ toolchain::{ distributable::DistributableToolchain, names::{ - LocalToolchainName, PathBasedToolchainName, ResolvableLocalToolchainName, - ResolvableToolchainName, ToolchainName, + CustomToolchainName, LocalToolchainName, PathBasedToolchainName, + ResolvableLocalToolchainName, ResolvableToolchainName, ToolchainName, }, toolchain::Toolchain, }, @@ -155,22 +155,28 @@ pub(crate) fn new_toolchain_with_reason<'a>( Err(anyhow!(reason_err).context(format!("override toolchain '{name}' is not installed"))) } -#[derive(Default, Debug)] -struct OverrideCfg { - toolchain: Option, - components: Vec, - targets: Vec, - profile: Option, +// Represents a toolchain override from a +toolchain command line option, +// RUSTUP_TOOLCHAIN environment variable, or rust-toolchain.toml file etc. Can +// include components and targets from a rust-toolchain.toml that should be +// downloaded and installed. +#[derive(Debug)] +enum OverrideCfg { + PathBased(PathBasedToolchainName), + Custom(CustomToolchainName), + Official { + toolchain: ToolchainDesc, + components: Vec, + targets: Vec, + profile: Option, + }, } impl OverrideCfg { fn from_file(cfg: &Cfg, file: OverrideFile) -> Result { let toolchain_name = match (file.toolchain.channel, file.toolchain.path) { - (Some(name), None) => Some( - (&ResolvableToolchainName::try_from(name)? - .resolve(&cfg.get_default_host_triple()?)?) - .into(), - ), + (Some(name), None) => { + ResolvableToolchainName::try_from(name)?.resolve(&cfg.get_default_host_triple()?)? + } (None, Some(path)) => { if file.toolchain.targets.is_some() || file.toolchain.components.is_some() @@ -186,7 +192,9 @@ impl OverrideCfg { // Longer term we'll not support path based toolchains at // all, because they also permit arbitrary code execution, // though with more challenges to exploit. - Some((&PathBasedToolchainName::try_from(&path as &Path)?).into()) + return Ok(Self::PathBased(PathBasedToolchainName::try_from( + &path as &Path, + )?)); } (Some(channel), Some(path)) => { bail!( @@ -195,21 +203,71 @@ impl OverrideCfg { path.display() ) } - (None, None) => None, + (None, None) => cfg + .get_default()? + .ok_or(RustupError::ToolchainNotSelected)?, }; - - Ok(Self { - toolchain: toolchain_name, - components: file.toolchain.components.unwrap_or_default(), - targets: file.toolchain.targets.unwrap_or_default(), - profile: file - .toolchain - .profile - .as_deref() - .map(dist::Profile::from_str) - .transpose()?, + Ok(match toolchain_name { + ToolchainName::Official(desc) => { + let components = file.toolchain.components.unwrap_or_default(); + let targets = file.toolchain.targets.unwrap_or_default(); + Self::Official { + toolchain: desc, + components, + targets, + profile: file + .toolchain + .profile + .as_deref() + .map(dist::Profile::from_str) + .transpose()?, + } + } + ToolchainName::Custom(name) => { + if file.toolchain.targets.is_some() + || file.toolchain.components.is_some() + || file.toolchain.profile.is_some() + { + bail!( + "toolchain options are ignored for a custom toolchain ({})", + name + ) + } + Self::Custom(name) + } }) } + + fn into_local_toolchain_name(self) -> LocalToolchainName { + match self { + Self::PathBased(path_based_name) => path_based_name.into(), + Self::Custom(custom_name) => custom_name.into(), + Self::Official { ref toolchain, .. } => toolchain.into(), + } + } +} + +impl From for OverrideCfg { + fn from(value: ToolchainName) -> Self { + match value { + ToolchainName::Official(desc) => Self::Official { + toolchain: desc, + components: vec![], + targets: vec![], + profile: None, + }, + ToolchainName::Custom(name) => Self::Custom(name), + } + } +} + +impl From for OverrideCfg { + fn from(value: LocalToolchainName) -> Self { + match value { + LocalToolchainName::Named(name) => Self::from(name), + LocalToolchainName::Path(path_name) => Self::PathBased(path_name), + } + } } pub(crate) const UNIX_FALLBACK_SETTINGS: &str = "/etc/rustup/settings.toml"; @@ -434,7 +492,7 @@ impl Cfg { } pub(crate) fn which_binary(&self, path: &Path, binary: &str) -> Result { - let (toolchain, _) = self.find_or_install_override_toolchain_or_default(path)?; + let (toolchain, _) = self.find_or_install_active_toolchain(path)?; Ok(toolchain.binary_file(binary)) } @@ -492,7 +550,7 @@ impl Cfg { ) -> Result> { Ok( if let Some((override_config, reason)) = self.find_override_config(path)? { - override_config.toolchain.map(|t| (t, reason)) + Some((override_config.into_local_toolchain_name(), reason)) } else { self.get_default()? .map(|x| (x.into(), ActiveReason::Default)) @@ -501,51 +559,34 @@ impl Cfg { } fn find_override_config(&self, path: &Path) -> Result> { - let mut override_ = None; - - // First check toolchain override from command - if let Some(ref name) = self.toolchain_override { - override_ = Some((name.to_string().into(), ActiveReason::CommandLine)); - } - - // Check RUSTUP_TOOLCHAIN - if let Some(ref name) = self.env_override { - // Because path based toolchain files exist, this has to support - // custom, distributable, and absolute path toolchains otherwise - // rustup's export of a RUSTUP_TOOLCHAIN when running a process will - // error when a nested rustup invocation occurs - override_ = Some((name.to_string().into(), ActiveReason::Environment)); - } - - // Then walk up the directory tree from 'path' looking for either the - // directory in override database, or a `rust-toolchain` file. - if override_.is_none() { - self.settings_file.with(|s| { - override_ = self.find_override_from_dir_walk(path, s)?; - - Ok(()) - })?; - } - - if let Some((file, reason)) = override_ { - let override_cfg = OverrideCfg::from_file(self, file)?; - // Overridden toolchains can be literally any string, but only - // distributable toolchains will be auto-installed by the wrapping - // code; provide a nice error for this common case. (default could - // be set badly too, but that is much less common). - match &override_cfg.toolchain { - Some(t @ LocalToolchainName::Named(ToolchainName::Custom(_))) - | Some(t @ LocalToolchainName::Path(_)) => { - new_toolchain_with_reason(self, t.clone(), &reason)?; - } - // Either official (can auto install) or no toolchain specified - _ => {} + let override_config: Option<(OverrideCfg, ActiveReason)> = + // First check +toolchain override from the command line + if let Some(ref name) = self.toolchain_override { + let override_config = name.resolve(&self.get_default_host_triple()?)?.into(); + Some((override_config, ActiveReason::CommandLine)) + } + // Then check the RUSTUP_TOOLCHAIN environment variable + else if let Some(ref name) = self.env_override { + // Because path based toolchain files exist, this has to support + // custom, distributable, and absolute path toolchains otherwise + // rustup's export of a RUSTUP_TOOLCHAIN when running a process will + // error when a nested rustup invocation occurs + Some((name.clone().into(), ActiveReason::Environment)) } + // Then walk up the directory tree from 'path' looking for either the + // directory in the override database, or a `rust-toolchain{.toml}` file, + // in that order. + else if let Some((override_file, active_reason)) = self.settings_file.with(|s| { + self.find_override_from_dir_walk(path, s) + })? { + Some((OverrideCfg::from_file(self, override_file)?, active_reason)) + } + // Otherwise, there is no override. + else { + None + }; - Ok(Some((override_cfg, reason))) - } else { - Ok(None) - } + Ok(override_config) } fn find_override_from_dir_walk( @@ -672,39 +713,54 @@ impl Cfg { } } + pub(crate) fn find_or_install_active_toolchain( + &self, + path: &Path, + ) -> Result<(Toolchain<'_>, ActiveReason)> { + self.maybe_find_or_install_active_toolchain(path)? + .ok_or(RustupError::ToolchainNotSelected.into()) + } + #[cfg_attr(feature = "otel", tracing::instrument(skip_all))] - pub(crate) fn find_or_install_override_toolchain_or_default( + pub(crate) fn maybe_find_or_install_active_toolchain( &self, path: &Path, - ) -> Result<(Toolchain<'_>, Option)> { - let (toolchain, components, targets, reason, profile) = - match self.find_override_config(path)? { - Some(( - OverrideCfg { - toolchain, - components, - targets, - profile, - }, - reason, - )) => (toolchain, components, targets, Some(reason), profile), - None => (None, vec![], vec![], None, None), - }; - let toolchain = match toolchain { - t @ Some(_) => t, - None => self.get_default()?.map(Into::into), - }; - match toolchain { - // No override and no default set - None => Err(RustupError::ToolchainNotSelected.into()), - Some(toolchain @ LocalToolchainName::Named(ToolchainName::Custom(_))) - | Some(toolchain @ LocalToolchainName::Path(_)) => { - Ok((Toolchain::new(self, toolchain)?, reason)) - } - Some(LocalToolchainName::Named(ToolchainName::Official(desc))) => { - let toolchain = self.ensure_installed(desc, components, targets, profile)?; - Ok((toolchain, reason)) - } + ) -> Result, ActiveReason)>> { + match self.find_override_config(path)? { + Some((override_config, reason)) => match override_config { + OverrideCfg::PathBased(path_based_name) => { + let toolchain = + new_toolchain_with_reason(self, path_based_name.into(), &reason)?; + Ok(Some((toolchain, reason))) + } + OverrideCfg::Custom(custom_name) => { + let toolchain = new_toolchain_with_reason(self, custom_name.into(), &reason)?; + Ok(Some((toolchain, reason))) + } + OverrideCfg::Official { + toolchain, + components, + targets, + profile, + } => { + let toolchain = + self.ensure_installed(toolchain, components, targets, profile)?; + Ok(Some((toolchain, reason))) + } + }, + None => match self.get_default()? { + None => Ok(None), + Some(ToolchainName::Custom(custom_name)) => { + let reason = ActiveReason::Default; + let toolchain = new_toolchain_with_reason(self, custom_name.into(), &reason)?; + Ok(Some((toolchain, reason))) + } + Some(ToolchainName::Official(toolchain_desc)) => { + let reason = ActiveReason::Default; + let toolchain = self.ensure_installed(toolchain_desc, vec![], vec![], None)?; + Ok(Some((toolchain, reason))) + } + }, } } @@ -858,7 +914,7 @@ impl Cfg { } pub(crate) fn create_command_for_dir(&self, path: &Path, binary: &str) -> Result { - let (toolchain, _) = self.find_or_install_override_toolchain_or_default(path)?; + let (toolchain, _) = self.find_or_install_active_toolchain(path)?; self.create_command_for_toolchain_(toolchain, binary) } diff --git a/tests/suite/cli_rustup.rs b/tests/suite/cli_rustup.rs index 1bd20ded99..14bc9dd73e 100644 --- a/tests/suite/cli_rustup.rs +++ b/tests/suite/cli_rustup.rs @@ -2003,7 +2003,7 @@ channel = "nightly" } #[test] -fn directory_override_beats_file_override() { +fn close_file_override_beats_far_directory_override() { test(&|config| { config.with_scenario(Scenario::SimpleV2, &|config| { config.expect_ok(&["rustup", "default", "stable"]); @@ -2014,36 +2014,79 @@ fn directory_override_beats_file_override() { config.expect_stdout_ok(&["rustc", "--version"], "hash-beta-1.2.0"); let cwd = config.current_dir(); - let toolchain_file = cwd.join("rust-toolchain"); + + let subdir = cwd.join("subdir"); + fs::create_dir_all(&subdir).unwrap(); + + let toolchain_file = subdir.join("rust-toolchain"); raw::write_file(&toolchain_file, "nightly").unwrap(); - config.expect_stdout_ok(&["rustc", "--version"], "hash-beta-1.2.0"); + config.change_dir(&subdir, &|config| { + config.expect_stdout_ok(&["rustc", "--version"], "hash-nightly-2"); + }); }) }); } #[test] -fn close_file_override_beats_far_directory_override() { +// Check that toolchain overrides have the correct priority. +fn override_order() { test(&|config| { - config.with_scenario(Scenario::SimpleV2, &|config| { - config.expect_ok(&["rustup", "default", "stable"]); - config.expect_ok(&["rustup", "toolchain", "install", "beta"]); - config.expect_ok(&["rustup", "toolchain", "install", "nightly"]); + config.with_scenario(Scenario::ArchivesV2, &|config| { + let host = this_host_triple(); + // give each override type a different toolchain + let default_tc = &format!("beta-2015-01-01-{}", host); + let env_tc = &format!("stable-2015-01-01-{}", host); + let dir_tc = &format!("beta-2015-01-02-{}", host); + let file_tc = &format!("stable-2015-01-02-{}", host); + let command_tc = &format!("nightly-2015-01-01-{}", host); + config.expect_ok(&["rustup", "install", default_tc]); + config.expect_ok(&["rustup", "install", env_tc]); + config.expect_ok(&["rustup", "install", dir_tc]); + config.expect_ok(&["rustup", "install", file_tc]); + config.expect_ok(&["rustup", "install", command_tc]); + + // No default + config.expect_ok(&["rustup", "default", "none"]); + config.expect_stdout_ok( + &["rustup", "show", "active-toolchain"], + "", + ); - config.expect_ok(&["rustup", "override", "set", "beta"]); - config.expect_stdout_ok(&["rustc", "--version"], "hash-beta-1.2.0"); + // Default + config.expect_ok(&["rustup", "default", default_tc]); + config.expect_stdout_ok(&["rustup", "show", "active-toolchain"], default_tc); - let cwd = config.current_dir(); + // file > default + let toolchain_file = config.current_dir().join("rust-toolchain.toml"); + raw::write_file( + &toolchain_file, + &format!("[toolchain]\nchannel='{}'", file_tc), + ) + .unwrap(); + config.expect_stdout_ok(&["rustup", "show", "active-toolchain"], file_tc); - let subdir = cwd.join("subdir"); - fs::create_dir_all(&subdir).unwrap(); + // dir override > file > default + config.expect_ok(&["rustup", "override", "set", dir_tc]); + config.expect_stdout_ok(&["rustup", "show", "active-toolchain"], dir_tc); - let toolchain_file = subdir.join("rust-toolchain"); - raw::write_file(&toolchain_file, "nightly").unwrap(); + // env > dir override > file > default + let out = config.run( + "rustup", + ["show", "active-toolchain"], + &[("RUSTUP_TOOLCHAIN", env_tc)], + ); + assert!(out.ok); + assert!(out.stdout.contains(env_tc)); - config.change_dir(&subdir, &|config| { - config.expect_stdout_ok(&["rustc", "--version"], "hash-nightly-2"); - }); + // +toolchain > env > dir override > file > default + let out = config.run( + "rustup", + [&format!("+{}", command_tc), "show", "active-toolchain"], + &[("RUSTUP_TOOLCHAIN", env_tc)], + ); + assert!(out.ok); + assert!(out.stdout.contains(command_tc)); }) }); } From 5a134442bf36147eb1c4b506df2b1fec70b3de65 Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Fri, 8 Dec 2023 10:36:11 +0000 Subject: [PATCH 06/11] 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 | 213 ++++++++++++-------------- src/config.rs | 4 +- tests/suite/cli_misc.rs | 2 +- tests/suite/cli_rustup.rs | 255 +++++++++++++++++++++----------- 5 files changed, 267 insertions(+), 212 deletions(-) diff --git a/doc/user-guide/src/overrides.md b/doc/user-guide/src/overrides.md index d3cfa56480..bfeef3922e 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 ac3bf7c6fc..37ec946bfc 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,120 +1082,99 @@ 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) { - match distributable.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(), + // So far, it is not possible to list targets for a custom toolchain. + ToolchainName::Custom(_) => None, + }) + .and_then(|distributable| distributable.components().ok()) + .map(|cs_vec| { + cs_vec + .into_iter() + .filter(|c| c.installed && c.component.short_name_in_manifest() == "rust-std") + .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_active_toolchain, is_default_toolchain) { + (true, true) => " (active, default)", + (true, false) => " (active)", + (false, true) => " (default)", + (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)?; - 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")?; + } } } @@ -1203,9 +1183,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(()) } @@ -1217,27 +1199,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(), &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 144867a4b6..b72ad39d6d 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 b1a2c358c4..1a3e60dcf3 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 14bc9dd73e..f70900f30c 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 From cf852f73799d92855bb5c113689081cea4e5e802 Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Fri, 8 Dec 2023 11:52:31 +0000 Subject: [PATCH 07/11] Update format of `toolchain list` Update the format of `rustup toolchain list` to have a similar flavour to the new `rustup show` format. --- src/cli/common.rs | 106 ++++++++++++++++++-------------------- tests/suite/cli_rustup.rs | 32 +++++++----- tests/suite/cli_v1.rs | 6 +-- tests/suite/cli_v2.rs | 6 +-- 4 files changed, 75 insertions(+), 75 deletions(-) diff --git a/src/cli/common.rs b/src/cli/common.rs index a3a0020ae1..340529c347 100644 --- a/src/cli/common.rs +++ b/src/cli/common.rs @@ -14,7 +14,6 @@ use once_cell::sync::Lazy; use super::self_update; use crate::cli::download_tracker::DownloadTracker; -use crate::config::ActiveReason; use crate::currentprocess::{ argsource::ArgSource, filesource::{StdinSource, StdoutSource}, @@ -24,6 +23,7 @@ use crate::currentprocess::{ use crate::dist::dist::{TargetTriple, ToolchainDesc}; use crate::dist::manifest::ComponentStatus; use crate::install::UpdateStatus; +use crate::toolchain::names::{LocalToolchainName, ToolchainName}; use crate::utils::notifications as util_notifications; use crate::utils::notify::NotificationLevel; use crate::utils::utils; @@ -426,78 +426,72 @@ fn list_items( Ok(utils::ExitCode(0)) } -fn print_toolchain_path( - cfg: &Cfg, - toolchain: &str, - if_default: &str, - if_override: &str, - verbose: bool, -) -> Result<()> { - let toolchain_path = cfg.toolchains_dir.join(toolchain); - let toolchain_meta = fs::symlink_metadata(&toolchain_path)?; - let toolchain_path = if verbose { - if toolchain_meta.is_dir() { - format!("\t{}", toolchain_path.display()) - } else { - format!("\t{}", fs::read_link(toolchain_path)?.display()) - } - } else { - String::new() - }; - writeln!( - process().stdout().lock(), - "{}{}{}{}", - &toolchain, - if_default, - if_override, - toolchain_path - )?; - Ok(()) -} - pub(crate) fn list_toolchains(cfg: &Cfg, verbose: bool) -> Result { - // Work with LocalToolchainName to accommodate path based overrides - let toolchains = cfg - .list_toolchains()? - .iter() - .map(Into::into) - .collect::>(); + let toolchains = cfg.list_toolchains()?; if toolchains.is_empty() { writeln!(process().stdout().lock(), "no installed toolchains")?; } else { - let def_toolchain_name = cfg.get_default()?.map(|t| (&t).into()); + let default_toolchain_name = cfg.get_default()?; let cwd = utils::current_dir()?; - let ovr_toolchain_name = - if let Ok(Some((toolchain, reason))) = cfg.find_active_toolchain(&cwd) { - match reason { - ActiveReason::Default => None, - _ => Some(toolchain), - } + let active_toolchain_name: Option = + if let Ok(Some((LocalToolchainName::Named(toolchain), _reason))) = + cfg.find_active_toolchain(&cwd) + { + Some(toolchain) } else { None }; + for toolchain in toolchains { - let if_default = if def_toolchain_name.as_ref() == Some(&toolchain) { - " (default)" - } else { - "" - }; - let if_override = if ovr_toolchain_name.as_ref() == Some(&toolchain) { - " (override)" - } else { - "" - }; + let is_default_toolchain = default_toolchain_name.as_ref() == Some(&toolchain); + let is_active_toolchain = active_toolchain_name.as_ref() == Some(&toolchain); - print_toolchain_path( + print_toolchain( cfg, &toolchain.to_string(), - if_default, - if_override, + is_default_toolchain, + is_active_toolchain, verbose, ) .context("Failed to list toolchains' directories")?; } } + + fn print_toolchain( + cfg: &Cfg, + toolchain: &str, + is_default: bool, + is_active: bool, + verbose: bool, + ) -> Result<()> { + let toolchain_path = cfg.toolchains_dir.join(toolchain); + let toolchain_meta = fs::symlink_metadata(&toolchain_path)?; + let toolchain_path = if verbose { + if toolchain_meta.is_dir() { + format!(" {}", toolchain_path.display()) + } else { + format!(" {}", fs::read_link(toolchain_path)?.display()) + } + } else { + String::new() + }; + let status_str = match (is_default, is_active) { + (true, true) => " (active, default)", + (true, false) => " (default)", + (false, true) => " (active)", + (false, false) => "", + }; + + writeln!( + process().stdout().lock(), + "{}{}{}", + &toolchain, + status_str, + toolchain_path + )?; + Ok(()) + } + Ok(utils::ExitCode(0)) } diff --git a/tests/suite/cli_rustup.rs b/tests/suite/cli_rustup.rs index f70900f30c..a07f2954b0 100644 --- a/tests/suite/cli_rustup.rs +++ b/tests/suite/cli_rustup.rs @@ -834,10 +834,7 @@ fn list_default_toolchain() { config.expect_ok(&["rustup", "default", "nightly"]); config.expect_ok_ex( &["rustup", "toolchain", "list"], - for_host!( - r"nightly-{0} (default) -" - ), + for_host!("nightly-{0} (active, default)\n"), r"", ); }) @@ -845,16 +842,28 @@ fn list_default_toolchain() { } #[test] -fn list_override_toolchain() { +fn list_no_default_toolchain() { + test(&|config| { + config.with_scenario(Scenario::SimpleV2, &|config| { + config.expect_ok(&["rustup", "install", "nightly"]); + config.expect_ok(&["rustup", "default", "none"]); + config.expect_ok_ex( + &["rustup", "toolchain", "list"], + for_host!("nightly-{0}\n"), + r"", + ); + }) + }); +} + +#[test] +fn list_no_default_override_toolchain() { test(&|config| { config.with_scenario(Scenario::SimpleV2, &|config| { config.expect_ok(&["rustup", "override", "set", "nightly"]); config.expect_ok_ex( &["rustup", "toolchain", "list"], - for_host!( - r"nightly-{0} (override) -" - ), + for_host!("nightly-{0} (active)\n"), r"", ); }) @@ -869,10 +878,7 @@ fn list_default_and_override_toolchain() { config.expect_ok(&["rustup", "override", "set", "nightly"]); config.expect_ok_ex( &["rustup", "toolchain", "list"], - for_host!( - r"nightly-{0} (default) (override) -" - ), + for_host!("nightly-{0} (active, default)\n"), r"", ); }) diff --git a/tests/suite/cli_v1.rs b/tests/suite/cli_v1.rs index 331b2747f6..b353c75c17 100644 --- a/tests/suite/cli_v1.rs +++ b/tests/suite/cli_v1.rs @@ -91,11 +91,11 @@ fn list_toolchains() { config.expect_ok(&["rustup", "update", "nightly"]); config.expect_ok(&["rustup", "update", "beta-2015-01-01"]); config.expect_stdout_ok(&["rustup", "toolchain", "list"], "nightly"); - config.expect_stdout_ok(&["rustup", "toolchain", "list", "-v"], "(default)\t"); + config.expect_stdout_ok(&["rustup", "toolchain", "list", "-v"], "(active, default) "); #[cfg(windows)] config.expect_stdout_ok( &["rustup", "toolchain", "list", "-v"], - for_host!("\\toolchains\\nightly-{}"), + for_host!(r"\toolchains\nightly-{}"), ); #[cfg(not(windows))] config.expect_stdout_ok( @@ -106,7 +106,7 @@ fn list_toolchains() { #[cfg(windows)] config.expect_stdout_ok( &["rustup", "toolchain", "list", "-v"], - "\\toolchains\\beta-2015-01-01", + r"\toolchains\beta-2015-01-01", ); #[cfg(not(windows))] config.expect_stdout_ok( diff --git a/tests/suite/cli_v2.rs b/tests/suite/cli_v2.rs index 81ba41e75d..0a38d3ee44 100644 --- a/tests/suite/cli_v2.rs +++ b/tests/suite/cli_v2.rs @@ -129,11 +129,11 @@ fn list_toolchains() { config.expect_ok(&["rustup", "update", "nightly"]); config.expect_ok(&["rustup", "update", "beta-2015-01-01"]); config.expect_stdout_ok(&["rustup", "toolchain", "list"], "nightly"); - config.expect_stdout_ok(&["rustup", "toolchain", "list", "-v"], "(default)\t"); + config.expect_stdout_ok(&["rustup", "toolchain", "list", "-v"], "(active, default) "); #[cfg(windows)] config.expect_stdout_ok( &["rustup", "toolchain", "list", "-v"], - for_host!("\\toolchains\\nightly-{}"), + for_host!(r"\toolchains\nightly-{}"), ); #[cfg(not(windows))] config.expect_stdout_ok( @@ -144,7 +144,7 @@ fn list_toolchains() { #[cfg(windows)] config.expect_stdout_ok( &["rustup", "toolchain", "list", "-v"], - "\\toolchains\\beta-2015-01-01", + r"\toolchains\beta-2015-01-01", ); #[cfg(not(windows))] config.expect_stdout_ok( From 12f737413ba31220cb8ecc5d30cbb8bb17e02d3f Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Fri, 8 Dec 2023 12:06:55 +0000 Subject: [PATCH 08/11] Make `rustup default` not error if no default `rustup default` will no longer error and print to stderr if there is no default toolchain configured. It will print to stdout and exit successfully instead. --- src/cli/rustup_mode.rs | 13 +++++++++---- tests/suite/cli_exact.rs | 7 +++++++ tests/suite/cli_misc.rs | 7 ------- tests/suite/cli_rustup.rs | 2 +- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index 37ec946bfc..6e9a820c66 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -872,10 +872,15 @@ fn default_(cfg: &Cfg, m: &ArgMatches) -> Result { } } } else { - let default_toolchain = cfg - .get_default()? - .ok_or_else(|| anyhow!("no default toolchain configured"))?; - writeln!(process().stdout().lock(), "{default_toolchain} (default)")?; + match cfg.get_default()? { + Some(default_toolchain) => { + writeln!(process().stdout().lock(), "{default_toolchain} (default)")?; + } + None => writeln!( + process().stdout().lock(), + "no default toolchain is configured" + )?, + } } Ok(utils::ExitCode(0)) diff --git a/tests/suite/cli_exact.rs b/tests/suite/cli_exact.rs index fc92c22ade..383d8f5bb1 100644 --- a/tests/suite/cli_exact.rs +++ b/tests/suite/cli_exact.rs @@ -577,6 +577,13 @@ fn default_none() { &["rustup", "default", "none"], "info: default toolchain unset", ); + + config.expect_ok_ex( + &["rustup", "default"], + "no default toolchain is configured\n", + "", + ); + config.expect_err_ex( &["rustc", "--version"], "", diff --git a/tests/suite/cli_misc.rs b/tests/suite/cli_misc.rs index 1a3e60dcf3..63e1720e97 100644 --- a/tests/suite/cli_misc.rs +++ b/tests/suite/cli_misc.rs @@ -477,13 +477,6 @@ fn toolchains_are_resolved_early() { }); } -#[test] -fn no_panic_on_default_toolchain_missing() { - setup(&|config| { - config.expect_err(&["rustup", "default"], "no default toolchain configured"); - }); -} - // #190 #[test] fn proxies_pass_empty_args() { diff --git a/tests/suite/cli_rustup.rs b/tests/suite/cli_rustup.rs index a07f2954b0..89d4f85a9f 100644 --- a/tests/suite/cli_rustup.rs +++ b/tests/suite/cli_rustup.rs @@ -2482,7 +2482,7 @@ fn check_unix_settings_fallback() { test(&|config| { config.with_scenario(Scenario::SimpleV2, &|config| { // No default toolchain specified yet - config.expect_err(&["rustup", "default"], r"no default toolchain configured"); + config.expect_stdout_ok(&["rustup", "default"], "no default toolchain is configured"); // Default toolchain specified in fallback settings file let mock_settings_file = config.current_dir().join("mock_fallback_settings.toml"); From 827e625263212b20f74da09cbc3f6a03d2c9ee37 Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Fri, 8 Dec 2023 12:30:14 +0000 Subject: [PATCH 09/11] Fix doc error with `rust-toolchain.toml` custom TC Fix an issue where the documentation erroneously states that `rust-toolchain.toml` config files can't specify custom toolchain names. This language seems to be very old, ultimately inherited from the original commit that added this feature, and is no longer true: https://github.com/rust-lang/rustup/commit/107d8e5f1ab83ce13cb33a7b4ca0f58198285ee8#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R330 --- doc/user-guide/src/overrides.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/doc/user-guide/src/overrides.md b/doc/user-guide/src/overrides.md index bfeef3922e..56923652b9 100644 --- a/doc/user-guide/src/overrides.md +++ b/doc/user-guide/src/overrides.md @@ -120,16 +120,12 @@ The `channel` setting specifies which [toolchain] to use. The value is a string in the following form: ``` -[-] +([-])| = stable|beta|nightly| = YYYY-MM-DD ``` -Note that this is a more restricted form than `rustup` toolchains -generally, and cannot be used to specify custom toolchains or -host-specific toolchains. - [toolchain]: concepts/toolchains.md #### path From 2f9a8d06ddbacd956a5ab2d84b47757aa6e543bc Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Wed, 20 Dec 2023 19:54:42 +0000 Subject: [PATCH 10/11] Make settings file allow multiple borrows --- src/settings.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/settings.rs b/src/settings.rs index 02e445278e..59fd8461e9 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -38,9 +38,10 @@ impl SettingsFile { fn read_settings(&self) -> Result<()> { let mut needs_save = false; { - let mut b = self.cache.borrow_mut(); + let b = self.cache.borrow(); if b.is_none() { - *b = Some(if utils::is_file(&self.path) { + drop(b); + *self.cache.borrow_mut() = Some(if utils::is_file(&self.path) { let content = utils::read_file("settings", &self.path)?; Settings::parse(&content)? } else { From b3efa0ce1c941fe68290a8918e4e95db64cab767 Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Wed, 20 Dec 2023 19:55:47 +0000 Subject: [PATCH 11/11] Make find_override_from_dir_walk return OverrideCfg --- src/config.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/config.rs b/src/config.rs index b72ad39d6d..9b81e244b5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -576,10 +576,10 @@ impl Cfg { // Then walk up the directory tree from 'path' looking for either the // directory in the override database, or a `rust-toolchain{.toml}` file, // in that order. - else if let Some((override_file, active_reason)) = self.settings_file.with(|s| { + else if let Some((override_cfg, active_reason)) = self.settings_file.with(|s| { self.find_override_from_dir_walk(path, s) })? { - Some((OverrideCfg::from_file(self, override_file)?, active_reason)) + Some((override_cfg, active_reason)) } // Otherwise, there is no override. else { @@ -593,7 +593,7 @@ impl Cfg { &self, dir: &Path, settings: &Settings, - ) -> Result> { + ) -> Result> { let notify = self.notify_handler.as_ref(); let mut dir = Some(dir); @@ -601,7 +601,16 @@ impl Cfg { // First check the override database if let Some(name) = settings.dir_override(d, notify) { let reason = ActiveReason::OverrideDB(d.to_owned()); - return Ok(Some((name.into(), reason))); + // Note that `rustup override set` fully resolves it's input + // before writing to settings.toml, so resolving here may not + // be strictly necessary (could instead model as ToolchainName). + // However, settings.toml could conceivably be hand edited to + // have an unresolved name. I'm just preserving pre-existing + // behaviour by choosing ResolvableToolchainName here. + let toolchain_name = ResolvableToolchainName::try_from(name)? + .resolve(&get_default_host_triple(settings))?; + let override_cfg = toolchain_name.into(); + return Ok(Some((override_cfg, reason))); } // Then look for 'rust-toolchain' or 'rust-toolchain.toml' @@ -674,7 +683,8 @@ impl Cfg { } let reason = ActiveReason::ToolchainFile(toolchain_file); - return Ok(Some((override_file, reason))); + let override_cfg = OverrideCfg::from_file(self, override_file)?; + return Ok(Some((override_cfg, reason))); } dir = d.parent();