Switch from subprocess crate to the builtin std::process (#1284)

* Switch from subprocess crate to the builtin std::process

* Update external.rs

* Update external.rs

* Update external.rs

Co-authored-by: Jonathan Turner <jonathandturner@users.noreply.github.com>

Cargo.lock

@@ -2282,7 +2282,6 @@ dependencies = [
  "shellexpand",
  "starship",
  "strip-ansi-escapes",
- "subprocess",
  "syntect",
  "tempfile",
  "term",
@@ -2326,7 +2325,6 @@ dependencies = [
  "serde 1.0.104",
  "serde_json",
  "serde_yaml",
- "subprocess",
  "toml 0.5.6",
 ]
 
@@ -2411,7 +2409,6 @@ dependencies = [
  "serde_bytes",
  "serde_json",
  "serde_yaml",
- "subprocess",
  "toml 0.5.6",
  "typetag",
 ]
@@ -3795,17 +3792,6 @@ version = "0.8.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a"
 
-[[package]]
-name = "subprocess"
-version = "0.1.20"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "68713fc0f9d941642c1e020d622e6421dfe09e8891ddd4bfa2109fda9a40431d"
-dependencies = [
- "crossbeam-utils 0.7.0",
- "libc",
- "winapi 0.3.8",
-]
-
 [[package]]
 name = "surf"
 version = "1.0.3"

Cargo.toml

@@ -101,7 +101,6 @@ nom_locate = "1.0.0"
 nom-tracable = "0.4.1"
 unicode-xid = "0.2.0"
 serde_ini = "0.2.0"
-subprocess = "0.1.20"
 pretty-hex = "0.1.1"
 hex = "0.4"
 tempfile = "3.1.0"

crates/nu-errors/Cargo.toml

@@ -24,7 +24,6 @@ nom_locate = "1.0.0"
 getset = "0.0.9"
 
 # implement conversions
-subprocess = "0.1.18"
 serde_yaml = "0.8"
 toml = "0.5.5"
 serde_json = "1.0.44"

crates/nu-errors/src/lib.rs

@@ -821,12 +821,6 @@ impl std::convert::From<std::io::Error> for ShellError {
     }
 }
 
-impl std::convert::From<subprocess::PopenError> for ShellError {
-    fn from(input: subprocess::PopenError) -> ShellError {
-        ShellError::untagged_runtime_error(format!("{}", input))
-    }
-}
-
 impl std::convert::From<serde_yaml::Error> for ShellError {
     fn from(input: serde_yaml::Error) -> ShellError {
         ShellError::untagged_runtime_error(format!("{:?}", input))

crates/nu-protocol/Cargo.toml

@@ -33,7 +33,6 @@ byte-unit = "3.0.3"
 natural = "0.3.0"
 
 # implement conversions
-subprocess = "0.1.18"
 serde_yaml = "0.8"
 toml = "0.5.5"
 serde_json = "1.0.44"

docs/commands/to-toml.md

@@ -70,7 +70,6 @@ rustyline = "4.1.0"
 serde = "1.0.91"
 serde_derive = "1.0.91"
 serde_json = "1.0.39"
-subprocess = "0.1.18"
 sysinfo = "0.8.4"
 term = "0.5.2"
 tokio-fs = "0.1.6"

src/commands/classified/external.rs

@@ -1,14 +1,14 @@
 use crate::prelude::*;
 use bytes::{BufMut, BytesMut};
 use futures::stream::StreamExt;
-use futures_codec::{Decoder, Encoder, Framed};
+use futures_codec::{Decoder, Encoder, FramedRead};
 use log::trace;
 use nu_errors::ShellError;
 use nu_parser::ExternalCommand;
 use nu_protocol::{Primitive, ShellTypeName, UntaggedValue, Value};
 use std::io::{Error, ErrorKind, Write};
 use std::ops::Deref;
-use subprocess::Exec;
+use std::process::{Command, Stdio};
 
 /// A simple `Codec` implementation that splits up data into lines.
 pub struct LinesCodec {}
@@ -275,11 +275,11 @@ async fn spawn(
     let mut process = {
         #[cfg(windows)]
         {
-            let mut process = Exec::cmd("cmd");
-            process = process.arg("/c");
-            process = process.arg(&command.name);
+            let mut process = Command::new("cmd");
+            process.arg("/c");
+            process.arg(&command.name);
             for arg in args {
-                process = process.arg(&arg);
+                process.arg(&arg);
             }
             process
         }
@@ -287,34 +287,34 @@ async fn spawn(
         #[cfg(not(windows))]
         {
             let cmd_with_args = vec![command.name.clone(), args.join(" ")].join(" ");
-            Exec::shell(&cmd_with_args)
+            let mut process = Command::new("sh");
+            process.arg("-c").arg(cmd_with_args);
+            process
         }
     };
 
-    process = process.cwd(path);
+    process.current_dir(path);
     trace!(target: "nu::run::external", "cwd = {:?}", &path);
 
     // We want stdout regardless of what
     // we are doing ($it case or pipe stdin)
     if !is_last {
-        process = process.stdout(subprocess::Redirection::Pipe);
+        process.stdout(Stdio::piped());
         trace!(target: "nu::run::external", "set up stdout pipe");
     }
 
     // open since we have some contents for stdin
     if stdin_contents.is_some() {
-        process = process.stdin(subprocess::Redirection::Pipe);
+        process.stdin(Stdio::piped());
         trace!(target: "nu::run::external", "set up stdin pipe");
     }
 
-    trace!(target: "nu::run::external", "built process {:?}", process);
+    trace!(target: "nu::run::external", "built command {:?}", process);
 
-    let popen = process.detached().popen();
-
-    if let Ok(mut popen) = popen {
+    if let Ok(mut child) = process.spawn() {
         let stream = async_stream! {
             if let Some(mut input) = stdin_contents.as_ref() {
-                let mut stdin_write = popen.stdin
+                let mut stdin_write = child.stdin
                     .take()
                     .expect("Internal error: could not get stdin pipe for external command");
 
@@ -335,7 +335,7 @@ async fn spawn(
             }
 
             if is_last && command.has_it_argument() {
-                if let Ok(status) = popen.wait() {
+                if let Ok(status) = child.wait() {
                     if status.success() {
                         return;
                     }
@@ -362,7 +362,7 @@ async fn spawn(
             }
 
             if !is_last {
-                let stdout = if let Some(stdout) = popen.stdout.take() {
+                let stdout = if let Some(stdout) = child.stdout.take() {
                     stdout
                 } else {
                     yield Ok(Value {
@@ -376,7 +376,7 @@ async fn spawn(
                 };
 
                 let file = futures::io::AllowStdIo::new(stdout);
-                let stream = Framed::new(file, LinesCodec {});
+                let stream = FramedRead::new(file, LinesCodec {});
 
                 let mut stream = stream.map(|line| {
                     if let Ok(line) = line {
@@ -394,13 +394,7 @@ async fn spawn(
                 }
             }
 
-            loop {
-                match popen.poll() {
-                    None => futures_timer::Delay::new(std::time::Duration::from_millis(10)).await,
-                    Some(status) => {
-                        if !status.success() {
-                            // We can give an error when we see a non-zero exit code, but this is different
-                            // than what other shells will do.
+            if child.wait().is_err() {
                 let cfg = crate::data::config::config(Tag::unknown());
                 if let Ok(cfg) = cfg {
                     if cfg.contains_key("nonzero_exit_errors") {
@@ -417,10 +411,6 @@ async fn spawn(
                     }
                 }
             }
-                        break;
-                    }
-                }
-            }
         };
 
         Ok(Some(stream.to_input_stream()))

tests/fixtures/formats/cargo_sample.toml

@@ -19,7 +19,6 @@ itertools = "0.8.0"
 ansi_term = "0.11.0"
 conch-parser = "0.1.1"
 nom = "5.0.0-beta1"
-subprocess = "0.1.18"
 dunce = "1.0.0"
 indexmap = { version = "1.0.2", features = ["serde-1"] }
 chrono-humanize = "0.0.11"