From af1f0314cb9fa06d7886da652a03479e7b7ec7aa Mon Sep 17 00:00:00 2001 From: Robert Collins Date: Thu, 5 Mar 2020 10:34:53 +1300 Subject: [PATCH] Auto-install toolchains more consistently This combines all auto-installation logic to the root of the codepaths leading to running a binary, which should lead to auto-installation happening for all cases, as long as a toolchain has been selected. Config.find_default no longer verifies the toolchain in order to permit this auto-installation to take place in one location; the callers of find_default do not generally need verified toolchains, so this should be fine. --- src/cli/rustup_mode.rs | 4 +-- src/config.rs | 60 ++++++++++++++++++++++-------------------- src/errors.rs | 4 +++ 3 files changed, 38 insertions(+), 30 deletions(-) diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index a0f9d3e7e29..0c820d2d5ae 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -928,7 +928,7 @@ fn show(cfg: &Cfg) -> Result<()> { let cwd = utils::current_dir()?; let installed_toolchains = cfg.list_toolchains()?; - let active_toolchain = cfg.find_override_toolchain_or_default(&cwd); + let active_toolchain = cfg.find_or_install_override_toolchain_or_default(&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 { @@ -1053,7 +1053,7 @@ fn show(cfg: &Cfg) -> Result<()> { fn show_active_toolchain(cfg: &Cfg) -> Result<()> { let cwd = utils::current_dir()?; - if let Some((toolchain, reason)) = cfg.find_override_toolchain_or_default(&cwd)? { + if let Some((toolchain, reason)) = cfg.find_or_install_override_toolchain_or_default(&cwd)? { if let Some(reason) = reason { println!("{} ({})", toolchain.name(), reason); } else { diff --git a/src/config.rs b/src/config.rs index d99822a1602..d8adf3f2dd2 100644 --- a/src/config.rs +++ b/src/config.rs @@ -309,7 +309,7 @@ impl Cfg { } pub fn which_binary(&self, path: &Path, binary: &str) -> Result> { - if let Some((toolchain, _)) = self.find_override_toolchain_or_default(path)? { + if let Some((toolchain, _)) = self.find_or_install_override_toolchain_or_default(path)? { Ok(Some(toolchain.binary_file(binary))) } else { Ok(None) @@ -370,10 +370,7 @@ impl Cfg { .with(|s| Ok(s.default_toolchain.clone()))?; if let Some(name) = opt_name { - let toolchain = self - .verify_toolchain(&name) - .chain_err(|| ErrorKind::ToolchainNotInstalled(name.to_string()))?; - + let toolchain = Toolchain::from(self, &name)?; Ok(Some(toolchain)) } else { Ok(None) @@ -427,24 +424,18 @@ impl Cfg { ), }; - match self.get_toolchain(&name, false) { - Ok(toolchain) => { - if toolchain.exists() { - Ok(Some((toolchain, reason))) - } else if toolchain.is_custom() { - // Strip the confusing NotADirectory error and only mention that the - // override toolchain is not installed. - Err(Error::from(reason_err)).chain_err(|| { - ErrorKind::OverrideToolchainNotInstalled(name.to_string()) - }) - } else { - toolchain.install_from_dist(true, false, &[], &[])?; - Ok(Some((toolchain, reason))) - } - } - Err(e) => Err(e) - .chain_err(|| Error::from(reason_err)) - .chain_err(|| ErrorKind::OverrideToolchainNotInstalled(name.to_string())), + let toolchain = Toolchain::from(self, &name)?; + // 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). + if !toolchain.exists() && toolchain.is_custom() { + // Strip the confusing NotADirectory error and only mention that the + // override toolchain is not installed. + Err(Error::from(reason_err)) + .chain_err(|| ErrorKind::OverrideToolchainNotInstalled(name.to_string())) + } else { + Ok(Some((toolchain, reason))) } } else { Ok(None) @@ -495,17 +486,30 @@ impl Cfg { Ok(None) } - pub fn find_override_toolchain_or_default( + pub fn find_or_install_override_toolchain_or_default( &self, path: &Path, ) -> Result, Option)>> { - Ok( + if let Some((toolchain, reason)) = if let Some((toolchain, reason)) = self.find_override(path)? { Some((toolchain, Some(reason))) } else { self.find_default()?.map(|toolchain| (toolchain, None)) - }, - ) + } + { + if !toolchain.exists() { + if toolchain.is_custom() { + return Err( + ErrorKind::ToolchainNotInstalled(toolchain.name().to_string()).into(), + ); + } + toolchain.install_from_dist(true, false, &[], &[])?; + } + Ok(Some((toolchain, reason))) + } else { + // No override and no default set + Err(ErrorKind::ToolchainNotSelected.into()) + } } pub fn get_default(&self) -> Result> { @@ -581,7 +585,7 @@ impl Cfg { &self, path: &Path, ) -> Result<(Toolchain<'_>, Option)> { - self.find_override_toolchain_or_default(path) + self.find_or_install_override_toolchain_or_default(path) .and_then(|r| r.ok_or_else(|| "no default toolchain configured".into())) } diff --git a/src/errors.rs b/src/errors.rs index 13c4dc4c816..5e497ac2924 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -300,6 +300,10 @@ error_chain! { description("toolchain is not installed") display("toolchain '{}' is not installed", t) } + ToolchainNotSelected { + description("toolchain is not selected") + display("no override and no default toolchain set") + } OverrideToolchainNotInstalled(t: String) { description("override toolchain is not installed") display("override toolchain '{}' is not installed", t)