Merge pull request #392 from fitzgen/child-process-and-output-management

Child process and output management

src/bindgen.rs

@@ -1,8 +1,10 @@
 //! Functionality related to installing and running `wasm-bindgen`.
 
 use binaries::{self, bin_path, install_binaries_from_targz_at_url};
+use child;
 use emoji;
 use error::Error;
+use failure::{self, ResultExt};
 use progressbar::Step;
 use slog::Logger;
 use std::path::{Path, PathBuf};
@@ -22,7 +24,7 @@ pub fn install_wasm_bindgen(
     install_permitted: bool,
     step: &Step,
     log: &Logger,
-) -> Result<(), Error> {
+) -> Result<(), failure::Error> {
     // If the `wasm-bindgen` dependency is already met, print a message and return.
     if wasm_bindgen_path(log, root_path)
         .map(|bindgen_path| wasm_bindgen_version_check(&bindgen_path, version, log))
@@ -37,7 +39,7 @@ pub fn install_wasm_bindgen(
     // permitted, return a configuration error.
     if !install_permitted {
         let msg = format!("wasm-bindgen v{} is not installed!", version);
-        return Error::crate_config(&msg);
+        return Err(Error::crate_config(&msg).into());
     }
 
     let msg = format!("{}Installing wasm-bindgen...", emoji::DOWN_ARROW);
@@ -48,7 +50,7 @@ pub fn install_wasm_bindgen(
             log,
             "could not download pre-built `wasm-bindgen`: {}. Falling back to `cargo install`.", e
         );
-        cargo_install_wasm_bindgen(root_path, version)
+        cargo_install_wasm_bindgen(log, root_path, version)
     })
 }
 
@@ -81,28 +83,23 @@ pub fn download_prebuilt_wasm_bindgen(root_path: &Path, version: &str) -> Result
 
 /// Use `cargo install` to install the `wasm-bindgen` CLI locally into the given
 /// crate.
