auto merge of #10936 : cadencemarseille/rust/issue-10754-std-run-unwrap-on-None, r=alexcrichton

The problem was that std::run::Process::new() was unwrap()ing the result
of std::io::process::Process::new(), which returns None in the case
where the io_error condition is raised to signal failure to start the
process.

Have std::run::Process::new() similarly return an Option\<run::Process\>
to reflect the fact that a subprocess might have failed to start. Update
utility functions run::process_status() and run::process_output() to
return Option\<ProcessExit\> and Option\<ProcessOutput\>, respectively.

Various parts of librustc and librustpkg needed to be updated to reflect
these API changes.

closes #10754

5 files changed, 112 insertions, 67 deletions

diff --git a/src/librustpkg/api.rs b/src/librustpkg/api.rs
index 0d0d0b7c4c7..8e4216562c0 100644
--- a/src/librustpkg/api.rs
+++ b/src/librustpkg/api.rs
@@ -142,14 +142,14 @@ pub fn install_pkg(cx: &BuildContext,
 /// Builds an arbitrary library whose short name is `output`,
 /// by invoking `tool` with arguments `args` plus "-o %s", where %s
 /// is the platform-specific library name for `output`.
-/// Returns that platform-specific name.
+/// Returns that platform-specific name, or None if `tool` could not be started.
 pub fn build_library_in_workspace(exec: &mut workcache::Exec,
                                   context: &mut Context,
                                   package_name: &str,
                                   tool: &str,
                                   flags: &[~str],
                                   paths: &[~str],
-                                  output: &str) -> ~str {
+                                  output: &str) -> Option<~str> {
     use command_failed = conditions::command_failed::cond;
 
     let workspace = my_workspace(context, package_name);
@@ -169,16 +169,20 @@ pub fn build_library_in_workspace(exec: &mut workcache::Exec,
 
     let all_args = flags + absolute_paths + cc_args +
          ~[~"-o", out_name.as_str().unwrap().to_owned()];
-    let exit_process = run::process_status(tool, all_args);
-    if exit_process.success() {
-        let out_name_str = out_name.as_str().unwrap().to_owned();
-        exec.discover_output("binary",
-                             out_name_str,
-                             digest_only_date(&out_name));
-        context.add_library_path(out_name.dir_path());
-        out_name_str
-    } else {
-        command_failed.raise((tool.to_owned(), all_args, exit_process))
+    match run::process_status(tool, all_args) {
+        Some(exit_process) => {
+            if exit_process.success() {
+                let out_name_str = out_name.as_str().unwrap().to_owned();
+                exec.discover_output("binary",
+                                     out_name_str,
+                                     digest_only_date(&out_name));
+                context.add_library_path(out_name.dir_path());
+                Some(out_name_str)
+            } else {
+                Some(command_failed.raise((tool.to_owned(), all_args, exit_process)))
+            }
+        },
+        None => None
     }
 }
 
diff --git a/src/librustpkg/lib.rs b/src/librustpkg/lib.rs
index cedb21eeb2a..76d2db0a587 100644
--- a/src/librustpkg/lib.rs
+++ b/src/librustpkg/lib.rs
@@ -166,29 +166,47 @@ impl<'a> PkgScript<'a> {
     /// Run the contents of this package script, where <what>
     /// is the command to pass to it (e.g., "build", "clean", "install")
     /// Returns a pair of an exit code and list of configs (obtained by
-    /// calling the package script's configs() function if it exists
-    fn run_custom(exe: &Path, sysroot: &Path) -> (~[~str], process::ProcessExit) {
+    /// calling the package script's configs() function if it exists, or
+    /// None if `exe` could not be started.
+    fn run_custom(exe: &Path, sysroot: &Path) -> Option<(~[~str], process::ProcessExit)> {
         debug!("Running program: {} {} {}", exe.as_str().unwrap().to_owned(),
                sysroot.display(), "install");
         // FIXME #7401 should support commands besides `install`
         // FIXME (#9639): This needs to handle non-utf8 paths
-        let status = run::process_status(exe.as_str().unwrap(),
-                                         [sysroot.as_str().unwrap().to_owned(), ~"install"]);
-        if !status.success() {
-            debug!("run_custom: first pkg command failed with {:?}", status);
-            (~[], status)
-        }
-        else {
-            debug!("Running program (configs): {} {} {}",
-                   exe.display(), sysroot.display(), "configs");
-            // FIXME (#9639): This needs to handle non-utf8 paths
-            let output = run::process_output(exe.as_str().unwrap(),
-                                             [sysroot.as_str().unwrap().to_owned(), ~"configs"]);
-            debug!("run_custom: second pkg command did {:?}", output.status);
-            // Run the configs() function to get the configs
-            let cfgs = str::from_utf8(output.output).words()
-                .map(|w| w.to_owned()).collect();
-            (cfgs, output.status)
+        let opt_status = run::process_status(exe.as_str().unwrap(),
+                                             [sysroot.as_str().unwrap().to_owned(), ~"install"]);
+        match opt_status {
+            Some(status) => {
+                if !status.success() {
+                    debug!("run_custom: first pkg command failed with {:?}", status);
+                    Some((~[], status))
+                }
+                else {
+                    debug!("Running program (configs): {} {} {}",
+                           exe.display(), sysroot.display(), "configs");
+                    // FIXME (#9639): This needs to handle non-utf8 paths
+                    let opt_output = run::process_output(exe.as_str().unwrap(),
+                                                         [sysroot.as_str().unwrap().to_owned(),
+                                                          ~"configs"]);
+                    match opt_output {
+                        Some(output) => {
+                            debug!("run_custom: second pkg command did {:?}", output.status);
+                            // Run the configs() function to get the configs
+                            let cfgs = str::from_utf8(output.output).words()
+                                .map(|w| w.to_owned()).collect();
+                            Some((cfgs, output.status))
+                        },
+                        None => {
+                            debug!("run_custom: second pkg command failed to start");
+                            Some((~[], status))
+                        }
+                    }
+                }
+            },
+            None => {
+                debug!("run_custom: first pkg command failed to start");
+                None
+            }
         }
     }
 }
@@ -481,14 +499,20 @@ impl CtxMethods for BuildContext {
                     })
                 });
                 // We always *run* the package script
-                let (cfgs, hook_result) = PkgScript::run_custom(&Path::new(pkg_exe), &sysroot);
-                debug!("Command return code = {:?}", hook_result);
-                if !hook_result.success() {
-                    fail!("Error running custom build command")
+                match PkgScript::run_custom(&Path::new(pkg_exe), &sysroot) {
+                    Some((cfgs, hook_result)) => {
+                        debug!("Command return code = {:?}", hook_result);
+                        if !hook_result.success() {
+                            fail!("Error running custom build command")
+                        }
+                        custom = true;
+                        // otherwise, the package script succeeded
+                        cfgs
+                    },
+                    None => {
+                        fail!("Error starting custom build command")
+                    }
                 }
-                custom = true;
-                // otherwise, the package script succeeded
-                cfgs
             }
             (Some(_), Inferred) => {
                 debug!("There is a package script, but we're ignoring it");
@@ -693,9 +717,14 @@ impl CtxMethods for BuildContext {
             Some(test_exec) => {
                 debug!("test: test_exec = {}", test_exec.display());
                 // FIXME (#9639): This needs to handle non-utf8 paths
-                let status = run::process_status(test_exec.as_str().unwrap(), [~"--test"]);
-                if !status.success() {
-                    fail!("Some tests failed");
+                let opt_status = run::process_status(test_exec.as_str().unwrap(), [~"--test"]);
+                match opt_status {
+                    Some(status) => {
+                        if !status.success() {
+                            fail!("Some tests failed");
+                        }
+                    },
+                    None => fail!("Could not exec `{}`", test_exec.display())
                 }
             }
             None => {
diff --git a/src/librustpkg/source_control.rs b/src/librustpkg/source_control.rs
index 702a849e4ad..cbb55030231 100644
--- a/src/librustpkg/source_control.rs
+++ b/src/librustpkg/source_control.rs
@@ -33,15 +33,16 @@ pub fn safe_git_clone(source: &Path, v: &Version, target: &Path) -> CloneResult
         if !target.exists() {
             debug!("Running: git clone {} {}", source.display(), target.display());
             // FIXME (#9639): This needs to handle non-utf8 paths
-            let outp = run::process_output("git", [~"clone",
-                                                   source.as_str().unwrap().to_owned(),
-                                                   target.as_str().unwrap().to_owned()]);
+            let opt_outp = run::process_output("git", [~"clone",
+                                                       source.as_str().unwrap().to_owned(),
+                                                       target.as_str().unwrap().to_owned()]);
+            let outp = opt_outp.expect("Failed to exec `git`");
             if !outp.status.success() {
                 println(str::from_utf8_owned(outp.output.clone()));
                 println(str::from_utf8_owned(outp.error));
                 return DirToUse(target.clone());
             }
-                else {
+            else {
                 match v {
                     &ExactRevision(ref s) => {
                         let git_dir = target.join(".git");
@@ -51,7 +52,7 @@ pub fn safe_git_clone(source: &Path, v: &Version, target: &Path) -> CloneResult
                         let outp = run::process_output("git",
                             [format!("--work-tree={}", target.as_str().unwrap().to_owned()),
                              format!("--git-dir={}", git_dir.as_str().unwrap().to_owned()),
-                             ~"checkout", format!("{}", *s)]);
+                             ~"checkout", format!("{}", *s)]).expect("Failed to exec `git`");
                         if !outp.status.success() {
                             println(str::from_utf8_owned(outp.output.clone()));
                             println(str::from_utf8_owned(outp.error));
@@ -72,7 +73,8 @@ pub fn safe_git_clone(source: &Path, v: &Version, target: &Path) -> CloneResult
             let args = [format!("--work-tree={}", target.as_str().unwrap().to_owned()),
                         format!("--git-dir={}", git_dir.as_str().unwrap().to_owned()),
                         ~"pull", ~"--no-edit", source.as_str().unwrap().to_owned()];
-            let outp = run::process_output("git", args);
+            let opt_outp = run::process_output("git", args);
+            let outp = opt_outp.expect("Failed to exec `git`");
             assert!(outp.status.success());
         }
         CheckedOutSources
@@ -108,8 +110,9 @@ pub fn git_clone_url(source: &str, target: &Path, v: &Version) {
     use conditions::git_checkout_failed::cond;
 
     // FIXME (#9639): This needs to handle non-utf8 paths
-    let outp = run::process_output("git", [~"clone", source.to_owned(),
-                                           target.as_str().unwrap().to_owned()]);
+    let opt_outp = run::process_output("git", [~"clone", source.to_owned(),
+                                               target.as_str().unwrap().to_owned()]);
+    let outp = opt_outp.expect("Failed to exec `git`");
     if !outp.status.success() {
          debug!("{}", str::from_utf8_owned(outp.output.clone()));
          debug!("{}", str::from_utf8_owned(outp.error));
@@ -118,8 +121,9 @@ pub fn git_clone_url(source: &str, target: &Path, v: &Version) {
     else {
         match v {
             &ExactRevision(ref s) | &Tagged(ref s) => {
-                    let outp = process_output_in_cwd("git", [~"checkout", s.to_owned()],
+                    let opt_outp = process_output_in_cwd("git", [~"checkout", s.to_owned()],
                                                          target);
+                    let outp = opt_outp.expect("Failed to exec `git`");
                     if !outp.status.success() {
                         debug!("{}", str::from_utf8_owned(outp.output.clone()));
                         debug!("{}", str::from_utf8_owned(outp.error));
@@ -131,10 +135,13 @@ pub fn git_clone_url(source: &str, target: &Path, v: &Version) {
     }
 }
 
-fn process_output_in_cwd(prog: &str, args: &[~str], cwd: &Path) -> ProcessOutput {
-    let mut prog = Process::new(prog, args, ProcessOptions{ dir: Some(cwd)
-                                ,..ProcessOptions::new()});
-    prog.finish_with_output()
+fn process_output_in_cwd(prog: &str, args: &[~str], cwd: &Path) -> Option<ProcessOutput> {
+    let mut opt_prog = Process::new(prog, args, ProcessOptions{ dir: Some(cwd)
+                                    ,..ProcessOptions::new()});
+    match opt_prog {
+        Some(ref mut prog) => Some(prog.finish_with_output()),
+        None => None
+    }
 }
 
 pub fn is_git_dir(p: &Path) -> bool {
diff --git a/src/librustpkg/tests.rs b/src/librustpkg/tests.rs
index 59fdf20941e..28bf8deb5a7 100644
--- a/src/librustpkg/tests.rs
+++ b/src/librustpkg/tests.rs
@@ -148,7 +148,7 @@ fn run_git(args: &[~str], env: Option<~[(~str, ~str)]>, cwd: &Path, err_msg: &st
         in_fd: None,
         out_fd: None,
         err_fd: None
-    });
+    }).expect("failed to exec `git`");
     let rslt = prog.finish_with_output();
     if !rslt.status.success() {
         fail!("{} [git returned {:?}, output = {}, error = {}]", err_msg,
@@ -285,7 +285,7 @@ fn command_line_test_with_env(args: &[~str], cwd: &Path, env: Option<~[(~str, ~s
         in_fd: None,
         out_fd: None,
         err_fd: None
-    });
+    }).expect(format!("failed to exec `{}`", cmd));
     let output = prog.finish_with_output();
     debug!("Output from command {} with args {:?} was {} \\{{}\\}[{:?}]",
            cmd, args, str::from_utf8(output.output),
@@ -503,7 +503,8 @@ fn touch_source_file(workspace: &Path, pkgid: &PkgId) {
             // n.b. Bumps time up by 2 seconds to get around granularity issues
             if !run::process_output("touch", [~"--date",
                                              ~"+2 seconds",
-                                             p.as_str().unwrap().to_owned()]).status.success() {
+                                             p.as_str().unwrap().to_owned()])
+                .expect("failed to exec `touch`").status.success() {
                 let _ = cond.raise((pkg_src_dir.clone(), ~"Bad path"));
             }
         }
@@ -521,7 +522,8 @@ fn touch_source_file(workspace: &Path, pkgid: &PkgId) {
             // FIXME (#9639): This needs to handle non-utf8 paths
             // n.b. Bumps time up by 2 seconds to get around granularity issues
             if !run::process_output("touch", [~"-A02",
-                                             p.as_str().unwrap().to_owned()]).status.success() {
+                                             p.as_str().unwrap().to_owned()])
+                .expect("failed to exec `touch`").status.success() {
                 let _ = cond.raise((pkg_src_dir.clone(), ~"Bad path"));
             }
         }
@@ -1276,7 +1278,7 @@ fn test_extern_mod() {
         in_fd: None,
         out_fd: None,
         err_fd: None
-    });
+    }).expect(format!("failed to exec `{}`", rustc.as_str().unwrap()));
     let outp = prog.finish_with_output();
     if !outp.status.success() {
         fail!("output was {}, error was {}",
@@ -1331,7 +1333,7 @@ fn test_extern_mod_simpler() {
         in_fd: None,
         out_fd: None,
         err_fd: None
-    });
+    }).expect(format!("failed to exec `{}`", rustc.as_str().unwrap()));
     let outp = prog.finish_with_output();
     if !outp.status.success() {
         fail!("output was {}, error was {}",
diff --git a/src/librustpkg/version.rs b/src/librustpkg/version.rs
index ba6cf5f513b..e9ccfccb126 100644
--- a/src/librustpkg/version.rs
+++ b/src/librustpkg/version.rs
@@ -104,8 +104,9 @@ pub fn try_getting_local_version(local_path: &Path) -> Option<Version> {
             continue;
         }
         // FIXME (#9639): This needs to handle non-utf8 paths
-        let outp = run::process_output("git",
+        let opt_outp = run::process_output("git",
                                    ["--git-dir=" + git_dir.as_str().unwrap(), ~"tag", ~"-l"]);
+        let outp = opt_outp.expect("Failed to exec `git`");
 
         debug!("git --git-dir={} tag -l ~~~> {:?}", git_dir.display(), outp.status);
 
@@ -140,9 +141,10 @@ pub fn try_getting_version(remote_path: &Path) -> Option<Version> {
                remote_path.display(),
                tmp_dir.display());
         // FIXME (#9639): This needs to handle non-utf8 paths
-        let outp  = run::process_output("git", [~"clone", format!("https://{}",
-                                                                  remote_path.as_str().unwrap()),
-                                                tmp_dir.as_str().unwrap().to_owned()]);
+        let opt_outp = run::process_output("git", [~"clone", format!("https://{}",
+                                                                     remote_path.as_str().unwrap()),
+                                                   tmp_dir.as_str().unwrap().to_owned()]);
+        let outp = opt_outp.expect("Failed to exec `git`");
         if outp.status.success() {
             debug!("Cloned it... ( {}, {} )",
                    str::from_utf8(outp.output),
@@ -152,9 +154,10 @@ pub fn try_getting_version(remote_path: &Path) -> Option<Version> {
             debug!("(getting version, now getting tags) executing \\{git --git-dir={} tag -l\\}",
                    git_dir.display());
             // FIXME (#9639): This needs to handle non-utf8 paths
-            let outp = run::process_output("git",
-                                           ["--git-dir=" + git_dir.as_str().unwrap(),
-                                            ~"tag", ~"-l"]);
+            let opt_outp = run::process_output("git",
+                                               ["--git-dir=" + git_dir.as_str().unwrap(),
+                                                ~"tag", ~"-l"]);
+            let outp = opt_outp.expect("Failed to exec `git`");
             let output_text = str::from_utf8(outp.output);
             debug!("Full output: ( {} ) [{:?}]", output_text, outp.status);
             for l in output_text.lines() {