Merge pull request #1578 from kinnison/kinnison/fix-745

Deal cleanly with malformed default-host

Author: nrc

src/rustup-cli/log.rs

@@ -16,6 +16,10 @@ macro_rules! verbose {
     ( $ ( $ arg : tt ) * ) => ( $crate::log::verbose_fmt ( format_args ! ( $ ( $ arg ) * ) ) )
 }
 
+macro_rules! debug {
+    ( $ ( $ arg : tt ) * ) => ( $crate::log::debug_fmt ( format_args ! ( $ ( $ arg ) * ) ) )
+}
+
 pub fn warn_fmt(args: fmt::Arguments) {
     let mut t = term2::stderr();
     let _ = t.fg(term2::color::BRIGHT_YELLOW);
@@ -54,3 +58,15 @@ pub fn verbose_fmt(args: fmt::Arguments) {
     let _ = t.write_fmt(args);
     let _ = write!(t, "\n");
 }
+
+pub fn debug_fmt(args: fmt::Arguments) {
+    if std::env::var("RUSTUP_DEBUG").is_ok() {
+        let mut t = term2::stderr();
+        let _ = t.fg(term2::color::BRIGHT_BLUE);
+        let _ = t.attr(term2::Attr::Bold);
+        let _ = write!(t, "verbose: ");
+        let _ = t.reset();
+        let _ = t.write_fmt(args);
+        let _ = write!(t, "\n");
+    }
+}

src/rustup-cli/self_update.rs

@@ -223,6 +223,7 @@ fn canonical_cargo_home() -> Result<String> {
 /// and adding `CARGO_HOME`/bin to PATH.
 pub fn install(no_prompt: bool, verbose: bool, mut opts: InstallOpts) -> Result<()> {
     do_pre_install_sanity_checks()?;
+    do_pre_install_options_sanity_checks(&opts)?;
     check_existence_of_rustc_or_cargo_in_path(no_prompt)?;
     do_anti_sudo_check(no_prompt)?;
 
@@ -454,7 +455,7 @@ fn do_pre_install_sanity_checks() -> Result<()> {
             "delete `{}` to remove rustup.sh",
             rustup_sh_path.expect("").display()
         );
-        warn!("or, if you already rustup installed, you can run");
+        warn!("or, if you already have rustup installed, you can run");
         warn!("`rustup self update` and `rustup toolchain list` to upgrade");
         warn!("your directory structure");
         return Err("cannot install while rustup.sh is installed".into());
@@ -463,6 +464,35 @@ fn do_pre_install_sanity_checks() -> Result<()> {
     Ok(())
 }
 
+fn do_pre_install_options_sanity_checks(opts: &InstallOpts) -> Result<()> {
+    // Verify that the installation options are vaguely sane
+    (|| {
+        let host_triple = dist::TargetTriple::from_str(&opts.default_host_triple);
+        let toolchain_to_use = if opts.default_toolchain == "none" {
+            "stable"
+        } else {
+            &opts.default_toolchain
+        };
+        let partial_channel = dist::PartialToolchainDesc::from_str(toolchain_to_use)?;
+        let resolved = partial_channel.resolve(&host_triple)?.to_string();
+        debug!(
+            "Successfully resolved installation toolchain as: {}",
+            resolved
+        );
+        Ok(())
+    })()
+    .map_err(|e: Box<std::error::Error>| {
+        format!(
+            "Pre-checks for host and toolchain failed: {}\n\
+             If you are unsure of suitable values, the 'stable' toolchain is the default.\n\
+             Valid host triples look something like: {}",
+            e,
+            dist::TargetTriple::from_host_or_build()
+        )
+    })?;
+    Ok(())
+}
+
 // If the user is trying to install with sudo, on some systems this will
 // result in writing root-owned files to the user's home directory, because
 // sudo is configured not to change $HOME. Don't let that bogosity happen.

src/rustup-dist/src/dist.rs

@@ -291,11 +291,25 @@ impl PartialToolchainDesc {
         }
     }
 
-    pub fn resolve(self, host: &TargetTriple) -> ToolchainDesc {
-        let host = PartialTargetTriple::from_str(&host.0)
-            .expect("host triple couldn't be converted to partial triple");
-        let host_arch = host.arch.expect("");
-        let host_os = host.os.expect("");
+    pub fn resolve(self, input_host: &TargetTriple) -> Result<ToolchainDesc> {
+        let host = PartialTargetTriple::from_str(&input_host.0).ok_or_else(|| {
+            format!(
+                "Provided host '{}' couldn't be converted to partial triple",
+                input_host.0
+            )
+        })?;
+        let host_arch = host.arch.ok_or_else(|| {
+            format!(
+                "Provided host '{}' did not specify a CPU architecture",
+                input_host.0
+            )
+        })?;
+        let host_os = host.os.ok_or_else(|| {
+            format!(
+                "Provided host '{}' did not specify an operating system",
+                input_host.0
+            )
+        })?;
         let host_env = host.env;
 
         // If OS was specified, don't default to host environment, even if the OS matches
@@ -314,11 +328,11 @@ impl PartialToolchainDesc {
             format!("{}-{}", arch, os)
         };
 
-        ToolchainDesc {
+        Ok(ToolchainDesc {
             channel: self.channel,
             date: self.date,
             target: TargetTriple(trip),
-        }
+        })
     }
 
     pub fn has_triple(&self) -> bool {

src/rustup/config.rs

@@ -99,7 +99,7 @@ impl Cfg {
         );
         let dist_root = dist_root_server.clone() + "/dist";
 
-        Ok(Cfg {
+        let cfg = Cfg {
             rustup_dir: rustup_dir,
             settings_file: settings_file,
             toolchains_dir: toolchains_dir,
@@ -111,7 +111,15 @@ impl Cfg {
             env_override: env_override,
             dist_root_url: dist_root,
             dist_root_server: dist_root_server,
-        })
+        };
+
+        // Run some basic checks against the constructed configuration
+        // For now, that means simply checking that 'stable' can resolve
+        // for the current configuration.
+        cfg.resolve_toolchain("stable")
+            .map_err(|e| format!("Unable parse configuration: {}", e))?;
+
+        Ok(cfg)
     }
 
     pub fn set_default(&self, toolchain: &str) -> Result<()> {
@@ -470,9 +478,11 @@ impl Cfg {
     }
 
     pub fn set_default_host_triple(&self, host_triple: &str) -> Result<()> {
-        if dist::PartialTargetTriple::from_str(host_triple).is_none() {
-            return Err("Invalid host triple".into());
-        }
+        // Ensure that the provided host_triple is capable of resolving
+        // against the 'stable' toolchain.  This provides early errors
+        // if the supplied triple is insufficient / bad.
+        dist::PartialToolchainDesc::from_str("stable")?
+            .resolve(&dist::TargetTriple::from_str(host_triple))?;
         self.settings_file.with_mut(|s| {
             s.default_host_triple = Some(host_triple.to_owned());
             Ok(())
@@ -493,7 +503,7 @@ impl Cfg {
     pub fn resolve_toolchain(&self, name: &str) -> Result<String> {
         if let Ok(desc) = dist::PartialToolchainDesc::from_str(name) {
             let host = self.get_default_host_triple()?;
-            Ok(desc.resolve(&host).to_string())
+            Ok(desc.resolve(&host)?.to_string())
         } else {
             Ok(name.to_owned())
         }

tests/cli-rustup.rs

@@ -900,7 +900,19 @@ fn set_default_host_invalid_triple() {
         expect_err(
             config,
             &["rustup", "set", "default-host", "foo"],
-            "Invalid host triple",
+            "error: Provided host 'foo' couldn't be converted to partial triple",
+        );
+    });
+}
+
+// #745
+#[test]
+fn set_default_host_invalid_triple_valid_partial() {
+    setup(&|config| {
+        expect_err(
+            config,
+            &["rustup", "set", "default-host", "x86_64-msvc"],
+            "error: Provided host 'x86_64-msvc' did not specify an operating system",
         );
     });
 }