Auto merge of #3091 - matklad:move-exec-streamed, r=alexcrichton

Move stream_output to ProcessBuilder

Make `stream_output` method more reusable (I intend to use it in #3000).

Unrelated question: what is that `ExecEngine` thing? Looks like it does nothing at the moment and can be removed.

src/cargo/ops/cargo_rustc/custom_build.rs

@@ -3,16 +3,12 @@ use std::fs;
 use std::path::{PathBuf, Path};
 use std::str;
 use std::sync::{Mutex, Arc};
-use std::process::{Stdio, Output};
 
 use core::PackageId;
-use util::{CargoResult, Human};
+use util::{CargoResult, Human, Freshness};
 use util::{internal, ChainError, profile, paths};
-use util::{Freshness, ProcessBuilder, read2};
-use util::errors::{process_error, ProcessError};
 
 use super::job::Work;
-use super::job_queue::JobState;
 use super::{fingerprint, Kind, Context, Unit};
 use super::CommandType;
 
@@ -209,7 +205,10 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>)
         // And now finally, run the build command itself!
         state.running(&p);
         let cmd = p.into_process_builder();
-        let output = try!(stream_output(state, &cmd).map_err(|mut e| {
+        let output = try!(cmd.exec_with_streaming(
+            &mut |out_line| state.stdout(out_line),
+            &mut |err_line| state.stderr(err_line),
+        ).map_err(|mut e| {
             e.desc = format!("failed to run custom build command for `{}`\n{}",
                              pkg_name, e.desc);
             Human(e)
@@ -443,55 +442,3 @@ pub fn build_map<'b, 'cfg>(cx: &mut Context<'b, 'cfg>,
         }
     }
 }
-
-fn stream_output(state: &JobState, cmd: &ProcessBuilder)
-                 -> Result<Output, ProcessError> {
-    let mut stdout = Vec::new();
-    let mut stderr = Vec::new();
-
-    let status = try!((|| {
-        let mut cmd = cmd.build_command();
-        cmd.stdout(Stdio::piped())
-           .stderr(Stdio::piped())
-           .stdin(Stdio::null());
-        let mut child = try!(cmd.spawn());
-        let out = child.stdout.take().unwrap();
-        let err = child.stderr.take().unwrap();
-
-        try!(read2(out, err, &mut |is_out, data, eof| {
-            let idx = if eof {
-                data.len()
-            } else {
-                match data.iter().rposition(|b| *b == b'\n') {
-                    Some(i) => i + 1,
-                    None => return,
-                }
-            };
-            let data = data.drain(..idx);
-            let dst = if is_out {&mut stdout} else {&mut stderr};
-            let start = dst.len();
-            dst.extend(data);
-            let s = String::from_utf8_lossy(&dst[start..]);
-            if is_out {
-                state.stdout(&s);
-            } else {
-                state.stderr(&s);
-            }
-        }));
-        child.wait()
-    })().map_err(|e| {
-        let msg = format!("could not exeute process {}", cmd);
-        process_error(&msg, Some(e), None, None)
-    }));
-    let output = Output {
-        stdout: stdout,
-        stderr: stderr,
-        status: status,
-    };
-    if !output.status.success() {
-        let msg = format!("process didn't exit successfully: {}", cmd);
-        Err(process_error(&msg, None, Some(&status), Some(&output)))
-    } else {
-        Ok(output)
-    }
-}

src/cargo/ops/cargo_rustc/job_queue.rs

@@ -170,12 +170,12 @@ impl<'a> JobQueue<'a> {
                 }
                 Message::Stdout(out) => {
                     if cx.config.extra_verbose() {
-                        try!(write!(cx.config.shell().out(), "{}", out));
+                        try!(writeln!(cx.config.shell().out(), "{}", out));
                     }
                 }
                 Message::Stderr(err) => {
                     if cx.config.extra_verbose() {
-                        try!(write!(cx.config.shell().err(), "{}", err));
+                        try!(writeln!(cx.config.shell().err(), "{}", err));
                     }
                 }
                 Message::Finish(result) => {

src/cargo/util/process_builder.rs

@@ -3,9 +3,9 @@ use std::env;
 use std::ffi::{OsString, OsStr};
 use std::fmt;
 use std::path::Path;
-use std::process::{Command, Output};
+use std::process::{Command, Stdio, Output};
 
-use util::{ProcessError, process_error};
+use util::{ProcessError, process_error, read2};
 use util::shell_escape::escape;
 
 #[derive(Clone, PartialEq, Debug)]
@@ -75,15 +75,15 @@ impl ProcessBuilder {
     pub fn exec(&self) -> Result<(), ProcessError> {
         let mut command = self.build_command();
         let exit = try!(command.status().map_err(|e| {
-            process_error(&format!("Could not execute process `{}`",
+            process_error(&format!("could not execute process `{}`",
                                    self.debug_string()),
                           Some(e), None, None)
         }));
 
         if exit.success() {
             Ok(())
         } else {
-            Err(process_error(&format!("Process didn't exit successfully: `{}`",
+            Err(process_error(&format!("process didn't exit successfully: `{}`",
                                        self.debug_string()),
                               None, Some(&exit), None))
         }
@@ -93,20 +93,77 @@ impl ProcessBuilder {
         let mut command = self.build_command();
 
         let output = try!(command.output().map_err(|e| {
-            process_error(&format!("Could not execute process `{}`",
+            process_error(&format!("could not execute process `{}`",
                                self.debug_string()),
                           Some(e), None, None)
         }));
 
         if output.status.success() {
             Ok(output)
         } else {
-            Err(process_error(&format!("Process didn't exit successfully: `{}`",
+            Err(process_error(&format!("process didn't exit successfully: `{}`",
                                        self.debug_string()),
                               None, Some(&output.status), Some(&output)))
         }
     }
 
+    pub fn exec_with_streaming(&self,
+                               on_stdout_line: &mut FnMut(&str),
+                               on_stderr_line: &mut FnMut(&str))
+                               -> Result<Output, ProcessError> {
+        let mut stdout = Vec::new();
+        let mut stderr = Vec::new();
+
+        let mut cmd = self.build_command();
+        cmd.stdout(Stdio::piped())
+            .stderr(Stdio::piped())
+            .stdin(Stdio::null());
+
+        let status = try!((|| {
+            let mut child = try!(cmd.spawn());
+            let out = child.stdout.take().unwrap();
+            let err = child.stderr.take().unwrap();
+            try!(read2(out, err, &mut |is_out, data, eof| {
+                let idx = if eof {
+                    data.len()
+                } else {
+                    match data.iter().rposition(|b| *b == b'\n') {
+                        Some(i) => i + 1,
+                        None => return,
+                    }
+                };
+                let data = data.drain(..idx);
+                let dst = if is_out {&mut stdout} else {&mut stderr};
+                let start = dst.len();
+                dst.extend(data);
+                for line in String::from_utf8_lossy(&dst[start..]).lines() {
+                    if is_out {
+                        on_stdout_line(line)
+                    } else {
+                        on_stderr_line(line)
+                    }
+                }
+            }));
+            child.wait()
+        })().map_err(|e| {
+            process_error(&format!("could not execute process `{}`",
+                                   self.debug_string()),
+                          Some(e), None, None)
+        }));
+        let output = Output {
+            stdout: stdout,
+            stderr: stderr,
+            status: status,
+        };
+        if !output.status.success() {
+            Err(process_error(&format!("process didn't exit successfully: `{}`",
+                                       self.debug_string()),
+                              None, Some(&output.status), Some(&output)))
+        } else {
+            Ok(output)
+        }
+    }
+
     pub fn build_command(&self) -> Command {
         let mut command = Command::new(&self.program);
         if let Some(cwd) = self.get_cwd() {

tests/build-script.rs

@@ -715,7 +715,7 @@ fn build_deps_not_for_normal() {
 [ERROR] Could not compile `foo`.
 
 Caused by:
-  Process didn't exit successfully: [..]
+  process didn't exit successfully: [..]
 "));
 }
 

tests/build.rs

@@ -1967,7 +1967,7 @@ fn rustc_env_var() {
                  .env("RUSTC", "rustc-that-does-not-exist").arg("-v"),
                 execs().with_status(101)
                        .with_stderr("\
-[ERROR] Could not execute process `rustc-that-does-not-exist -vV` ([..])
+[ERROR] could not execute process `rustc-that-does-not-exist -vV` ([..])
 
 Caused by:
 [..]

tests/run.rs

@@ -132,7 +132,7 @@ fn exit_code() {
 [COMPILING] foo v0.0.1 (file[..])
 [FINISHED] debug [unoptimized + debuginfo] target(s) in [..]
 [RUNNING] `target[..]`
-[ERROR] Process didn't exit successfully: `target[..]foo[..]` (exit code: 2)
+[ERROR] process didn't exit successfully: `target[..]foo[..]` (exit code: 2)
 "));
 }
 
@@ -156,7 +156,7 @@ fn exit_code_verbose() {
 [RUNNING] `rustc [..]`
 [FINISHED] debug [unoptimized + debuginfo] target(s) in [..]
 [RUNNING] `target[..]`
-[ERROR] Process didn't exit successfully: `target[..]foo[..]` (exit code: 2)
+[ERROR] process didn't exit successfully: `target[..]foo[..]` (exit code: 2)
 "));
 }