-pub fn cargo_install_wasm_bindgen(crate_path: &Path, version: &str) -> Result<(), Error> {
+pub fn cargo_install_wasm_bindgen(
-    let output = Command::new("cargo")
+    logger: &Logger,
-        .arg("install")
+    crate_path: &Path,
+    version: &str,
+) -> Result<(), failure::Error> {
+    let mut cmd = Command::new("cargo");
+    cmd.arg("install")
         .arg("--force")
         .arg("wasm-bindgen-cli")
         .arg("--version")
         .arg(version)
         .arg("--root")
-        .arg(crate_path)
+        .arg(crate_path);
-        .output()?;
+
-    if !output.status.success() {
+    child::run(logger, cmd, "cargo install").context("Installing wasm-bindgen with cargo")?;
-        let message = "Installing wasm-bindgen failed".to_string();
+    assert!(binaries::local_bin_path(crate_path, "wasm-bindgen").is_file());
-        let s = String::from_utf8_lossy(&output.stderr);
+    Ok(())
-        Err(Error::Cli {
-            message,
-            stderr: s.to_string(),
-            exit_status: output.status,
-        })
-    } else {
-        assert!(binaries::local_bin_path(crate_path, "wasm-bindgen").is_file());
-        Ok(())
-    }
 }
 
 /// Run the `wasm-bindgen` CLI to generate bindings for the current crate's
@@ -116,7 +113,7 @@ pub fn wasm_bindgen_build(
     debug: bool,
     step: &Step,
     log: &Logger,
-) -> Result<(), Error> {
+) -> Result<(), failure::Error> {
     let msg = format!("{}Running WASM-bindgen...", emoji::RUNNER);
     PBAR.step(step, &msg);
 
@@ -153,27 +150,20 @@ pub fn wasm_bindgen_build(
             cmd.arg("--debug");
         }
 
-        let output = cmd.output()?;
+        child::run(log, cmd, "wasm-bindgen").context("Running the wasm-bindgen CLI")?;
-        if !output.status.success() {
+        Ok(())
-            let s = String::from_utf8_lossy(&output.stderr);
-            Error::cli("wasm-bindgen failed to execute properly", s, output.status)
-        } else {
-            Ok(())
-        }
     } else {
-        Error::crate_config("Could not find `wasm-bindgen`")
+        Err(Error::crate_config("Could not find `wasm-bindgen`").into())
     }
 }
 
 /// Check if the `wasm-bindgen` dependency is locally satisfied.
 fn wasm_bindgen_version_check(bindgen_path: &PathBuf, dep_version: &str, log: &Logger) -> bool {
-    Command::new(bindgen_path)
+    let mut cmd = Command::new(bindgen_path);
-        .arg("--version")
+    cmd.arg("--version");
-        .output()
+    child::run(log, cmd, "wasm-bindgen")
-        .ok()
+        .map(|stdout| {
-        .filter(|output| output.status.success())
+            stdout
-        .map(|output| {
-            String::from_utf8_lossy(&output.stdout)
                 .trim()
                 .split_whitespace()
                 .nth(1)

src/build.rs

@@ -1,8 +1,11 @@
 //! Building a Rust crate into a `.wasm` binary.
 
+use child;
 use emoji;
 use error::Error;
+use failure::ResultExt;
 use progressbar::Step;
+use slog::Logger;
 use std::path::Path;
 use std::process::Command;
 use std::str;
@@ -54,28 +57,23 @@ fn rustc_minor_version() -> Option<u32> {
 
 /// Ensure that `rustup` has the `wasm32-unknown-unknown` target installed for
 /// current toolchain
-pub fn rustup_add_wasm_target(step: &Step) -> Result<(), Error> {
+pub fn rustup_add_wasm_target(log: &Logger, step: &Step) -> Result<(), failure::Error> {
     let msg = format!("{}Adding WASM target...", emoji::TARGET);
     PBAR.step(step, &msg);
-    let output = Command::new("rustup")
+    let mut cmd = Command::new("rustup");
-        .arg("target")
+    cmd.arg("target").arg("add").arg("wasm32-unknown-unknown");
-        .arg("add")
+    child::run(log, cmd, "rustup")
-        .arg("wasm32-unknown-unknown")
+        .context("Adding the wasm32-unknown-unknown target with rustup")?;
-        .output()?;
+    Ok(())
-    if !output.status.success() {
-        let s = String::from_utf8_lossy(&output.stderr);
-        Error::cli(
-            "Adding the wasm32-unknown-unknown target failed",
-            s,
-            output.status,
-        )
-    } else {
-        Ok(())
-    }
 }
 
 /// Run `cargo build` targetting `wasm32-unknown-unknown`.
-pub fn cargo_build_wasm(path: &Path, debug: bool, step: &Step) -> Result<(), Error> {
+pub fn cargo_build_wasm(
+    log: &Logger,
+    path: &Path,
+    debug: bool,
+    step: &Step,
+) -> Result<(), failure::Error> {
     let msg = format!("{}Compiling to WASM...", emoji::CYCLONE);
     PBAR.step(step, &msg);
     let mut cmd = Command::new("cargo");
@@ -84,32 +82,22 @@ pub fn cargo_build_wasm(path: &Path, debug: bool, step: &Step) -> Result<(), Err
         cmd.arg("--release");
     }
     cmd.arg("--target").arg("wasm32-unknown-unknown");
-    let output = cmd.output()?;
+    child::run(log, cmd, "cargo build").context("Compiling your crate to WebAssembly")?;
-
+    Ok(())
-    if !output.status.success() {
-        let s = String::from_utf8_lossy(&output.stderr);
-        Error::cli("Compilation of your program failed", s, output.status)
-    } else {
-        Ok(())
-    }
 }
 
 /// Run `cargo build --tests` targetting `wasm32-unknown-unknown`.
-pub fn cargo_build_wasm_tests(path: &Path, debug: bool) -> Result<(), Error> {
+pub fn cargo_build_wasm_tests(
-    let output = {
+    log: &Logger,
-        let mut cmd = Command::new("cargo");
+    path: &Path,
-        cmd.current_dir(path).arg("build").arg("--tests");
+    debug: bool,
-        if !debug {
+) -> Result<(), failure::Error> {
-            cmd.arg("--release");
+    let mut cmd = Command::new("cargo");
-        }
+    cmd.current_dir(path).arg("build").arg("--tests");
-        cmd.arg("--target").arg("wasm32-unknown-unknown");
+    if !debug {
-        cmd.output()?
+        cmd.arg("--release");
-    };
-
-    if !output.status.success() {
-        let s = String::from_utf8_lossy(&output.stderr);
-        Error::cli("Compilation of your program failed", s, output.status)
-    } else {
-        Ok(())
     }
+    cmd.arg("--target").arg("wasm32-unknown-unknown");
+    child::run(log, cmd, "cargo build").context("Compilation of your program failed")?;
+    Ok(())
 }

src/child.rs

@@ -0,0 +1,177 @@
+//! Utilties for managing child processes.
+//!
+//! This module helps us ensure that all child processes that we spawn get
+//! properly logged and their output is logged as well.
+
+use error::Error;
+use failure;
+use slog::Logger;
+use std::{
+    io::{self, Read},
+    mem, process, string,
+    sync::mpsc,
+    thread,
+};
+use PBAR;
+
+#[derive(Debug)]
+enum OutputFragment {
+    Stdout(Vec<u8>),
+    Stderr(Vec<u8>),
+}
+
+/// Read data from the give reader and send it as an `OutputFragment` over the
+/// given sender.
+fn read_and_send<R, F>(
+    mut reader: R,
+    sender: mpsc::Sender<OutputFragment>,
+    mut map: F,
+) -> io::Result<()>
+where
+    R: Read,
+    F: FnMut(Vec<u8>) -> OutputFragment,
+{
+    let mut buf = vec![0; 1024];
+    loop {
+        match reader.read(&mut buf) {
+            Err(e) => {
+                if e.kind() == io::ErrorKind::Interrupted {
+                    continue;
+                } else {
+                    return Err(e);
+                }
+            }
+            Ok(0) => return Ok(()),
+            Ok(n) => {
+                buf.truncate(n);
+                let buf = mem::replace(&mut buf, vec![0; 1024]);
+                sender.send(map(buf)).unwrap();
+            }
+        }
+    }
+}
+
+/// Accumulates output from a stream of output fragments and calls a callback on
+/// each complete line as it is accumulating.
+struct OutputAccumulator<F> {
+    result: String,
+    in_progress: Vec<u8>,
+    on_each_line: F,
+}
+
+impl<F> OutputAccumulator<F>
+where
+    F: FnMut(&str),
+{
+    /// Construct a new output accumulator with the given `on_each_line`
+    /// callback.
+    fn new(on_each_line: F) -> OutputAccumulator<F> {
+        OutputAccumulator {
+            result: String::new(),
+            in_progress: Vec::new(),
+            on_each_line,
+        }
+    }
+
+    /// Add another fragment of output to the accumulation, calling the
+    /// `on_each_line` callback for any complete lines we accumulate.
+    fn push(&mut self, fragment: Vec<u8>) -> Result<(), string::FromUtf8Error> {
+        debug_assert!(!fragment.is_empty());
+        self.in_progress.extend(fragment);
+
+        if let Some((last_newline, _)) = self
+            .in_progress
+            .iter()
+            .cloned()
+            .enumerate()
+            .rev()
+            .find(|(_, ch)| *ch == b'\n')
+        {
+            let next_in_progress: Vec<u8> = self.in_progress[last_newline + 1..]
+                .iter()
+                .cloned()
+                .collect();
+            let mut these_lines = mem::replace(&mut self.in_progress, next_in_progress);
+            these_lines.truncate(last_newline + 1);
+            let these_lines = String::from_utf8(these_lines)?;
+            for line in these_lines.lines() {
+                (self.on_each_line)(line);
+            }
+            self.result.push_str(&these_lines);
+        }
+
+        Ok(())
+    }
+
+    /// Finish accumulation, run the `on_each_line` callback on the final line
+    /// (if any), and return the accumulated output.
+    fn finish(mut self) -> Result<String, string::FromUtf8Error> {
+        if !self.in_progress.is_empty() {
+            let last_line = String::from_utf8(self.in_progress)?;
+            (self.on_each_line)(&last_line);
+            self.result.push_str(&last_line);
+        }
+        Ok(self.result)
+    }
+}
+
+/// Run the given command and return its stdout.
+pub fn run(
+    logger: &Logger,
+    mut command: process::Command,
+    command_name: &str,
+) -> Result<String, failure::Error> {
+    info!(logger, "Running {:?}", command);
+
+    let mut child = command
+        .stdout(process::Stdio::piped())
+        .stderr(process::Stdio::piped())
+        .spawn()?;
+
+    let stdout = child.stdout.take().unwrap();
+    let stderr = child.stderr.take().unwrap();
+
+    let (send, recv) = mpsc::channel();
+    let stdout_send = send.clone();
+    let stderr_send = send;
+
+    // Because pipes have a fixed-size buffer, we need to keep reading stdout
+    // and stderr on a separate thread to avoid potential dead locks with
+    // waiting on the child process.
+
+    let stdout_handle =
+        thread::spawn(move || read_and_send(stdout, stdout_send, OutputFragment::Stdout));
+    let stderr_handle =
+        thread::spawn(move || read_and_send(stderr, stderr_send, OutputFragment::Stderr));
+
+    let mut stdout = OutputAccumulator::new(|line| {
+        info!(logger, "{} (stdout): {}", command_name, line);
+        PBAR.message(line)
+    });
+    let mut stderr = OutputAccumulator::new(|line| {
+        info!(logger, "{} (stderr): {}", command_name, line);
+        PBAR.message(line)
+    });
+
+    for output in recv {
+        match output {
+            OutputFragment::Stdout(line) => stdout.push(line)?,
+            OutputFragment::Stderr(line) => stderr.push(line)?,
+        };
+    }
+
+    let stdout = stdout.finish()?;
+    let stderr = stderr.finish()?;
+
+    // Join the threads reading the child's output to make sure the finish OK.
+    stdout_handle.join().unwrap()?;
+    stderr_handle.join().unwrap()?;
+
+    let exit = child.wait()?;
+    if exit.success() {
+        return Ok(stdout);
+    } else {
+        let msg = format!("`{}` did not exit successfully", command_name);
+        return Err(Error::cli(&msg, stdout.into(), stderr.into(), exit).into());
+    }
+}

src/command/build.rs

@@ -55,7 +55,7 @@ impl FromStr for BuildMode {
             "no-install" => Ok(BuildMode::Noinstall),
             "normal" => Ok(BuildMode::Normal),
             "force" => Ok(BuildMode::Force),
-            _ => Error::crate_config(&format!("Unknown build mode: {}", s)).map(|_| unreachable!()),
+            _ => Err(Error::crate_config(&format!("Unknown build mode: {}", s))),
         }
     }
 }
@@ -94,11 +94,11 @@ pub struct BuildOptions {
     pub out_dir: String,
 }
 
-type BuildStep = fn(&mut Build, &Step, &Logger) -> Result<(), Error>;
+type BuildStep = fn(&mut Build, &Step, &Logger) -> Result<(), failure::Error>;
 
 impl Build {
     /// Construct a build command from the given options.
-    pub fn try_from_opts(build_opts: BuildOptions) -> Result<Self, Error> {
+    pub fn try_from_opts(build_opts: BuildOptions) -> Result<Self, failure::Error> {
         let crate_path = set_crate_path(build_opts.path)?;
         let crate_name = manifest::get_crate_name(&crate_path)?;
         let out_dir = crate_path.join(PathBuf::from(build_opts.out_dir));
@@ -117,7 +117,7 @@ impl Build {
     }
 
     /// Execute this `Build` command.
-    pub fn run(&mut self, log: &Logger) -> Result<(), Error> {
+    pub fn run(&mut self, log: &Logger) -> Result<(), failure::Error> {
         let process_steps = Build::get_process_steps(&self.mode);
 
         let mut step_counter = Step::new(process_steps.len());
@@ -188,7 +188,11 @@ impl Build {
         }
     }
 
-    fn step_check_rustc_version(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_check_rustc_version(
+        &mut self,
+        step: &Step,
+        log: &Logger,
+    ) -> Result<(), failure::Error> {
         info!(&log, "Checking rustc version...");
         let version = build::check_rustc_version(step)?;
         let msg = format!("rustc version is {}.", version);
@@ -196,23 +200,23 @@ impl Build {
         Ok(())
     }
 
-    fn step_check_crate_config(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_check_crate_config(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         info!(&log, "Checking crate configuration...");
         manifest::check_crate_config(&self.crate_path, step)?;
         info!(&log, "Crate is correctly configured.");
         Ok(())
     }
 
-    fn step_add_wasm_target(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_add_wasm_target(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         info!(&log, "Adding wasm-target...");
-        build::rustup_add_wasm_target(step)?;
+        build::rustup_add_wasm_target(log, step)?;
         info!(&log, "Adding wasm-target was successful.");
         Ok(())
     }
 
-    fn step_build_wasm(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_build_wasm(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         info!(&log, "Building wasm...");
-        build::cargo_build_wasm(&self.crate_path, self.debug, step)?;
+        build::cargo_build_wasm(log, &self.crate_path, self.debug, step)?;
 
         info!(
             &log,
@@ -226,14 +230,14 @@ impl Build {
         Ok(())
     }
 
-    fn step_create_dir(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_create_dir(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         info!(&log, "Creating a pkg directory...");
         create_pkg_dir(&self.out_dir, step)?;
         info!(&log, "Created a pkg directory at {:#?}.", &self.crate_path);
         Ok(())
     }
 
-    fn step_create_json(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_create_json(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         info!(&log, "Writing a package.json...");
         manifest::write_package_json(
             &self.crate_path,
@@ -251,14 +255,18 @@ impl Build {
         Ok(())
     }
 
-    fn step_copy_readme(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_copy_readme(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         info!(&log, "Copying readme from crate...");
         readme::copy_from_crate(&self.crate_path, &self.out_dir, step)?;
         info!(&log, "Copied readme from crate to {:#?}.", &self.out_dir);
         Ok(())
     }
 
-    fn step_install_wasm_bindgen(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_install_wasm_bindgen(
+        &mut self,
+        step: &Step,
+        log: &Logger,
+    ) -> Result<(), failure::Error> {
         info!(&log, "Identifying wasm-bindgen dependency...");
         let lockfile = Lockfile::new(&self.crate_path)?;
         let bindgen_version = lockfile.require_wasm_bindgen()?;
@@ -288,7 +296,7 @@ impl Build {
         Ok(())
     }
 
-    fn step_run_wasm_bindgen(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_run_wasm_bindgen(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         info!(&log, "Building the wasm bindings...");
         bindgen::wasm_bindgen_build(
             &self.crate_path,

src/command/login.rs

@@ -1,4 +1,3 @@
-use error::Error;
 use npm;
 use slog::Logger;
 use std::result;
@@ -10,7 +9,7 @@ pub fn login(
     always_auth: bool,
     auth_type: Option<String>,
     log: &Logger,
-) -> result::Result<(), Error> {
+) -> result::Result<(), failure::Error> {
     let registry = registry.unwrap_or(npm::DEFAULT_NPM_REGISTRY.to_string());
 
     info!(&log, "Logging in to npm...");
@@ -23,7 +22,7 @@ pub fn login(
         &auth_type
     );
     info!(&log, "npm info located in the npm debug log");
-    npm::npm_login(&registry, &scope, always_auth, &auth_type)?;
+    npm::npm_login(log, &registry, &scope, always_auth, &auth_type)?;
     info!(&log, "Logged you in!");
 
     PBAR.message(&format!("👋  logged you in!"));

src/command/mod.rs

@@ -84,7 +84,7 @@ pub enum Command {
 }
 
 /// Run a command with the given logger!
-pub fn run_wasm_pack(command: Command, log: &Logger) -> result::Result<(), Error> {
+pub fn run_wasm_pack(command: Command, log: &Logger) -> result::Result<(), failure::Error> {
     // Run the correct command based off input and store the result of it so that we can clear
     // the progress bar then return it
     let status = match command {
@@ -129,7 +129,12 @@ pub fn run_wasm_pack(command: Command, log: &Logger) -> result::Result<(), Error
         Ok(_) => {}
         Err(ref e) => {
             error!(&log, "{}", e);
-            PBAR.error(e.error_type());
+            for c in e.iter_chain() {
+                if let Some(e) = c.downcast_ref::<Error>() {
+                    PBAR.error(e.error_type());
+                    break;
+                }
+            }
         }
     }
 

src/command/pack.rs

@@ -8,7 +8,7 @@ use PBAR;
 
 /// Executes the 'npm pack' command on the 'pkg' directory
 /// which creates a tarball that can be published to the NPM registry
-pub fn pack(path: Option<PathBuf>, log: &Logger) -> result::Result<(), Error> {
+pub fn pack(path: Option<PathBuf>, log: &Logger) -> result::Result<(), failure::Error> {
     let crate_path = set_crate_path(path)?;
 
     info!(&log, "Packing up the npm package...");
@@ -18,7 +18,7 @@ pub fn pack(path: Option<PathBuf>, log: &Logger) -> result::Result<(), Error> {
             &crate_path, &crate_path
         ),
     })?;
-    npm::npm_pack(&pkg_directory.to_string_lossy())?;
+    npm::npm_pack(log, &pkg_directory.to_string_lossy())?;
     info!(
         &log,
         "Your package is located at {:#?}",

src/command/publish/mod.rs

@@ -16,7 +16,7 @@ pub fn publish(
     path: Option<PathBuf>,
     access: Option<Access>,
     log: &Logger,
-) -> result::Result<(), Error> {
+) -> result::Result<(), failure::Error> {
     let crate_path = set_crate_path(path)?;
 
     info!(&log, "Publishing the npm package...");
@@ -28,7 +28,7 @@ pub fn publish(
         ),
     })?;
 
-    npm::npm_publish(&pkg_directory.to_string_lossy(), access)?;
+    npm::npm_publish(log, &pkg_directory.to_string_lossy(), access)?;
     info!(&log, "Published your package!");
 
     PBAR.message("💥  published your package!");

src/command/test.rs

@@ -94,11 +94,11 @@ pub struct Test {
     test_runner_path: Option<PathBuf>,
 }
 
-type TestStep = fn(&mut Test, &Step, &Logger) -> Result<(), Error>;
+type TestStep = fn(&mut Test, &Step, &Logger) -> Result<(), failure::Error>;
 
 impl Test {
     /// Construct a test command from the given options.
-    pub fn try_from_opts(test_opts: TestOptions) -> Result<Self, Error> {
+    pub fn try_from_opts(test_opts: TestOptions) -> Result<Self, failure::Error> {
         let TestOptions {
             path,
             node,
@@ -124,18 +124,18 @@ impl Test {
         let any_browser = chrome || firefox || safari;
 
         if !node && !any_browser {
-            return Error::crate_config(
+            return Err(Error::crate_config(
                 "Must specify at least one of `--node`, `--chrome`, `--firefox`, or `--safari`",
             )
-            .map(|_| unreachable!());
+            .into());
         }
 
         if headless && !any_browser {
-            return Error::crate_config(
+            return Err(Error::crate_config(
                 "The `--headless` flag only applies to browser tests. Node does not provide a UI, \
                  so it doesn't make sense to talk about a headless version of Node tests.",
             )
-            .map(|_| unreachable!());
+            .into());
         }
 
         Ok(Test {
@@ -155,7 +155,7 @@ impl Test {
     }
 
     /// Execute this test command.
-    pub fn run(mut self, log: &Logger) -> Result<(), Error> {
+    pub fn run(mut self, log: &Logger) -> Result<(), failure::Error> {
         let process_steps = self.get_process_steps();
         let mut step_counter = Step::new(process_steps.len());
 
@@ -227,40 +227,48 @@ impl Test {
         }
     }
 
-    fn step_check_rustc_version(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_check_rustc_version(
+        &mut self,
+        step: &Step,
+        log: &Logger,
+    ) -> Result<(), failure::Error> {
         info!(log, "Checking rustc version...");
         let _ = build::check_rustc_version(step)?;
         info!(log, "Rustc version is correct.");
         Ok(())
     }
 
-    fn step_check_crate_config(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_check_crate_config(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         info!(log, "Checking crate configuration...");
         manifest::check_crate_config(&self.crate_path, step)?;
         info!(log, "Crate is correctly configured.");
         Ok(())
     }
 
-    fn step_add_wasm_target(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_add_wasm_target(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         info!(&log, "Adding wasm-target...");
-        build::rustup_add_wasm_target(step)?;
+        build::rustup_add_wasm_target(log, step)?;
         info!(&log, "Adding wasm-target was successful.");
         Ok(())
     }
 
-    fn step_build_tests(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_build_tests(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         info!(log, "Compiling tests to wasm...");
 
         let msg = format!("{}Compiling tests to WASM...", emoji::CYCLONE);
         PBAR.step(step, &msg);
 
-        build::cargo_build_wasm_tests(&self.crate_path, !self.release)?;
+        build::cargo_build_wasm_tests(log, &self.crate_path, !self.release)?;
 
         info!(log, "Finished compiling tests to wasm.");
         Ok(())
     }
 
-    fn step_install_wasm_bindgen(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_install_wasm_bindgen(
+        &mut self,
+        step: &Step,
+        log: &Logger,
+    ) -> Result<(), failure::Error> {
         info!(&log, "Identifying wasm-bindgen dependency...");
         let lockfile = Lockfile::new(&self.crate_path)?;
         let bindgen_version = lockfile.require_wasm_bindgen()?;
@@ -277,7 +285,7 @@ impl Test {
                  wasm-bindgen-test = \"0.2\"",
                 style("wasm-bindgen-test").bold().dim(),
             );
-            return Err(Error::CrateConfig { message });
+            return Err(Error::CrateConfig { message }.into());
         }
 
         let install_permitted = match self.mode {
@@ -310,7 +318,7 @@ impl Test {
         Ok(())
     }
 
-    fn step_test_node(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_test_node(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         assert!(self.node);
         info!(log, "Running tests in node...");
         PBAR.step(step, "Running tests in node...");
@@ -327,7 +335,7 @@ impl Test {
         Ok(())
     }
 
-    fn step_get_chromedriver(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_get_chromedriver(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         PBAR.step(step, "Getting chromedriver...");
         assert!(self.chrome && self.chromedriver.is_none());
 
@@ -339,7 +347,7 @@ impl Test {
         Ok(())
     }
 
-    fn step_test_chrome(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_test_chrome(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         PBAR.step(step, "Running tests in Chrome...");
 
         let chromedriver = self.chromedriver.as_ref().unwrap().display().to_string();
@@ -366,10 +374,11 @@ impl Test {
             envs.push(("NO_HEADLESS", "1"));
         }
 
-        test::cargo_test_wasm(&self.crate_path, self.release, log, envs)
+        test::cargo_test_wasm(&self.crate_path, self.release, log, envs)?;
+        Ok(())
     }
 
-    fn step_get_geckodriver(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_get_geckodriver(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         PBAR.step(step, "Getting geckodriver...");
         assert!(self.firefox && self.geckodriver.is_none());
 
@@ -381,7 +390,7 @@ impl Test {
         Ok(())
     }
 
-    fn step_test_firefox(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_test_firefox(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         PBAR.step(step, "Running tests in Firefox...");
 
         let geckodriver = self.geckodriver.as_ref().unwrap().display().to_string();
@@ -408,10 +417,11 @@ impl Test {
             envs.push(("NO_HEADLESS", "1"));
         }
 
-        test::cargo_test_wasm(&self.crate_path, self.release, log, envs)
+        test::cargo_test_wasm(&self.crate_path, self.release, log, envs)?;
+        Ok(())
     }
 
-    fn step_get_safaridriver(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_get_safaridriver(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         PBAR.step(step, "Getting safaridriver...");
         assert!(self.safari && self.safaridriver.is_none());
 
@@ -419,7 +429,7 @@ impl Test {
         Ok(())
     }
 
-    fn step_test_safari(&mut self, step: &Step, log: &Logger) -> Result<(), Error> {
+    fn step_test_safari(&mut self, step: &Step, log: &Logger) -> Result<(), failure::Error> {
         PBAR.step(step, "Running tests in Safari...");
 
         let safaridriver = self.safaridriver.as_ref().unwrap().display().to_string();
@@ -446,6 +456,7 @@ impl Test {
             envs.push(("NO_HEADLESS", "1"));
         }
 
-        test::cargo_test_wasm(&self.crate_path, self.release, log, envs)
+        test::cargo_test_wasm(&self.crate_path, self.release, log, envs)?;
+        Ok(())
     }
 }

src/error.rs

@@ -48,14 +48,17 @@ pub enum Error {
 
     /// An error invoking another CLI tool.
     #[fail(
-        display = "Process exited with {}: {}. stderr:\n\n{}",
+        display = "Process exited with {}: {}.\n\nstdout:{}\n\nstderr:\n\n{}",
         exit_status,
         message,
+        stdout,
         stderr
     )]
     Cli {
         /// Error message.
         message: String,
+        /// The underlying CLI's `stdout` output.
+        stdout: String,
         /// The underlying CLI's `stderr` output.
         stderr: String,
         /// The exit status of the subprocess
@@ -101,19 +104,20 @@ pub enum Error {
 
 impl Error {
     /// Construct a CLI error.
-    pub fn cli(message: &str, stderr: Cow<str>, exit_status: ExitStatus) -> Result<(), Self> {
+    pub fn cli(message: &str, stdout: Cow<str>, stderr: Cow<str>, exit_status: ExitStatus) -> Self {
-        Err(Error::Cli {
+        Error::Cli {
             message: message.to_string(),
+            stdout: stdout.to_string(),
             stderr: stderr.to_string(),
             exit_status,
-        })
+        }
     }
 
     /// Construct a crate configuration error.
-    pub fn crate_config(message: &str) -> Result<(), Self> {
+    pub fn crate_config(message: &str) -> Self {
-        Err(Error::CrateConfig {
+        Error::CrateConfig {
             message: message.to_string(),
-        })
+        }
     }
 
     /// Construct an archive error.
@@ -161,6 +165,7 @@ impl Error {
             } => "Your rustc version is not supported. Please install version 1.30.0 or higher.",
             Error::Cli {
                 message: _,
+                stdout: _,
                 stderr: _,
                 exit_status: _,
             } => "There was an error while calling another CLI tool. Details:\n\n",

src/lib.rs

@@ -29,6 +29,7 @@ extern crate zip;
 pub mod binaries;
 pub mod bindgen;
 pub mod build;
+pub mod child;
 pub mod command;
 pub mod emoji;
 pub mod error;

src/manifest/mod.rs

@@ -79,11 +79,10 @@ struct CargoLib {
 fn read_cargo_toml(path: &Path) -> Result<CargoManifest, Error> {
     let manifest_path = path.join("Cargo.toml");
     if !manifest_path.is_file() {
-        return Error::crate_config(&format!(
+        return Err(Error::crate_config(&format!(
             "Crate directory is missing a `Cargo.toml` file; is `{}` the wrong directory?",
             path.display()
-        ))
+        )));
-        .map(|_| unreachable!());
     }
     let mut cargo_file = File::open(manifest_path)?;
     let mut cargo_contents = String::new();
@@ -255,10 +254,10 @@ fn check_crate_type(path: &Path) -> Result<(), Error> {
     }) {
         return Ok(());
     }
-    Error::crate_config(
+    Err(Error::crate_config(
       "crate-type must be cdylib to compile to wasm32-unknown-unknown. Add the following to your \
        Cargo.toml file:\n\n\
        [lib]\n\
        crate-type = [\"cdylib\", \"rlib\"]"
-    )
+    ))
 }

src/npm.rs

@@ -1,56 +1,51 @@
 //! Functionality related to publishing to npm.
 
+use child;
 use command::publish::access::Access;
-use error::Error;
+use failure::{self, ResultExt};
+use slog::Logger;
 use std::process::{Command, Stdio};
 
 /// The default npm registry used when we aren't working with a custom registry.
 pub const DEFAULT_NPM_REGISTRY: &'static str = "https://registry.npmjs.org/";
 
 /// Run the `npm pack` command.
-pub fn npm_pack(path: &str) -> Result<(), Error> {
+pub fn npm_pack(log: &Logger, path: &str) -> Result<(), failure::Error> {
-    let output = Command::new("npm").current_dir(path).arg("pack").output()?;
+    let mut cmd = Command::new("npm");
-    if !output.status.success() {
+    cmd.current_dir(path).arg("pack");
-        let s = String::from_utf8_lossy(&output.stderr);
+    child::run(log, cmd, "npm pack").context("Packaging up your code failed")?;
-        Error::cli("Packaging up your code failed", s, output.status)
+    Ok(())
-    } else {
-        Ok(())
-    }
 }
 
 /// Run the `npm publish` command.
-pub fn npm_publish(path: &str, access: Option<Access>) -> Result<(), Error> {
+pub fn npm_publish(log: &Logger, path: &str, access: Option<Access>) -> Result<(), failure::Error> {
-    let output = match access {
+    let mut cmd = Command::new("npm");
-        Some(a) => Command::new("npm")
+    match access {
+        Some(a) => cmd
             .current_dir(path)
             .arg("publish")
             .arg(&format!("{}", a.to_string()))
             .stdin(Stdio::inherit())
-            .stdout(Stdio::inherit())
+            .stdout(Stdio::inherit()),
-            .output()?,
+        None => cmd
-        None => Command::new("npm")
             .current_dir(path)
             .arg("publish")
             .stdin(Stdio::inherit())
-            .stdout(Stdio::inherit())
+            .stdout(Stdio::inherit()),
-            .output()?,
     };
 
-    if !output.status.success() {
+    child::run(log, cmd, "npm publish").context("Publishing to npm failed")?;
-        let s = String::from_utf8_lossy(&output.stderr);
+    Ok(())
-        Error::cli("Publishing to npm failed", s, output.status)
-    } else {
-        Ok(())
-    }
 }
 
 /// Run the `npm login` command.
 pub fn npm_login(
+    log: &Logger,
     registry: &String,
     scope: &Option<String>,
     always_auth: bool,
     auth_type: &Option<String>,
-) -> Result<(), Error> {
+) -> Result<(), failure::Error> {
     let mut args = String::new();
 
     args.push_str(&format!("--registry={}", registry));
@@ -67,21 +62,12 @@ pub fn npm_login(
         args.push_str(&format!(" --auth_type={}", auth_type));
     }
 
-    let output = Command::new("npm")
+    let mut cmd = Command::new("npm");
-        .arg("login")
+    cmd.arg("login")
         .arg(args)
         .stdin(Stdio::inherit())
-        .stdout(Stdio::inherit())
+        .stdout(Stdio::inherit());
-        .output()?;
+    child::run(log, cmd, "npm login")
-
+        .with_context(|_| format!("Login to registry {} failed", registry))?;
-    if !output.status.success() {
+    Ok(())
-        let s = String::from_utf8_lossy(&output.stderr);
-        Error::cli(
-            &format!("Login to registry {} failed", registry),
-            s,
-            output.status,
-        )
-    } else {
-        Ok(())
-    }
 }

src/test/mod.rs

@@ -2,7 +2,8 @@
 
 pub mod webdriver;
 
-use error::Error;
+use child;
+use failure::{self, ResultExt};
 use slog::Logger;
 use std::ffi::OsStr;
 use std::path::Path;
@@ -15,7 +16,7 @@ pub fn cargo_test_wasm<I, K, V>(
     release: bool,
     log: &Logger,
     envs: I,
-) -> Result<(), Error>
+) -> Result<(), failure::Error>
 where
     I: IntoIterator<Item = (K, V)>,
     K: AsRef<OsStr>,
@@ -35,17 +36,13 @@ where
             cmd.arg("--release");
         }
         cmd.arg("--target").arg("wasm32-unknown-unknown");
-        cmd.output()?
+        child::run(log, cmd, "cargo test")
+            .context("Running Wasm tests with wasm-bindgen-test failed")?
     };
 
-    if !output.status.success() {
+    for line in output.lines() {
-        let s = String::from_utf8_lossy(&output.stderr);
+        info!(log, "test output: {}", line);
-        Error::cli("Running wasm tests failed", s, output.status)
+        println!("{}", line);
-    } else {
-        for line in String::from_utf8_lossy(&output.stdout).lines() {
-            info!(log, "test output: {}", line);
-            println!("{}", line);
-        }
-        Ok(())
     }
+    Ok(())
 }

src/test/webdriver.rs

@@ -20,12 +20,11 @@ pub fn get_or_install_chromedriver(
         (_, Some(path)) => Ok(path),
         (BuildMode::Normal, None) => install_chromedriver(crate_path),
         (BuildMode::Force, None) => install_chromedriver(crate_path),
-        (BuildMode::Noinstall, None) => Error::crate_config(
+        (BuildMode::Noinstall, None) => Err(Error::crate_config(
             "No crate-local `chromedriver` binary found, and could not find a global \
              `chromedriver` on the `$PATH`. Not installing `chromedriver` because of noinstall \
              mode.",
-        )
+        )),
-        .map(|_| unreachable!()),
     }
 }
 
@@ -72,11 +71,10 @@ pub fn get_or_install_geckodriver(
         (_, Some(path)) => Ok(path),
         (BuildMode::Normal, None) => install_geckodriver(crate_path),
         (BuildMode::Force, None) => install_geckodriver(crate_path),
-        (BuildMode::Noinstall, None) => Error::crate_config(
+        (BuildMode::Noinstall, None) => Err(Error::crate_config(
             "No crate-local `geckodriver` binary found, and could not find a global `geckodriver` \
              on the `$PATH`. Not installing `geckodriver` because of noinstall mode.",
-        )
+        )),
-        .map(|_| unreachable!()),
     }
 }
 
@@ -133,6 +131,8 @@ pub fn get_safaridriver(log: &Logger, crate_path: &Path) -> Result<PathBuf, Erro
     if let Some(p) = bin_path(log, crate_path, "safaridriver") {
         Ok(p)
     } else {
-        Error::crate_config("could not find `safaridriver` on the `$PATH`").map(|_| unreachable!())
+        Err(Error::crate_config(
+            "could not find `safaridriver` on the `$PATH`",
+        ))
     }
 }

tests/all/main.rs

@@ -2,6 +2,8 @@ extern crate failure;
 #[macro_use]
 extern crate serde_derive;
 extern crate serde_json;
+#[macro_use]
+extern crate slog;
 extern crate structopt;
 extern crate tempfile;
 extern crate wasm_pack;

tests/all/utils/fixture.rs

@@ -1,3 +1,4 @@
+use super::logger::null_logger;
 use std::env;
 use std::fs;
 use std::io;
@@ -6,9 +7,8 @@ use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
 use std::sync::{Once, ONCE_INIT};
 use std::thread;
-use wasm_pack;
-
 use tempfile::TempDir;
+use wasm_pack;
 
 fn hard_link_or_copy<P1: AsRef<Path>, P2: AsRef<Path>>(from: P1, to: P2) -> io::Result<()> {
     let from = from.as_ref();
@@ -151,7 +151,11 @@ impl Fixture {
             const WASM_BINDGEN_VERSION: &str = "0.2.21";
             wasm_pack::bindgen::download_prebuilt_wasm_bindgen(&tests, WASM_BINDGEN_VERSION)
                 .or_else(|_| {
-                    wasm_pack::bindgen::cargo_install_wasm_bindgen(&tests, WASM_BINDGEN_VERSION)
+                    wasm_pack::bindgen::cargo_install_wasm_bindgen(
+                        &null_logger(),
+                        &tests,
+                        WASM_BINDGEN_VERSION,
+                    )
                 })
                 .unwrap();
         });

tests/all/utils/logger.rs

@@ -0,0 +1,6 @@
+use slog::Logger;
+
+// Create a logger that ignores log messages for testing.
+pub fn null_logger() -> Logger {
+    Logger::root(slog::Discard, o!())
+}

tests/all/utils/mod.rs

@@ -1,3 +1,4 @@
 pub mod file;
 pub mod fixture;
+pub mod logger;
 pub mod manifest;