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

@@ -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 mut process = std::process::Command::new(&head);
-
-        let new_head;
-
-        #[cfg(windows)]
-        {
-            new_head = head.replace("\\", "\\\\");
-        }
-
-        #[cfg(not(windows))]
-        {
-            new_head = head;
-        }
-
-        let mut process = std::process::Command::new(&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)]
+                            process.arg(&arg);
-                            {
-                                new_arg = arg.replace("\\", "\\\\");
-                            }
-
-                            #[cfg(not(windows))]
-                            {
-                                new_arg = arg;
-                            }
-
-                            process.arg(&new_arg);
                         } else {
-                            let new_arg;
+                            process.arg(&arg.item);
-
-                            #[cfg(windows)]
-                            {
-                                new_arg = arg.item.replace("\\", "\\\\");
-                            }
-
-                            #[cfg(not(windows))]
-                            {
-                                new_arg = arg.item.clone();
-                            }
-
-                            process.arg(&new_arg);
                         }
                     }
                 }
             } else {
-                let new_arg;
+                process.arg(&arg.item);
-
-                #[cfg(windows)]
-                {
-                    new_arg = arg.item.replace("\\", "\\\\");
-                }
-
-                #[cfg(not(windows))]
-                {
-                    new_arg = arg.item;
-                }
-
-                process.arg(&new_arg);
             }
         }
 

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"
-}

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
     )
 }