From 4ca47258a0c112953c4cb71feb3b15c9942cfbac Mon Sep 17 00:00:00 2001 From: Jelle Besseling Date: Fri, 28 Apr 2023 14:55:48 +0200 Subject: [PATCH] Add --redirect-combine option to run-external (#8918) # Description Add option that combines both output streams to the `run-external` command. This allows you to do something like this: ```nushell let res = do -i { run-external --redirect-combine } | complete if $res.exit_code != 0 { # Only print output when command has failed. print "The command has failed, these are the logs:" print $res.stdout } ``` # User-Facing Changes No breaking changes, just an extra option. # Tests + Formatting Added a test that checks the new option # After Submitting Co-authored-by: Jelle Besseling --- Cargo.lock | 11 +++ crates/nu-command/Cargo.toml | 1 + crates/nu-command/src/env/config/utils.rs | 1 + crates/nu-command/src/system/exec.rs | 2 + crates/nu-command/src/system/run_external.rs | 72 ++++++++++++++----- .../nu-command/tests/commands/run_external.rs | 15 ++++ 6 files changed, 86 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4bfb9f2db1..707c5f0d76 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2868,6 +2868,7 @@ dependencies = [ "num-traits", "once_cell", "open", + "os_pipe", "pathdiff", "percent-encoding", "polars", @@ -3427,6 +3428,16 @@ dependencies = [ "hashbrown 0.12.3", ] +[[package]] +name = "os_pipe" +version = "1.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a53dbb20faf34b16087a931834cba2d7a73cc74af2b7ef345a4c8324e2409a12" +dependencies = [ + "libc", + "windows-sys 0.45.0", +] + [[package]] name = "os_str_bytes" version = "6.5.0" diff --git a/crates/nu-command/Cargo.toml b/crates/nu-command/Cargo.toml index c2667f6bf9..ce33ecf93d 100644 --- a/crates/nu-command/Cargo.toml +++ b/crates/nu-command/Cargo.toml @@ -100,6 +100,7 @@ uuid = { version = "1.3.0", features = ["v4"] } wax = { version = "0.5.0" } which = { version = "4.4.0", optional = true } print-positions = "0.6.1" +os_pipe = "1.1.3" [target.'cfg(windows)'.dependencies] winreg = "0.50.0" diff --git a/crates/nu-command/src/env/config/utils.rs b/crates/nu-command/src/env/config/utils.rs index 2bef0d1d40..0e68f2b738 100644 --- a/crates/nu-command/src/env/config/utils.rs +++ b/crates/nu-command/src/env/config/utils.rs @@ -71,6 +71,7 @@ pub(crate) fn gen_command( arg_keep_raw: vec![false; number_of_args], redirect_stdout: false, redirect_stderr: false, + redirect_combine: false, env_vars: env_vars_str, trim_end_newline: false, } diff --git a/crates/nu-command/src/system/exec.rs b/crates/nu-command/src/system/exec.rs index e988d18960..f14692d17b 100644 --- a/crates/nu-command/src/system/exec.rs +++ b/crates/nu-command/src/system/exec.rs @@ -67,6 +67,7 @@ fn exec( let redirect_stdout = call.has_flag("redirect-stdout"); let redirect_stderr = call.has_flag("redirect-stderr"); + let redirect_combine = call.has_flag("redirect-combine"); let trim_end_newline = call.has_flag("trim-end-newline"); let external_command = create_external_command( @@ -75,6 +76,7 @@ fn exec( call, redirect_stdout, redirect_stderr, + redirect_combine, trim_end_newline, )?; diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 59f295642b..c3f4d5db70 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -11,6 +11,7 @@ use nu_protocol::{ SyntaxShape, Type, Value, }; use nu_system::ForegroundProcess; +use os_pipe::PipeReader; use pathdiff::diff_paths; use std::collections::HashMap; use std::io::{BufRead, BufReader, Read, Write}; @@ -41,6 +42,11 @@ impl Command for External { .input_output_types(vec![(Type::Any, Type::Any)]) .switch("redirect-stdout", "redirect stdout to the pipeline", None) .switch("redirect-stderr", "redirect stderr to the pipeline", None) + .switch( + "redirect-combine", + "redirect both stdout and stderr combined to the pipeline (collected in stdout)", + None, + ) .switch("trim-end-newline", "trimming end newlines", None) .required("command", SyntaxShape::String, "external command to run") .rest("args", SyntaxShape::Any, "arguments for external command") @@ -56,14 +62,25 @@ impl Command for External { ) -> Result { let redirect_stdout = call.has_flag("redirect-stdout"); let redirect_stderr = call.has_flag("redirect-stderr"); + let redirect_combine = call.has_flag("redirect-combine"); let trim_end_newline = call.has_flag("trim-end-newline"); + if redirect_combine && (redirect_stdout || redirect_stderr) { + return Err(ShellError::ExternalCommand { + label: "Cannot use --redirect-combine with --redirect-stdout or --redirect-stderr" + .into(), + help: "use either --redirect-combine or redirect a single output stream".into(), + span: call.head, + }); + } + let command = create_external_command( engine_state, stack, call, redirect_stdout, redirect_stderr, + redirect_combine, trim_end_newline, )?; @@ -93,6 +110,7 @@ pub fn create_external_command( call: &Call, redirect_stdout: bool, redirect_stderr: bool, + redirect_combine: bool, trim_end_newline: bool, ) -> Result { let name: Spanned = call.req(engine_state, stack, 0)?; @@ -149,6 +167,7 @@ pub fn create_external_command( arg_keep_raw, redirect_stdout, redirect_stderr, + redirect_combine, env_vars: env_vars_str, trim_end_newline, }) @@ -161,6 +180,7 @@ pub struct ExternalCommand { pub arg_keep_raw: Vec, pub redirect_stdout: bool, pub redirect_stderr: bool, + pub redirect_combine: bool, pub env_vars: HashMap, pub trim_end_newline: bool, } @@ -177,10 +197,10 @@ impl ExternalCommand { let ctrlc = engine_state.ctrlc.clone(); - let mut fg_process = ForegroundProcess::new( - self.create_process(&input, false, head)?, - engine_state.pipeline_externals_state.clone(), - ); + #[allow(unused_mut)] + let (cmd, mut reader) = self.create_process(&input, false, head)?; + let mut fg_process = + ForegroundProcess::new(cmd, engine_state.pipeline_externals_state.clone()); // mut is used in the windows branch only, suppress warning on other platforms #[allow(unused_mut)] let mut child; @@ -211,8 +231,10 @@ impl ExternalCommand { .any(|&cmd| command_name_upper == cmd); if looks_like_cmd_internal { + let (cmd, new_reader) = self.create_process(&input, true, head)?; + reader = new_reader; let mut cmd_process = ForegroundProcess::new( - self.create_process(&input, true, head)?, + cmd, engine_state.pipeline_externals_state.clone(), ); child = cmd_process.spawn(); @@ -242,9 +264,11 @@ impl ExternalCommand { item: file_name.to_string_lossy().to_string(), span: self.name.span, }; + let (cmd, new_reader) = new_command + .create_process(&input, true, head)?; + reader = new_reader; let mut cmd_process = ForegroundProcess::new( - new_command - .create_process(&input, true, head)?, + cmd, engine_state.pipeline_externals_state.clone(), ); child = cmd_process.spawn(); @@ -419,6 +443,7 @@ impl ExternalCommand { let commandname = self.name.item.clone(); let redirect_stdout = self.redirect_stdout; let redirect_stderr = self.redirect_stderr; + let redirect_combine = self.redirect_combine; let span = self.name.span; let output_ctrlc = ctrlc.clone(); let stderr_ctrlc = ctrlc.clone(); @@ -441,6 +466,12 @@ impl ExternalCommand { .to_string(), span } })?; + read_and_redirect_message(stdout, stdout_tx, ctrlc) + } else if redirect_combine { + let stdout = reader.ok_or_else(|| { + ShellError::ExternalCommand { label: "Error taking combined stdout and stderr from external".to_string(), help: "Combined redirects need access to reader pipe of an external command" + .to_string(), span } + })?; read_and_redirect_message(stdout, stdout_tx, ctrlc) } @@ -516,7 +547,7 @@ impl ExternalCommand { let exit_code_receiver = ValueReceiver::new(exit_code_rx); Ok(PipelineData::ExternalStream { - stdout: if redirect_stdout { + stdout: if redirect_stdout || redirect_combine { Some(RawStream::new( Box::new(stdout_receiver), output_ctrlc.clone(), @@ -553,7 +584,7 @@ impl ExternalCommand { input: &PipelineData, use_cmd: bool, span: Span, - ) -> Result { + ) -> Result<(CommandSys, Option), ShellError> { let mut process = if let Some(d) = self.env_vars.get("PWD") { let mut process = if use_cmd { self.spawn_cmd_command(d) @@ -583,13 +614,22 @@ impl ExternalCommand { // If the external is not the last command, its output will get piped // either as a string or binary - if self.redirect_stdout { - process.stdout(Stdio::piped()); - } + let reader = if self.redirect_combine { + let (reader, writer) = os_pipe::pipe()?; + let writer_clone = writer.try_clone()?; + process.stdout(writer); + process.stderr(writer_clone); + Some(reader) + } else { + if self.redirect_stdout { + process.stdout(Stdio::piped()); + } - if self.redirect_stderr { - process.stderr(Stdio::piped()); - } + if self.redirect_stderr { + process.stderr(Stdio::piped()); + } + None + }; // 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 @@ -597,7 +637,7 @@ impl ExternalCommand { process.stdin(Stdio::piped()); } - Ok(process) + Ok((process, reader)) } fn create_command(&self, cwd: &str) -> Result { diff --git a/crates/nu-command/tests/commands/run_external.rs b/crates/nu-command/tests/commands/run_external.rs index 1a7e5b6a12..c3642670b9 100644 --- a/crates/nu-command/tests/commands/run_external.rs +++ b/crates/nu-command/tests/commands/run_external.rs @@ -320,3 +320,18 @@ fn quotes_trimmed_when_shelling_out() { assert_eq!(actual.out, "foo"); } + +#[test] +fn redirect_combine() { + Playground::setup("redirect_combine", |dirs, _| { + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + run-external --redirect-combine sh [-c 'echo Foo; echo >&2 Bar'] + "# + )); + + // Lines are collapsed in the nu! macro + assert_eq!(actual.out, "FooBar"); + }); +}