diff --git a/src/cli/common.rs b/src/cli/common.rs index 887ae211f9..4044dc4169 100644 --- a/src/cli/common.rs +++ b/src/cli/common.rs @@ -321,7 +321,7 @@ pub(crate) async fn list_toolchains( verbose: bool, quiet: bool, ) -> anyhow::Result { - let toolchains = cfg.list_toolchains()?; + let mut toolchains = cfg.list_toolchains(quiet)?; if toolchains.is_empty() { writeln!(cfg.process.stdout().lock(), "no installed toolchains")?; } else { @@ -335,6 +335,7 @@ pub(crate) async fn list_toolchains( None }; + toolchains.sort(); for toolchain in toolchains { let is_default_toolchain = default_toolchain_name.as_ref() == Some(&toolchain); let is_active_toolchain = active_toolchain_name.as_ref() == Some(&toolchain); diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index 9b2a5d9c04..06f14cd038 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -890,7 +890,7 @@ pub async fn main( } }?; - if should_warn && cfg.list_toolchains()?.is_empty() && cfg.get_default()?.is_none() { + if should_warn && cfg.list_toolchains(true)?.is_empty() && cfg.get_default()?.is_none() { warn!("no toolchain installed and no default toolchain set\n{DEFAULT_STABLE_HINT}"); } @@ -902,7 +902,8 @@ pub async fn main( } fn completion_command(cfg: &Cfg<'_>) -> clap::Command { - let toolchains = cfg.list_toolchains().unwrap_or_default(); + let mut toolchains = cfg.list_toolchains(true).unwrap_or_default(); + toolchains.sort(); Rustup::command().mut_arg("+toolchain", move |arg| { arg.add(ArgValueCompleter::new(move |current: &OsStr| { let Some(prefix) = current.to_str() else { @@ -1280,7 +1281,8 @@ async fn show(cfg: &Cfg<'_>, verbose: bool) -> anyhow::Result { writeln!(t)?; } - let installed_toolchains = cfg.list_toolchains()?; + let mut installed_toolchains = cfg.list_toolchains(cfg.quiet)?; + installed_toolchains.sort(); let active_toolchain_and_source: Option<(ToolchainName, ActiveSource)> = if let Ok(Some((LocalToolchainName::Named(toolchain_name), source))) = cfg.maybe_ensure_active_toolchain(None).await diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 95a5d0f00c..2f60fad5b4 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -972,7 +972,7 @@ pub(crate) fn uninstall( } info!("removing toolchains"); - for toolchain in cfg.list_toolchains()? { + for toolchain in cfg.list_toolchains(true)? { Toolchain::ensure_removed(cfg, toolchain.into())?; } diff --git a/src/config.rs b/src/config.rs index 3f617b2788..20ab15e72c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,6 +1,5 @@ use std::{ fmt::{self, Debug, Display}, - io, io::Write, ops::Deref, path::{Path, PathBuf}, @@ -755,7 +754,11 @@ impl<'a> Cfg<'a> { // 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::from_str(toolchain_name_str)?; - if !self.list_toolchains()?.iter().any(|s| s == resolved_name) { + if !self + .list_toolchains(true)? + .iter() + .any(|s| s == resolved_name) + { return Err(anyhow!(format!("target tuple in channel name '{name}'"))); } } @@ -976,51 +979,75 @@ impl<'a> Cfg<'a> { Ok(Some(ResolvableToolchainName::from_str(toolchain)?)) } - /// List all the installed toolchains: that is paths in the toolchain dir - /// that are: - /// - not files - /// - named with a valid resolved toolchain name - /// Currently no notification of incorrect names or entry type is done. + /// Lists all the installed toolchains. + /// + /// # Note + /// + /// This function returns every valid toolchain name that has a corresponding non-file entry in + /// the toolchains directory. These names may be returned in any order. #[tracing::instrument(level = "trace", skip_all)] - pub(crate) fn list_toolchains(&self) -> anyhow::Result> { - if utils::is_directory(&self.toolchains_dir) { - 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| ToolchainName::from_str(&n).ok()) - .collect(); - - toolchains.sort(); - - Ok(toolchains) - } else { - Ok(Vec::new()) + pub(crate) fn list_toolchains(&self, quiet: bool) -> anyhow::Result> { + if !utils::is_directory(&self.toolchains_dir) { + return Ok(vec![]); } + + let mut toolchains = vec![]; + for entry in utils::read_dir("toolchains", &self.toolchains_dir)? { + let entry = match entry { + Ok(entry) => entry, + Err(e) => { + if !quiet { + warn!("failed to read toolchain FS entry: {e}"); + } + continue; + } + }; + if !entry.file_type().is_ok_and(|t| !t.is_file()) { + continue; + } + let tc = match entry.file_name().into_string() { + Ok(tc) => tc, + Err(e) => { + if !quiet { + warn!( + "ignoring invalid potential toolchain name `{}`", + e.display() + ); + } + continue; + } + }; + match ToolchainName::from_str(&tc) { + Ok(tc) => toolchains.push(tc), + Err(e) => { + if !quiet { + warn!("ignoring invalid toolchain: {e}") + } + } + } + } + + Ok(toolchains) } pub(crate) fn list_channels( &self, ) -> anyhow::Result)>> { - self.list_toolchains()? + let mut channels = self + .list_toolchains(true)? .into_iter() - .filter_map(|t| { - if let ToolchainName::Official(desc) = t { - Some(desc) - } else { - None + .filter_map(|t| match t { + ToolchainName::Official(n) if n.is_tracking() => { + Some(DistributableToolchain::new(self, n.clone()).map(|t| (n, t))) } + _ => None, }) - .filter(ToolchainDesc::is_tracking) - .map(|n| { - DistributableToolchain::new(self, n.clone()) - .map_err(Into::into) - .map(|t| (n.clone(), t)) - }) - .collect::>>() + .collect::, _>>()?; + + // HACK: `.sort_by_key()` is impossible here without cloning. + // See: + channels.sort_by(|(n, _), (m, _)| n.cmp(m)); + Ok(channels) } /// Create an override for a toolchain diff --git a/tests/suite/cli_v2.rs b/tests/suite/cli_v2.rs index 4be6d1438c..a55e4335f4 100644 --- a/tests/suite/cli_v2.rs +++ b/tests/suite/cli_v2.rs @@ -285,6 +285,30 @@ nightly-[HOST_TUPLE] (active, default) .is_ok(); } +#[tokio::test] +async fn list_toolchains_with_illegal_names() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + cx.config + .expect(["rustup", "update", "nightly"]) + .await + .is_ok(); + + fs::create_dir(cx.config.rustupdir.join("toolchains/--illegal-name")).unwrap(); + + cx.config + .expect(["rustup", "toolchain", "list"]) + .await + .with_stdout(snapbox::str![[r#" +nightly-[HOST_TUPLE] (active, default) + +"#]]) + .with_stderr(snapbox::str![[r#" +warn: ignoring invalid toolchain: invalid toolchain name '--illegal-name'; valid toolchain names do not start with '-' + +"#]]) + .is_ok(); +} + #[tokio::test] async fn list_toolchains_with_none() { let cx = CliTestContext::new(Scenario::SimpleV2).await;