From 757436c256bbf45f35076e6d5f48bdaf998f04c2 Mon Sep 17 00:00:00 2001 From: rami3l Date: Mon, 25 Sep 2023 10:27:02 +0800 Subject: [PATCH 1/7] Make `find_override_config()` hierarchical --- src/config.rs | 79 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 64 insertions(+), 15 deletions(-) diff --git a/src/config.rs b/src/config.rs index ed19012e47..6977cdd445 100644 --- a/src/config.rs +++ b/src/config.rs @@ -55,6 +55,15 @@ impl OverrideFile { fn is_empty(&self) -> bool { self.toolchain.is_empty() } + + fn has_toolchain(&self) -> bool { + self.toolchain.has_toolchain() + } + + /// A left-biased merge of two [`OverrideFile`]s. + fn merge(&mut self, other: Self) { + self.toolchain.merge(other.toolchain) + } } #[derive(Debug, Default, Deserialize, PartialEq, Eq)] @@ -69,9 +78,33 @@ struct ToolchainSection { impl ToolchainSection { fn is_empty(&self) -> bool { self.channel.is_none() + && self.path.is_none() && self.components.is_none() && self.targets.is_none() - && self.path.is_none() + } + + fn has_toolchain(&self) -> bool { + self.channel.is_some() || self.path.is_some() + } + + /// A left-biased merge of two [`ToolchainSelection`]s. + /// + /// If a field appears in both operands, the one from the left-hand side is selected. + fn merge(&mut self, other: Self) { + if !self.has_toolchain() { + // `channel` and `path` are mutually exclusive, so they need to be updated together, + // and this will happen only if `self` doesn't have a toolchain yet. + (self.channel, self.path) = (other.channel, other.path); + } + if self.components.is_none() { + self.components = other.components; + } + if self.targets.is_none() { + self.targets = other.targets; + } + if self.profile.is_none() { + self.profile = other.profile; + } } } @@ -96,6 +129,7 @@ impl> From for OverrideFile { } } +/// The reason for which the toolchain is overridden. #[derive(Debug)] pub(crate) enum OverrideReason { Environment, @@ -453,31 +487,46 @@ impl Cfg { } fn find_override_config(&self, path: &Path) -> Result> { - let mut override_ = None; + let mut override_ = None::<(OverrideFile, OverrideReason)>; + + let mut update_override = |file, reason| { + if let Some((file1, reason1)) = &mut override_ { + if file1.has_toolchain() { + // Update the reason only if the override file has a toolchain. + *reason1 = reason + } + file1.merge(file); + } else { + override_ = Some((file, reason)) + }; + }; + + // Check for all possible toolchain overrides below... + // See: - // First check toolchain override from command + // 1. Check toolchain override from command (i.e. the `+toolchain` syntax) if let Some(ref name) = self.toolchain_override { - override_ = Some((name.to_string().into(), OverrideReason::CommandLine)); + update_override(name.to_string().into(), OverrideReason::CommandLine); } - // Check RUSTUP_TOOLCHAIN + // 2. 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(), OverrideReason::Environment)); + update_override(name.to_string().into(), OverrideReason::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(()) - })?; - } + // 3. walk up the directory tree from 'path' looking for either the + // directory in override database, or + // 4. a `rust-toolchain` file. + self.settings_file.with(|s| { + if let Some((file, reason)) = self.find_override_from_dir_walk(path, s)? { + update_override(file, reason); + } + Ok(()) + })?; if let Some((file, reason)) = override_ { // This is hackishly using the error chain to provide a bit of From ec8b7c4ee4b6d19e48a03775fb35c98dd2801848 Mon Sep 17 00:00:00 2001 From: rami3l Date: Mon, 25 Sep 2023 12:41:09 +0800 Subject: [PATCH 2/7] Extract `update_override()` --- src/config.rs | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/config.rs b/src/config.rs index 6977cdd445..cc1f73da7c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -487,26 +487,18 @@ impl Cfg { } fn find_override_config(&self, path: &Path) -> Result> { - let mut override_ = None::<(OverrideFile, OverrideReason)>; - - let mut update_override = |file, reason| { - if let Some((file1, reason1)) = &mut override_ { - if file1.has_toolchain() { - // Update the reason only if the override file has a toolchain. - *reason1 = reason - } - file1.merge(file); - } else { - override_ = Some((file, reason)) - }; - }; + let mut override_ = None; // Check for all possible toolchain overrides below... // See: // 1. Check toolchain override from command (i.e. the `+toolchain` syntax) if let Some(ref name) = self.toolchain_override { - update_override(name.to_string().into(), OverrideReason::CommandLine); + update_override( + &mut override_, + name.to_string().into(), + OverrideReason::CommandLine, + ); } // 2. Check RUSTUP_TOOLCHAIN @@ -515,7 +507,11 @@ 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 - update_override(name.to_string().into(), OverrideReason::Environment); + update_override( + &mut override_, + name.to_string().into(), + OverrideReason::Environment, + ); } // 3. walk up the directory tree from 'path' looking for either the @@ -523,7 +519,7 @@ impl Cfg { // 4. a `rust-toolchain` file. self.settings_file.with(|s| { if let Some((file, reason)) = self.find_override_from_dir_walk(path, s)? { - update_override(file, reason); + update_override(&mut override_, file, reason); } Ok(()) })?; @@ -1006,6 +1002,22 @@ impl Cfg { } } +fn update_override( + override_: &mut Option<(OverrideFile, OverrideReason)>, + file: OverrideFile, + reason: OverrideReason, +) { + if let Some((file1, reason1)) = override_ { + if file1.has_toolchain() { + // Update the reason only if the override file has a toolchain. + *reason1 = reason + } + file1.merge(file); + } else { + *override_ = Some((file, reason)) + }; +} + fn get_default_host_triple(s: &Settings) -> dist::TargetTriple { s.default_host_triple .as_ref() From 3f5e2173e427329920a217754230000ccf378dd3 Mon Sep 17 00:00:00 2001 From: rami3l Date: Sat, 4 Nov 2023 20:28:26 +0800 Subject: [PATCH 3/7] Use `iter::successors` in `find_override_from_dir_walk()` --- src/config.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/config.rs b/src/config.rs index cc1f73da7c..4df8f50848 100644 --- a/src/config.rs +++ b/src/config.rs @@ -581,9 +581,8 @@ impl Cfg { settings: &Settings, ) -> Result> { let notify = self.notify_handler.as_ref(); - let mut dir = Some(dir); - while let Some(d) = dir { + for d in iter::successors(Some(dir), |d| d.parent()) { // First check the override database if let Some(name) = settings.dir_override(d, notify) { let reason = OverrideReason::OverrideDB(d.to_owned()); @@ -664,8 +663,6 @@ impl Cfg { let reason = OverrideReason::ToolchainFile(toolchain_file); return Ok(Some((override_file, reason))); } - - dir = d.parent(); } Ok(None) From 15bf1cdf2a659313a9505341a94e5706eed6b615 Mon Sep 17 00:00:00 2001 From: rami3l Date: Sat, 4 Nov 2023 20:29:42 +0800 Subject: [PATCH 4/7] Make `find_override_from_dir_walk()` hierarchical --- src/config.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/config.rs b/src/config.rs index 4df8f50848..b2bffa0d29 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,10 +1,10 @@ -use std::borrow::Cow; use std::fmt::{self, Display}; use std::io; use std::path::{Path, PathBuf}; use std::process::Command; use std::str::FromStr; use std::sync::Arc; +use std::{borrow::Cow, iter}; use anyhow::{anyhow, bail, Context, Result}; use derivative::Derivative; @@ -580,13 +580,15 @@ impl Cfg { dir: &Path, settings: &Settings, ) -> Result> { + let mut override_ = None; + let notify = self.notify_handler.as_ref(); for d in iter::successors(Some(dir), |d| d.parent()) { // First check the override database if let Some(name) = settings.dir_override(d, notify) { let reason = OverrideReason::OverrideDB(d.to_owned()); - return Ok(Some((name.into(), reason))); + update_override(&mut override_, name.into(), reason); } // Then look for 'rust-toolchain' or 'rust-toolchain.toml' @@ -661,11 +663,15 @@ impl Cfg { } let reason = OverrideReason::ToolchainFile(toolchain_file); - return Ok(Some((override_file, reason))); + update_override(&mut override_, override_file, reason); + } + + if override_.is_some() { + break; } } - Ok(None) + Ok(override_) } fn parse_override_file>( From 60e0b76214f718d614e7ef7f9e44b9fb9b821379 Mon Sep 17 00:00:00 2001 From: rami3l Date: Tue, 10 Oct 2023 10:21:24 +0800 Subject: [PATCH 5/7] Add unit tests for `Cfg` overriding --- src/config.rs | 165 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 164 insertions(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index b2bffa0d29..a3e81515ab 100644 --- a/src/config.rs +++ b/src/config.rs @@ -36,6 +36,9 @@ use crate::{ utils::utils, }; +#[cfg(feature = "test")] +use crate::test::mock::clitools; + #[derive(Debug, ThisError)] enum OverrideFileConfigError { #[error("empty toolchain override file detected. Please remove it, or else specify the desired toolchain properties in the file")] @@ -149,7 +152,7 @@ impl Display for OverrideReason { } } -#[derive(Default, Debug)] +#[derive(Default, Debug, PartialEq)] struct OverrideCfg { toolchain: Option, components: Vec, @@ -1005,6 +1008,29 @@ impl Cfg { } } +#[cfg(feature = "test")] +impl From for Cfg { + fn from(cfg: clitools::Config) -> Self { + let rustup_dir = &cfg.rustupdir; + let dist_root_server = dist::DEFAULT_DIST_SERVER; + + Self { + rustup_dir: rustup_dir.rustupdir.clone(), + fallback_settings: None, + settings_file: SettingsFile::new(rustup_dir.join("settings.toml")), + toolchains_dir: rustup_dir.join("toolchains"), + update_hash_dir: rustup_dir.join("update-hashes"), + download_dir: rustup_dir.join("downloads"), + dist_root_url: dist_root_server.to_owned() + "/dist", + temp_cfg: temp::Cfg::new(rustup_dir.join("tmp"), dist_root_server, Box::new(|_| {})), + toolchain_override: None, + profile_override: None, + env_override: None, + notify_handler: Arc::new(|_| {}), + } + } +} + fn update_override( override_: &mut Option<(OverrideFile, OverrideReason)>, file: OverrideFile, @@ -1046,6 +1072,11 @@ enum ParseMode { mod tests { use rustup_macros::unit_test as test; + use crate::{ + test::{mock::clitools::setup_test_state, test_dist_dir}, + utils::raw, + }; + use super::*; #[test] @@ -1248,4 +1279,136 @@ channel = nightly Ok(OverrideFileConfigError::Parsing) )); } + + /// Ensures that `rust-toolchain.toml` configs can be overridden by ` +`. + /// See: + #[test] + fn toolchain_override_beats_toml() { + let test_dist_dir = test_dist_dir().unwrap(); + let (cwd, config) = setup_test_state(test_dist_dir); + + let toolchain_file = cwd.path().join("rust-toolchain.toml"); + raw::write_file( + &toolchain_file, + r#" + [toolchain] + channel = "nightly" + components = [ "rls" ] + "#, + ) + .unwrap(); + + let mut cfg = Cfg::try_from(config).unwrap(); + let default_host_triple = cfg.get_default_host_triple().unwrap(); + cfg.toolchain_override = Some("beta".try_into().unwrap()); + + let found_override = cfg.find_override_config(cwd.path()).unwrap(); + let override_cfg = found_override.map(|it| it.0); + + let expected_override_cfg = OverrideCfg { + toolchain: Some(LocalToolchainName::Named(ToolchainName::Official( + ToolchainDesc { + channel: "beta".to_owned(), + date: None, + target: default_host_triple, + }, + ))), + components: vec!["rls".to_owned()], + targets: vec![], + profile: None, + }; + assert_eq!(override_cfg, Some(expected_override_cfg)); + } + + /// Ensures that `rust-toolchain.toml` configs can be overridden by `RUSTUP_TOOLCHAIN`. + /// See: + #[test] + fn env_override_beats_toml() { + let test_dist_dir = test_dist_dir().unwrap(); + let (cwd, config) = setup_test_state(test_dist_dir); + + let toolchain_file = cwd.path().join("rust-toolchain.toml"); + raw::write_file( + &toolchain_file, + r#" + [toolchain] + channel = "nightly" + components = [ "rls" ] + "#, + ) + .unwrap(); + + let mut cfg = Cfg::try_from(config).unwrap(); + let default_host_triple = cfg.get_default_host_triple().unwrap(); + cfg.env_override = Some( + ResolvableLocalToolchainName::try_from("beta") + .unwrap() + .resolve(&default_host_triple) + .unwrap(), + ); + + let found_override = cfg.find_override_config(cwd.path()).unwrap(); + let override_cfg = found_override.map(|it| it.0); + + let expected_override_cfg = OverrideCfg { + toolchain: Some(LocalToolchainName::Named(ToolchainName::Official( + ToolchainDesc { + channel: "beta".to_owned(), + date: None, + target: default_host_triple, + }, + ))), + components: vec!["rls".to_owned()], + targets: vec![], + profile: None, + }; + assert_eq!(override_cfg, Some(expected_override_cfg)); + } + + /// Ensures that `rust-toolchain.toml` configs can be overridden by `rustup override set`. + /// See: + #[test] + fn override_set_beats_toml() { + let test_dist_dir = test_dist_dir().unwrap(); + let (cwd, config) = setup_test_state(test_dist_dir); + + let toolchain_file = cwd.path().join("rust-toolchain.toml"); + raw::write_file( + &toolchain_file, + r#" + [toolchain] + channel = "nightly" + components = [ "rls" ] + "#, + ) + .unwrap(); + + let cfg = Cfg::try_from(config).unwrap(); + let default_host_triple = cfg.get_default_host_triple().unwrap(); + + cfg.settings_file + .with_mut(|settings| { + // "." -> beta, no rls + settings.add_override(cwd.path(), "beta".to_owned(), &|_| {}); + Ok(()) + }) + .unwrap(); + + let found_override = cfg.find_override_config(cwd.path()).unwrap(); + let override_cfg = found_override.map(|it| it.0); + + let expected_override_cfg = OverrideCfg { + toolchain: Some(LocalToolchainName::Named(ToolchainName::Official( + ToolchainDesc { + channel: "beta".to_owned(), + date: None, + target: default_host_triple, + }, + ))), + components: vec!["rls".to_owned()], + targets: vec![], + profile: None, + }; + assert_eq!(override_cfg, Some(expected_override_cfg)); + } } From 2e24b258210fc76da01f73ddbe2c98e3c9aa2465 Mon Sep 17 00:00:00 2001 From: rami3l Date: Sat, 4 Nov 2023 17:29:51 +0800 Subject: [PATCH 6/7] Test `file_override_beats_existing_toolchain()` --- tests/suite/cli_rustup.rs | 43 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/suite/cli_rustup.rs b/tests/suite/cli_rustup.rs index 9a6adde7a4..bd486e740c 100644 --- a/tests/suite/cli_rustup.rs +++ b/tests/suite/cli_rustup.rs @@ -2027,6 +2027,49 @@ fn plus_override_beats_file_override() { }); } +// Ensures that the specified toolchain components and targets still work even if the toolchain is already installed. +// See: +#[test] +fn file_override_beats_existing_toolchain() { + use clitools::MULTI_ARCH1; + + test(&|config| { + config.with_scenario(Scenario::MultiHost, &|config| { + let beta = "hash-beta-1.2.0"; + config.expect_ok(&[ + "rustup", + "toolchain", + "install", + "beta", + "--profile=minimal", + ]); + config.expect_ok(&["rustup", "override", "set", "beta"]); + + let cwd = config.current_dir(); + let toolchain_file = cwd.join("rust-toolchain.toml"); + raw::write_file( + &toolchain_file, + &format!( + r#" + [toolchain] + channel = "beta" + components = [ "rls" ] + targets = [ "{MULTI_ARCH1}" ] + "#, + ), + ) + .unwrap(); + + // Implicitly install the missing components and targets. + config.expect_stdout_ok(&["rustc", "--version"], beta); + + let list_installed = &["rustup", "component", "list", "--installed"]; + config.expect_stdout_ok(list_installed, "rls-"); + config.expect_stdout_ok(list_installed, &format!("rust-std-{MULTI_ARCH1}")); + }) + }); +} + #[test] fn file_override_not_installed_custom() { test(&|config| { From 4e32bbe97d0a25afdeede305cf6906e5f6175f8c Mon Sep 17 00:00:00 2001 From: rami3l Date: Sat, 4 Nov 2023 21:11:37 +0800 Subject: [PATCH 7/7] Introduce hierarchical override config in the user guide --- doc/user-guide/src/overrides.md | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/doc/user-guide/src/overrides.md b/doc/user-guide/src/overrides.md index 3115b96bfc..28ff593c8e 100644 --- a/doc/user-guide/src/overrides.md +++ b/doc/user-guide/src/overrides.md @@ -12,14 +12,18 @@ and override which toolchain is used: 5. The [default toolchain]. The toolchain is chosen in the order listed above, using the first one that is -specified. There is one exception though: directory overrides and the -`rust-toolchain.toml` file are also preferred by their proximity to the current -directory. That is, these two override methods are discovered by walking up -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`, +specified. There are a number of caveats though: + +- Directory overrides and `rust-toolchain.toml` overrides are determined together. + Since each of them is bounded to the directory it was set in, to find out + which of them to use, `rustup` walks up the directory tree towards the + filesystem root, and whichever has its bounded directory closest to the + current directory wins. If that same closest directory has both overrides, + then `rust-toolchain.toml` is overridden by the directory override. +- When overriding a `rust-toolchain.toml`, its `components` and `targets` + will be carried over to the new toolchain. + +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].)