From 32dfb32741bc59291885bfdea263a66740a6a170 Mon Sep 17 00:00:00 2001 From: Jason Gedge Date: Sat, 25 Jan 2020 22:03:21 -0500 Subject: [PATCH] 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 --- Cargo.lock | 14 ----- Cargo.toml | 1 - crates/nu-errors/Cargo.toml | 1 - crates/nu-errors/src/lib.rs | 6 -- crates/nu-protocol/Cargo.toml | 1 - docs/commands/to-toml.md | 1 - src/commands/classified/external.rs | 74 ++++++++++-------------- tests/fixtures/formats/cargo_sample.toml | 1 - 8 files changed, 32 insertions(+), 67 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 509ff06252..107637c766 100644 --- a/Cargo.lock +++ b/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" diff --git a/Cargo.toml b/Cargo.toml index 5236b4b0ac..2b7b6fa50c 100644 --- a/Cargo.toml +++ b/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" diff --git a/crates/nu-errors/Cargo.toml b/crates/nu-errors/Cargo.toml index ed4e623034..03d87172ec 100644 --- a/crates/nu-errors/Cargo.toml +++ b/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" diff --git a/crates/nu-errors/src/lib.rs b/crates/nu-errors/src/lib.rs index a78689d9de..141fe708c8 100644 --- a/crates/nu-errors/src/lib.rs +++ b/crates/nu-errors/src/lib.rs @@ -821,12 +821,6 @@ impl std::convert::From for ShellError { } } -impl std::convert::From for ShellError { - fn from(input: subprocess::PopenError) -> ShellError { - ShellError::untagged_runtime_error(format!("{}", input)) - } -} - impl std::convert::From for ShellError { fn from(input: serde_yaml::Error) -> ShellError { ShellError::untagged_runtime_error(format!("{:?}", input)) diff --git a/crates/nu-protocol/Cargo.toml b/crates/nu-protocol/Cargo.toml index d4f25a1d5a..c0743cec37 100644 --- a/crates/nu-protocol/Cargo.toml +++ b/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" diff --git a/docs/commands/to-toml.md b/docs/commands/to-toml.md index a026520696..b459a7c187 100644 --- a/docs/commands/to-toml.md +++ b/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" diff --git a/src/commands/classified/external.rs b/src/commands/classified/external.rs index 233a6b53b4..c1724099d9 100644 --- a/src/commands/classified/external.rs +++ b/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,30 +394,20 @@ 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. - let cfg = crate::data::config::config(Tag::unknown()); - if let Ok(cfg) = cfg { - if cfg.contains_key("nonzero_exit_errors") { - yield Ok(Value { - value: UntaggedValue::Error( - ShellError::labeled_error( - "External command failed", - "command failed", - &name_tag, - ) - ), - tag: name_tag, - }); - } - } - } - break; + 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") { + yield Ok(Value { + value: UntaggedValue::Error( + ShellError::labeled_error( + "External command failed", + "command failed", + &name_tag, + ) + ), + tag: name_tag, + }); } } } diff --git a/tests/fixtures/formats/cargo_sample.toml b/tests/fixtures/formats/cargo_sample.toml index f845705179..cdecccd4c0 100644 --- a/tests/fixtures/formats/cargo_sample.toml +++ b/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"