Tests passing

rbtcollins · rbtcollins · commit 379918da1dae · 2023-04-28T07:18:08.000+02:00
diff --git a/src/config.rs b/src/config.rs
@@ -23,7 +23,8 @@ use crate::notifications::*;
 use crate::process;
 use crate::settings::{Settings, SettingsFile, DEFAULT_METADATA_VERSION};
 use crate::toolchain::{
-    LocalToolchainName, PathBasedToolchain, ResolvableToolchainName, ToolchainName,
+    LocalToolchainName, PathBasedToolchain, ResolvableLocalToolchainName, ResolvableToolchainName,
+    ToolchainName,
 };
 use crate::toolchain_old::{
     InstalledToolchain, OwnedCustomToolchain, OwnedDistributableToolchain, OwnedInstalled,
@@ -177,7 +178,7 @@ pub(crate) struct Cfg {
     pub download_dir: PathBuf,
     pub temp_cfg: temp::Cfg,
     pub toolchain_override: Option<ResolvableToolchainName>,
-    pub env_override: Option<String>,
+    pub env_override: Option<LocalToolchainName>,
     pub dist_root_url: String,
     pub notify_handler: Arc<dyn Fn(Notification<'_>)>,
 }
@@ -209,11 +210,17 @@ impl Cfg {
         let update_hash_dir = rustup_dir.join("update-hashes");
         let download_dir = rustup_dir.join("downloads");
 
+        // Figure out get_default_host_triple before Config is populated
+        let default_host_triple = settings_file.with(|s| Ok(get_default_host_triple(s)))?;
         // Environment override
         let env_override = process()
             .var("RUSTUP_TOOLCHAIN")
             .ok()
-            .and_then(utils::if_not_empty);
+            .and_then(utils::if_not_empty)
+            .map(|s| ResolvableLocalToolchainName::try_from(s))
+            .transpose()?
+            .map(|t| t.resolve(&default_host_triple))
+            .transpose()?;
 
         let dist_root_server = match process().var("RUSTUP_DIST_SERVER") {
             Ok(ref s) if !s.is_empty() => s.clone(),
@@ -458,10 +465,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((
-                LocalToolchainName::try_from(name)?.to_string().into(),
-                OverrideReason::Environment,
-            ));
+            override_ = Some((name.to_string().into(), OverrideReason::Environment));
         }
 
         // Then walk up the directory tree from 'path' looking for either the
@@ -574,25 +578,29 @@ impl Cfg {
                 // internal only safe struct
                 let override_file = Cfg::parse_override_file(contents, parse_mode)
                     .with_context(add_file_context)?;
-                if let Some(toolchain_name) = &override_file.toolchain.channel {
-                    let toolchain_name =
-                        ResolvableToolchainName::try_from(&toolchain_name as &str)?;
+                if let Some(toolchain_name_str) = &override_file.toolchain.channel {
+                    let toolchain_name = ResolvableToolchainName::try_from(toolchain_name_str)?;
+                    let default_host_triple = get_default_host_triple(settings);
                     // Do not permit architecture/os selection in channels as
                     // these are host specific and toolchain files are portable.
                     if let ResolvableToolchainName::Official(ref name) = toolchain_name {
                         if name.has_triple() {
-                            return Err(anyhow!(format!("target triple in channel name '{name}'")));
+                            // Permit fully qualified names IFF the toolchain is installed. TODO(robertc): consider
+                            // disabling this and backing out https://github.com/rust-lang/rustup/pull/2141 (but provide
+                            // the base name in the error to help users)
+                            let resolved_name = &ToolchainName::try_from(toolchain_name_str)?;
+                            let ts = self.list_toolchains()?;
+                            eprintln!("{resolved_name:?} {ts:?}");
+                            if !self.list_toolchains()?.iter().any(|s| s == resolved_name) {
+                                return Err(anyhow!(format!(
+                                    "target triple in channel name '{name}'"
+                                )));
+                            }
                         }
                     }
 
                     // XXX: this awkwardness deals with settings file being locked already
-                    let toolchain_name = toolchain_name.resolve(
-                        &settings
-                            .default_host_triple
-                            .as_ref()
-                            .map(|s| dist::TargetTriple::new(s))
-                            .unwrap_or_else(dist::TargetTriple::from_host_or_build),
-                    )?;
+                    let toolchain_name = toolchain_name.resolve(&default_host_triple)?;
                     let toolchain = Toolchain::from_name(self, &(&toolchain_name).into());
                     if !toolchain.exists() && matches!(toolchain_name, ToolchainName::Custom(_)) {
                         bail!(
@@ -759,23 +767,19 @@ impl Cfg {
     /// List all the installed toolchains: that is paths in the toolchain dir
     /// that are:
     /// - not files
-    /// - named with a valid toolchain name
+    /// - named with a valid resolved toolchain name
+    /// Currently no notification of incorrect names or entry type is done.
     #[cfg_attr(feature = "otel", tracing::instrument(skip_all))]
     pub(crate) fn list_toolchains(&self) -> Result<Vec<ToolchainName>> {
         if utils::is_directory(&self.toolchains_dir) {
-            let default_host_triple = &self.get_default_host_triple()?;
             let mut toolchains: Vec<_> = utils::read_dir("toolchains", &self.toolchains_dir)?
                 // TODO: this discards errors reading the directory, is that
                 // correct? could we get a short-read and report less toolchains
                 // than exist?
                 .filter_map(io::Result::ok)
                 .filter(|e| e.file_type().map(|f| !f.is_file()).unwrap_or(false))
                 .filter_map(|e| e.file_name().into_string().ok())
-                .filter_map(|n| {
-                    ResolvableToolchainName::try_from(n)
-                        .ok()
-                        .and_then(|n| n.resolve(default_host_triple).ok())
-                })
+                .filter_map(|n| ToolchainName::try_from(&n).ok())
                 .collect();
 
             crate::toolchain::toolchain_sort(&mut toolchains);
@@ -954,12 +958,7 @@ impl Cfg {
     pub(crate) fn get_default_host_triple(&self) -> Result<dist::TargetTriple> {
         Ok(self
             .settings_file
-            .with(|s| {
-                Ok(s.default_host_triple
-                    .as_ref()
-                    .map(|s| dist::TargetTriple::new(s)))
-            })?
-            .unwrap_or_else(dist::TargetTriple::from_host_or_build))
+            .with(|s| Ok(get_default_host_triple(s)))?)
     }
 
     pub(crate) fn toolchain_path(&self, toolchain: &LocalToolchainName) -> PathBuf {
@@ -970,6 +969,13 @@ impl Cfg {
     }
 }
 
+fn get_default_host_triple(s: &Settings) -> dist::TargetTriple {
+    s.default_host_triple
+        .as_ref()
+        .map(|s| dist::TargetTriple::new(s))
+        .unwrap_or_else(dist::TargetTriple::from_host_or_build)
+}
+
 /// Specifies how a `rust-toolchain`/`rust-toolchain.toml` configuration file should be parsed.
 enum ParseMode {
     /// Only permit TOML format in a configuration file.
diff --git a/src/dist/dist.rs b/src/dist/dist.rs
@@ -105,7 +105,7 @@ struct ParsedToolchainDesc {
 // 'stable-msvc' to work. Partial target triples though are parsed
 // from a hardcoded set of known triples, whereas target triples
 // are nearly-arbitrary strings.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord)]
 pub struct PartialToolchainDesc {
     // Either "nightly", "stable", "beta", or an explicit version number
     pub channel: String,
diff --git a/src/dist/triple.rs b/src/dist/triple.rs
@@ -47,7 +47,7 @@ static LIST_ENVS: &[&str] = &[
     "musl",
 ];
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
 pub struct PartialTargetTriple {
     pub arch: Option<String>,
     pub os: Option<String>,
diff --git a/src/toolchain.rs b/src/toolchain.rs
@@ -2,7 +2,7 @@
 //!
 //! From the user (including config files, toolchain files and manifests) we get
 //! a String. Strings are convertable into `MaybeOfficialToolchainName`,
-//! `ResolvableToolchainName`, and `LocalToolchainName`.
+//! `ResolvableToolchainName`, and `ResolvableLocalToolchainName`.
 //!
 //! `MaybeOfficialToolchainName` represents a toolchain passed to rustup-init:
 //! 'none' to select no toolchain to install, and otherwise a partial toolchain
@@ -15,11 +15,15 @@
 //! for both custom and official names.
 //!
 //! `ToolchainName` is the result of resolving `ResolvableToolchainName` with a
-//! host triple.
+//! host triple, or parsing an installed toolchain name directly.
+//!
+//! `ResolvableLocalToolchainName` represents the values permittable in
+//! `RUSTUP_TOOLCHAIN`: resolved or not resolved official names, custom names,
+//! and absolute paths.
 //!
 //! `LocalToolchainName` represents all the toolchain names that can make sense
-//! for referring to locally present toolchains. One of a
-//! `ResolvedToolchainName` or an absolute path.
+//! for referring to actually present toolchains. One of a `ToolchainName` or an
+//! absolute path.
 //!
 //! From the toolchains directory we can iterate directly over
 //! `ResolvedToolchainName`.
@@ -123,7 +127,7 @@ pub(crate) fn partial_toolchain_desc_parser(
 }
 
 /// A toolchain name from user input.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
 pub(crate) enum ResolvableToolchainName {
     Custom(CustomToolchainName),
     Official(PartialToolchainDesc),
@@ -338,29 +342,60 @@ pub(crate) fn toolchain_sort(v: &mut [ToolchainName]) {
     });
 }
 
-/// LocalToolchainName can be used in calls to Cfg that alter configuration,
-/// like setting overrides, or that depend on configuration, like calculating
-/// the toolchain directory. It is also used to model the RUSTUP_TOOLCHAIN
-/// variable, as that can take any possible resolved toolchain value.
+/// ResolvableLocalToolchainName is used to process values set in
+/// RUSTUP_TOOLCHAIN: resolvable and resolved official names, custom names and
+/// absolute paths.
 #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
-pub(crate) enum LocalToolchainName {
-    Named(ToolchainName),
+pub(crate) enum ResolvableLocalToolchainName {
+    Named(ResolvableToolchainName),
     Path(PathBasedToolchain),
 }
 
-impl LocalToolchainName {
-    /// Validates if the string is a concrete toolchain, or a custom toolchain, or a path based toolchain.
-    fn validate(candidate: &str) -> Result<LocalToolchainName, Error> {
+impl ResolvableLocalToolchainName {
+    /// Resolve to a concrete toolchain name
+    pub fn resolve(&self, host: &TargetTriple) -> Result<LocalToolchainName, anyhow::Error> {
+        match self.clone() {
+            ResolvableLocalToolchainName::Named(t) => {
+                Ok(LocalToolchainName::Named(t.resolve(host)?))
+            }
+            ResolvableLocalToolchainName::Path(t) => Ok(LocalToolchainName::Path(t)),
+        }
+    }
+
+    /// Validates if the string is a resolvable toolchain, or a path based toolchain.
+    fn validate(candidate: &str) -> Result<Self, Error> {
         let candidate = validate(candidate)?;
-        ToolchainName::try_from(candidate)
-            .map(LocalToolchainName::Named)
+        ResolvableToolchainName::try_from(candidate)
+            .map(ResolvableLocalToolchainName::Named)
             .or_else(|_| {
                 PathBasedToolchain::try_from(&PathBuf::from(candidate) as &Path)
-                    .map(LocalToolchainName::Path)
+                    .map(ResolvableLocalToolchainName::Path)
             })
     }
 }
 
+try_from_str!(ResolvableLocalToolchainName);
+
+impl Display for ResolvableLocalToolchainName {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            ResolvableLocalToolchainName::Named(t) => write!(f, "{t}"),
+            ResolvableLocalToolchainName::Path(t) => write!(f, "{t}"),
+        }
+    }
+}
+
+/// LocalToolchainName can be used in calls to Cfg that alter configuration,
+/// like setting overrides, or that depend on configuration, like calculating
+/// the toolchain directory. It is not used to model the RUSTUP_TOOLCHAIN
+/// variable, because that can take unresolved toolchain values that are not
+/// invalid for referring to an installed toolchain.
+#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
+pub(crate) enum LocalToolchainName {
+    Named(ToolchainName),
+    Path(PathBasedToolchain),
+}
+
 impl From<&ToolchainDesc> for LocalToolchainName {
     fn from(value: &ToolchainDesc) -> Self {
         ToolchainName::Official(value.to_owned()).into()
@@ -385,7 +420,6 @@ from_variant!(
     LocalToolchainName,
     LocalToolchainName::Path
 );
-try_from_str!(LocalToolchainName);
 
 impl Display for LocalToolchainName {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
diff --git a/tests/suite/cli_rustup.rs b/tests/suite/cli_rustup.rs
@@ -1069,14 +1069,10 @@ fn show_toolchain_env() {
     test(&|config| {
         config.with_scenario(Scenario::SimpleV2, &|config| {
             config.expect_ok(&["rustup", "default", "nightly"]);
-            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 out = config.run("rustup", ["show"], &[("RUSTUP_TOOLCHAIN", "nightly")]);
+            assert!(out.ok);
             assert_eq!(
-                &stdout,
+                &out.stdout,
                 for_host_and_home!(
                     config,
                     r"Default host: {0}
@@ -2017,6 +2013,7 @@ fn valid_override_settings() {
             config.expect_ok(&["rustup", "default", "nightly"]);
             raw::write_file(&toolchain_file, "nightly").unwrap();
             config.expect_ok(&["rustc", "--version"]);
+            // Special case: same version as is installed is permitted.
             raw::write_file(&toolchain_file, for_host!("nightly-{}")).unwrap();
             config.expect_ok(&["rustc", "--version"]);
             let fullpath = config
@@ -2042,16 +2039,14 @@ fn file_override_with_target_info() {
     // Target info is not portable between machines, so we reject toolchain
     // files that include it.
     test(&|config| {
-        config.with_scenario(Scenario::SimpleV2, &|config| {
-            let cwd = config.current_dir();
-            let toolchain_file = cwd.join("rust-toolchain");
-            raw::write_file(&toolchain_file, "nightly-x86_64-unknown-linux-gnu").unwrap();
+        let cwd = config.current_dir();
+        let toolchain_file = cwd.join("rust-toolchain");
+        raw::write_file(&toolchain_file, "nightly-x86_64-unknown-linux-gnu").unwrap();
 
-            config.expect_err(
-                &["rustc", "--version"],
-                "target triple in channel name 'nightly-x86_64-unknown-linux-gnu'",
-            );
-        })
+        config.expect_err(
+            &["rustc", "--version"],
+            "target triple in channel name 'nightly-x86_64-unknown-linux-gnu'",
+        );
     });
 }
 
diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs
@@ -8,6 +8,10 @@ use std::process::Command;
 
 use remove_dir_all::remove_dir_all;
 
+use retry::{
+    delay::{jitter, Fibonacci},
+    retry, OperationResult,
+};
 use rustup::test::{
     mock::{
         clitools::{self, output_release_file, self_update_setup, Config, Scenario},
@@ -255,21 +259,30 @@ fn uninstall_self_delete_works() {
 // file in CONFIG.CARGODIR/.. ; check that it doesn't exist.
 #[test]
 fn uninstall_doesnt_leave_gc_file() {
-    use std::thread;
-    use std::time::Duration;
-
     setup_empty_installed(&|config| {
         config.expect_ok(&["rustup", "self", "uninstall", "-y"]);
-
-        // The gc removal happens after rustup terminates. Give it a moment.
-        thread::sleep(Duration::from_millis(100));
-
         let parent = config.cargodir.parent().unwrap();
-        // Actually, there just shouldn't be any files here
-        for dirent in fs::read_dir(parent).unwrap() {
-            let dirent = dirent.unwrap();
-            println!("residual garbage '{}'", dirent.path().display());
-            panic!();
+
+        // The gc removal happens after rustup terminates. Typically under
+        // 100ms, but during the contention of test suites can be substantially
+        // longer while still succeeding.
+
+        #[derive(thiserror::Error, Debug)]
+        #[error("{:?}", .0)]
+        struct GcErr(Vec<String>);
+
+        match retry(Fibonacci::from_millis(1).map(jitter).take(10), || {
+            let garbage = fs::read_dir(parent)
+                .unwrap()
+                .map(|d| d.unwrap().path().to_string_lossy().to_string())
+                .collect::<Vec<_>>();
+            match garbage.len() {
+                0 => OperationResult::Ok(()),
+                _ => OperationResult::Err(GcErr(garbage)),
+            }
+        }) {
+            Ok(_) => (),
+            Err(e) => panic!("{e}"),
         }
     })
 }
