From 4c9df9c7c1abeea3ff54c640d7e2a01c8e2fa3a6 Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Mon, 31 Jan 2022 12:42:12 -0500 Subject: [PATCH] Add a fallback if Windows external spawn fails (#902) * Add a fallback if Windows external spawn fails * Remove path workaround * More fixes * More fixes * Be more flexible with error tests --- crates/nu-command/src/system/run_external.rs | 107 +++++++++---------- src/tests.rs | 7 +- src/tests/test_hiding.rs | 29 ++--- 3 files changed, 70 insertions(+), 73 deletions(-) diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 76376ebf78..560816cc9f 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -148,7 +148,55 @@ impl ExternalCommand { process.stdin(Stdio::piped()); } - match process.spawn() { + let child; + + #[cfg(windows)] + { + match process.spawn() { + Err(_) => { + let mut process = self.spawn_cmd_command(); + if let Some(d) = self.env_vars.get("PWD") { + process.current_dir(d); + } else { + return Err(ShellError::SpannedLabeledErrorHelp( + "Current directory not found".to_string(), + "did not find PWD environment variable".to_string(), + head, + concat!( + "The environment variable 'PWD' was not found. ", + "It is required to define the current directory when running an external command." + ).to_string(), + )); + }; + + process.envs(&self.env_vars); + + // If the external is not the last command, its output will get piped + // either as a string or binary + if !self.last_expression { + process.stdout(Stdio::piped()); + } + + // If there is an input from the pipeline. The stdin from the process + // is piped so it can be used to send the input information + if !matches!(input, PipelineData::Value(Value::Nothing { .. }, ..)) { + process.stdin(Stdio::piped()); + } + + child = process.spawn(); + } + Ok(process) => { + child = Ok(process); + } + } + } + + #[cfg(not(windows))] + { + child = process.spawn() + } + + match child { Err(err) => Err(ShellError::ExternalCommand( "can't run executable".to_string(), err.to_string(), @@ -289,21 +337,7 @@ impl ExternalCommand { head }; - //let head = head.replace("\\", "\\\\"); - - let new_head; - - #[cfg(windows)] - { - new_head = head.replace("\\", "\\\\"); - } - - #[cfg(not(windows))] - { - new_head = head; - } - - let mut process = std::process::Command::new(&new_head); + let mut process = std::process::Command::new(&head); for arg in self.args.iter() { let mut arg = Spanned { @@ -350,50 +384,15 @@ impl ExternalCommand { } else { arg.to_string_lossy().to_string() }; - let new_arg; - #[cfg(windows)] - { - new_arg = arg.replace("\\", "\\\\"); - } - - #[cfg(not(windows))] - { - new_arg = arg; - } - - process.arg(&new_arg); + process.arg(&arg); } else { - let new_arg; - - #[cfg(windows)] - { - new_arg = arg.item.replace("\\", "\\\\"); - } - - #[cfg(not(windows))] - { - new_arg = arg.item.clone(); - } - - process.arg(&new_arg); + process.arg(&arg.item); } } } } else { - let new_arg; - - #[cfg(windows)] - { - new_arg = arg.item.replace("\\", "\\\\"); - } - - #[cfg(not(windows))] - { - new_arg = arg.item; - } - - process.arg(&new_arg); + process.arg(&arg.item); } } diff --git a/src/tests.rs b/src/tests.rs index fa930cdf35..3a422fab48 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -93,12 +93,7 @@ pub fn fail_test(input: &str, expected: &str) -> TestResult { println!("stdout: {}", stdout); println!("stderr: {}", stderr); - assert!(stderr.contains(expected)); + assert!(!stderr.is_empty() && stderr.contains(expected)); Ok(()) } - -#[cfg(test)] -pub fn not_found_msg() -> &'static str { - "can't run executable" -} diff --git a/src/tests/test_hiding.rs b/src/tests/test_hiding.rs index 028d1a65ef..7929f2f04f 100644 --- a/src/tests/test_hiding.rs +++ b/src/tests/test_hiding.rs @@ -1,9 +1,12 @@ -use crate::tests::{fail_test, not_found_msg, run_test, TestResult}; +use crate::tests::{fail_test, run_test, TestResult}; // TODO: Test the use/hide tests also as separate lines in REPL (i.e., with merging the delta in between) #[test] fn hides_def() -> TestResult { - fail_test(r#"def foo [] { "foo" }; hide foo; foo"#, not_found_msg()) + fail_test( + r#"def foo [] { "foo" }; hide foo; foo"#, + "", // we just care if it errors + ) } #[test] @@ -33,7 +36,7 @@ fn hides_env_then_redefines() -> TestResult { fn hides_def_in_scope_1() -> TestResult { fail_test( r#"def foo [] { "foo" }; do { hide foo; foo }"#, - not_found_msg(), + "", // we just care if it errors ) } @@ -49,7 +52,7 @@ fn hides_def_in_scope_2() -> TestResult { fn hides_def_in_scope_3() -> TestResult { fail_test( r#"def foo [] { "foo" }; do { hide foo; def foo [] { "bar" }; hide foo; foo }"#, - not_found_msg(), + "", // we just care if it errors ) } @@ -57,7 +60,7 @@ fn hides_def_in_scope_3() -> TestResult { fn hides_def_in_scope_4() -> TestResult { fail_test( r#"def foo [] { "foo" }; do { def foo [] { "bar" }; hide foo; hide foo; foo }"#, - not_found_msg(), + "", // we just care if it errors ) } @@ -134,7 +137,7 @@ fn hides_def_and_env() -> TestResult { fn hides_def_import_1() -> TestResult { fail_test( r#"module spam { export def foo [] { "foo" } }; use spam; hide spam foo; spam foo"#, - not_found_msg(), + "", // we just care if it errors ) } @@ -142,7 +145,7 @@ fn hides_def_import_1() -> TestResult { fn hides_def_import_2() -> TestResult { fail_test( r#"module spam { export def foo [] { "foo" } }; use spam; hide spam; spam foo"#, - not_found_msg(), + "", // we just care if it errors ) } @@ -150,7 +153,7 @@ fn hides_def_import_2() -> TestResult { fn hides_def_import_3() -> TestResult { fail_test( r#"module spam { export def foo [] { "foo" } }; use spam; hide spam [foo]; spam foo"#, - not_found_msg(), + "", // we just care if it errors ) } @@ -158,7 +161,7 @@ fn hides_def_import_3() -> TestResult { fn hides_def_import_4() -> TestResult { fail_test( r#"module spam { export def foo [] { "foo" } }; use spam foo; hide foo; foo"#, - not_found_msg(), + "", // we just care if it errors ) } @@ -166,7 +169,7 @@ fn hides_def_import_4() -> TestResult { fn hides_def_import_5() -> TestResult { fail_test( r#"module spam { export def foo [] { "foo" } }; use spam *; hide foo; foo"#, - not_found_msg(), + "", // we just care if it errors ) } @@ -174,7 +177,7 @@ fn hides_def_import_5() -> TestResult { fn hides_def_import_6() -> TestResult { fail_test( r#"module spam { export def foo [] { "foo" } }; use spam *; hide spam *; foo"#, - not_found_msg(), + "", // we just care if it errors ) } @@ -246,7 +249,7 @@ fn hides_def_and_env_import_1() -> TestResult { fn hides_def_and_env_import_2() -> TestResult { fail_test( r#"module spam { export env foo { "foo" }; export def foo [] { "bar" } }; use spam foo; hide foo; hide foo; foo"#, - not_found_msg(), + "", // we just care if it errors ) } @@ -286,7 +289,7 @@ fn hide_shadowed_env() -> TestResult { fn hides_all_decls_within_scope() -> TestResult { fail_test( r#"module spam { export def foo [] { "bar" } }; def foo [] { "foo" }; use spam foo; hide foo; foo"#, - not_found_msg(), + "", // we just care if it errors ) }