diff --git a/Cargo.lock b/Cargo.lock index 5cf8a11d9d..509ff06252 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -552,12 +552,6 @@ dependencies = [ "crossbeam-utils 0.7.0", ] -[[package]] -name = "crossbeam-utils" -version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "677d453a17e8bd2b913fa38e8b9cf04bcdbb5be790aa294f2389661d72036015" - [[package]] name = "crossbeam-utils" version = "0.6.6" @@ -2301,6 +2295,7 @@ dependencies = [ "unicode-xid", "url", "users", + "which", ] [[package]] @@ -3802,11 +3797,11 @@ checksum = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a" [[package]] name = "subprocess" -version = "0.1.18" +version = "0.1.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "28fc0f40f0c0da73339d347aa7d6d2b90341a95683a47722bc4eebed71ff3c00" +checksum = "68713fc0f9d941642c1e020d622e6421dfe09e8891ddd4bfa2109fda9a40431d" dependencies = [ - "crossbeam-utils 0.5.0", + "crossbeam-utils 0.7.0", "libc", "winapi 0.3.8", ] @@ -4452,6 +4447,16 @@ dependencies = [ "nom 4.2.3", ] +[[package]] +name = "which" +version = "3.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5475d47078209a02e60614f7ba5e645ef3ed60f771920ac1906d7c1cc65024c8" +dependencies = [ + "failure", + "libc", +] + [[package]] name = "widestring" version = "0.4.0" diff --git a/Cargo.toml b/Cargo.toml index d676b133e2..5236b4b0ac 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -101,10 +101,11 @@ nom_locate = "1.0.0" nom-tracable = "0.4.1" unicode-xid = "0.2.0" serde_ini = "0.2.0" -subprocess = "0.1.18" +subprocess = "0.1.20" pretty-hex = "0.1.1" hex = "0.4" tempfile = "3.1.0" +which = "3.1.0" ichwh = "0.3" textwrap = {version = "0.11.0", features = ["term_size"]} shellexpand = "1.1.1" diff --git a/src/commands/classified/external.rs b/src/commands/classified/external.rs index ab92dc86fd..233a6b53b4 100644 --- a/src/commands/classified/external.rs +++ b/src/commands/classified/external.rs @@ -89,6 +89,14 @@ pub(crate) async fn run_external_command( ) -> Result, ShellError> { trace!(target: "nu::run::external", "-> {}", command.name); + if !did_find_command(&command.name) { + return Err(ShellError::labeled_error( + "Command not found", + "command not found", + &command.name_tag, + )); + } + if command.has_it_argument() { run_with_iterator_arg(command, context, input, is_last).await } else { @@ -333,13 +341,23 @@ async fn spawn( } } - yield Ok(Value { - value: UntaggedValue::Error(ShellError::labeled_error( - "External command failed", - "command failed", - &name_tag)), - tag: name_tag - }); + // 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, + }); + } + } return; } @@ -381,16 +399,23 @@ async fn spawn( None => futures_timer::Delay::new(std::time::Duration::from_millis(10)).await, Some(status) => { if !status.success() { - yield Ok(Value { - value: UntaggedValue::Error( - ShellError::labeled_error( - "External command failed", - "command failed", - &name_tag, - ) - ), - tag: name_tag, - }); + // 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; } @@ -408,6 +433,27 @@ async fn spawn( } } +fn did_find_command(name: &str) -> bool { + #[cfg(not(windows))] + { + which::which(name).is_ok() + } + + #[cfg(windows)] + { + if which::which(name).is_ok() { + true + } else { + let cmd_builtins = [ + "call", "cls", "color", "date", "dir", "echo", "find", "hostname", "pause", + "start", "time", "title", "ver", "copy", "mkdir", "rename", "rd", "rmdir", "type", + ]; + + cmd_builtins.contains(&name) + } + } +} + fn expand_tilde(input: &SI, home_dir: HD) -> std::borrow::Cow where SI: AsRef, @@ -459,70 +505,60 @@ fn shell_os_paths() -> Vec { mod tests { use super::{ add_quotes, argument_contains_whitespace, argument_is_quoted, expand_tilde, remove_quotes, - run_external_command, Context, OutputStream, + run_external_command, Context, }; use futures::executor::block_on; - use futures::stream::TryStreamExt; use nu_errors::ShellError; - use nu_protocol::{UntaggedValue, Value}; use nu_test_support::commands::ExternalBuilder; - async fn read(mut stream: OutputStream) -> Option { - match stream.try_next().await { - Ok(val) => { - if let Some(val) = val { - val.raw_value() - } else { - None - } - } - Err(_) => None, - } - } + // async fn read(mut stream: OutputStream) -> Option { + // match stream.try_next().await { + // Ok(val) => { + // if let Some(val) = val { + // val.raw_value() + // } else { + // None + // } + // } + // Err(_) => None, + // } + // } async fn non_existent_run() -> Result<(), ShellError> { let cmd = ExternalBuilder::for_name("i_dont_exist.exe").build(); let mut ctx = Context::basic().expect("There was a problem creating a basic context."); - let stream = run_external_command(cmd, &mut ctx, None, false) - .await? - .expect("There was a problem running the external command."); - - match read(stream.into()).await { - Some(Value { - value: UntaggedValue::Error(_), - .. - }) => {} - None | _ => panic!("Apparently a command was found (It's not supposed to be found)"), - } + assert!(run_external_command(cmd, &mut ctx, None, false) + .await + .is_err()); Ok(()) } - async fn failure_run() -> Result<(), ShellError> { - let cmd = ExternalBuilder::for_name("fail").build(); + // async fn failure_run() -> Result<(), ShellError> { + // let cmd = ExternalBuilder::for_name("fail").build(); - let mut ctx = Context::basic().expect("There was a problem creating a basic context."); - let stream = run_external_command(cmd, &mut ctx, None, false) - .await? - .expect("There was a problem running the external command."); + // let mut ctx = Context::basic().expect("There was a problem creating a basic context."); + // let stream = run_external_command(cmd, &mut ctx, None, false) + // .await? + // .expect("There was a problem running the external command."); - match read(stream.into()).await { - Some(Value { - value: UntaggedValue::Error(_), - .. - }) => {} - None | _ => panic!("Command didn't fail."), - } + // match read(stream.into()).await { + // Some(Value { + // value: UntaggedValue::Error(_), + // .. + // }) => {} + // None | _ => panic!("Command didn't fail."), + // } - Ok(()) - } + // Ok(()) + // } - #[test] - fn identifies_command_failed() -> Result<(), ShellError> { - block_on(failure_run()) - } + // #[test] + // fn identifies_command_failed() -> Result<(), ShellError> { + // block_on(failure_run()) + // } #[test] fn identifies_command_not_found() -> Result<(), ShellError> { diff --git a/tests/shell/pipeline/commands/external.rs b/tests/shell/pipeline/commands/external.rs index 9b0e82fef0..f0edd9e352 100644 --- a/tests/shell/pipeline/commands/external.rs +++ b/tests/shell/pipeline/commands/external.rs @@ -1,14 +1,14 @@ use nu_test_support::{nu, nu_error}; -#[test] -fn shows_error_for_command_that_fails() { - let actual = nu_error!( - cwd: ".", - "fail" - ); +// #[test] +// fn shows_error_for_command_that_fails() { +// let actual = nu_error!( +// cwd: ".", +// "fail" +// ); - assert!(actual.contains("External command failed")); -} +// assert!(actual.contains("External command failed")); +// } #[test] fn shows_error_for_command_not_found() { @@ -17,7 +17,7 @@ fn shows_error_for_command_not_found() { "ferris_is_not_here.exe" ); - assert!(actual.contains("External command failed")); + assert!(actual.contains("Command not found")); } mod it_evaluation {