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.

Author: rbtcollins

src/cli/rustup_mode.rs

@@ -928,22 +928,19 @@ 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);
+    // XXX: we may want a find_without_install capability for show.
+    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 {
-        if let Some((ref t, _)) = *at {
-            match t.list_components() {
-                Ok(None) => vec![],
-                Ok(Some(cs_vec)) => cs_vec
-                    .into_iter()
-                    .filter(|c| c.component.short_name_in_manifest() == "rust-std")
-                    .filter(|c| c.installed)
-                    .collect(),
-                Err(_) => vec![],
-            }
-        } else {
-            vec![]
+        match at.0.list_components() {
+            Ok(None) => vec![],
+            Ok(Some(cs_vec)) => cs_vec
+                .into_iter()
+                .filter(|c| c.component.short_name_in_manifest() == "rust-std")
+                .filter(|c| c.installed)
+                .collect(),
+            Err(_) => vec![],
         }
     } else {
         vec![]
@@ -1013,18 +1010,18 @@ fn show(cfg: &Cfg) -> Result<()> {
 
         match active_toolchain {
             Ok(atc) => match atc {
-                Some((ref toolchain, Some(ref reason))) => {
+                (ref toolchain, Some(ref reason)) => {
                     writeln!(t, "{} ({})", toolchain.name(), reason)?;
                     writeln!(t, "{}", toolchain.rustc_version())?;
                 }
-                Some((ref toolchain, None)) => {
+                (ref toolchain, None) => {
                     writeln!(t, "{} (default)", toolchain.name())?;
                     writeln!(t, "{}", toolchain.rustc_version())?;
                 }
-                None => {
-                    writeln!(t, "no active toolchain")?;
-                }
             },
+            Err(rustup::Error(rustup::ErrorKind::ToolchainNotSelected, _)) => {
+                writeln!(t, "no active toolchain")?;
+            }
             Err(err) => {
                 if let Some(cause) = err.source() {
                     writeln!(t, "(error: {}, {})", err, cause)?;
@@ -1053,11 +1050,15 @@ 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(reason) = reason {
-            println!("{} ({})", toolchain.name(), reason);
-        } else {
-            println!("{} (default)", toolchain.name());
+    match cfg.find_or_install_override_toolchain_or_default(&cwd) {
+        Err(rustup::Error(rustup::ErrorKind::ToolchainNotSelected, _)) => {}
+        Err(e) => return Err(e.into()),
+        Ok((toolchain, reason)) => {
+            if let Some(reason) = reason {
+                println!("{} ({})", toolchain.name(), reason);
+            } else {
+                println!("{} (default)", toolchain.name());
+            }
         }
     }
     Ok(())

src/config.rs

@@ -309,11 +309,8 @@ impl Cfg {
     }
 
     pub fn which_binary(&self, path: &Path, binary: &str) -> Result<Option<PathBuf>> {
-        if let Some((toolchain, _)) = self.find_override_toolchain_or_default(path)? {
-            Ok(Some(toolchain.binary_file(binary)))
-        } else {
-            Ok(None)
-        }
+        let (toolchain, _) = self.find_or_install_override_toolchain_or_default(path)?;
+        Ok(Some(toolchain.binary_file(binary)))
     }
 
     pub fn upgrade_data(&self) -> Result<()> {
@@ -370,10 +367,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 +421,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 +483,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<(Toolchain<'_>, Option<OverrideReason>)>> {
-        Ok(
+    ) -> Result<(Toolchain<'_>, Option<OverrideReason>)> {
+        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((toolchain, reason))
+        } else {
+            // No override and no default set
+            Err(ErrorKind::ToolchainNotSelected.into())
+        }
     }
 
     pub fn get_default(&self) -> Result<Option<String>> {
@@ -581,8 +582,7 @@ impl Cfg {
         &self,
         path: &Path,
     ) -> Result<(Toolchain<'_>, Option<OverrideReason>)> {
-        self.find_override_toolchain_or_default(path)
-            .and_then(|r| r.ok_or_else(|| "no default toolchain configured".into()))
+        self.find_or_install_override_toolchain_or_default(path)
     }
 
     pub fn create_command_for_dir(&self, path: &Path, binary: &str) -> Result<Command> {

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)

tests/cli-v1.rs

@@ -22,7 +22,11 @@ pub fn setup(f: &dyn Fn(&mut Config)) {
 #[test]
 fn rustc_no_default_toolchain() {
     setup(&|config| {
-        expect_err(config, &["rustc"], "no default toolchain configured");
+        expect_err(
+            config,
+            &["rustc"],
+            "no override and no default toolchain set",
+        );
     });
 }
 
@@ -158,14 +162,14 @@ fn remove_toolchain() {
 }
 
 #[test]
-fn remove_default_toolchain_err_handling() {
+fn remove_default_toolchain_autoinstalls() {
     setup(&|config| {
         expect_ok(config, &["rustup", "default", "nightly"]);
         expect_ok(config, &["rustup", "toolchain", "remove", "nightly"]);
-        expect_err(
+        expect_stderr_ok(
             config,
-            &["rustc"],
-            for_host!("toolchain 'nightly-{0}' is not installed"),
+            &["rustc", "--version"],
+            "info: installing component",
         );
     });
 }
@@ -304,7 +308,11 @@ fn remove_override_no_default() {
         config.change_dir(tempdir.path(), &|| {
             expect_ok(config, &["rustup", "override", "add", "nightly"]);
             expect_ok(config, &["rustup", "override", "remove"]);
-            expect_err(config, &["rustc"], "no default toolchain configured");
+            expect_err(
+                config,
+                &["rustc"],
+                "no override and no default toolchain set",
+            );
         });
     });
 }

tests/cli-v2.rs

@@ -30,7 +30,11 @@ pub fn setup_complex(f: &dyn Fn(&mut Config)) {
 #[test]
 fn rustc_no_default_toolchain() {
     setup(&|config| {
-        expect_err(config, &["rustc"], "no default toolchain configured");
+        expect_err(
+            config,
+            &["rustc"],
+            "no override and no default toolchain set",
+        );
     });
 }
 
@@ -245,14 +249,14 @@ fn add_remove_multiple_toolchains() {
 }
 
 #[test]
-fn remove_default_toolchain_err_handling() {
+fn remove_default_toolchain_autoinstalls() {
     setup(&|config| {
         expect_ok(config, &["rustup", "default", "nightly"]);
         expect_ok(config, &["rustup", "toolchain", "remove", "nightly"]);
-        expect_err(
+        expect_stderr_ok(
             config,
-            &["rustc"],
-            for_host!("toolchain 'nightly-{0}' is not installed"),
+            &["rustc", "--version"],
+            "info: installing component",
         );
     });
 }
@@ -446,7 +450,11 @@ fn remove_override_no_default() {
         config.change_dir(tempdir.path(), &|| {
             expect_ok(config, &["rustup", "override", "add", "nightly"]);
             expect_ok(config, &["rustup", "override", "remove"]);
-            expect_err(config, &["rustc"], "no default toolchain configured");
+            expect_err(
+                config,
+                &["rustc"],
+                "no override and no default toolchain set",
+            );
         });
     });
 }