From 6e569ecbfa71f00a7a4984780b89c89c117aef1f Mon Sep 17 00:00:00 2001 From: Matt Harding Date: Wed, 20 Dec 2023 21:53:51 +0000 Subject: [PATCH] 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)); }) }); }