From 50343f2d6ae78a7f5460f343c9795b21931d31b5 Mon Sep 17 00:00:00 2001 From: Jonathan Turner Date: Fri, 7 Aug 2020 16:53:37 +1200 Subject: [PATCH] Add stderr back when using `do -i` (#2309) * Add stderr back when using do -i * Add stderr back when using do -i --- crates/nu-cli/src/commands/autoview.rs | 4 +- .../src/commands/classified/external.rs | 130 ++++++++++++++++-- .../src/commands/classified/internal.rs | 4 +- crates/nu-cli/src/commands/do_.rs | 30 +++- crates/nu-cli/src/commands/enter.rs | 3 +- crates/nu-cli/src/commands/run_alias.rs | 2 +- crates/nu-cli/src/commands/run_external.rs | 15 +- crates/nu-cli/src/commands/save.rs | 7 +- crates/nu-cli/src/evaluate/evaluator.rs | 6 +- crates/nu-parser/src/parse.rs | 30 +++- crates/nu-protocol/src/hir.rs | 30 ++-- 11 files changed, 204 insertions(+), 57 deletions(-) diff --git a/crates/nu-cli/src/commands/autoview.rs b/crates/nu-cli/src/commands/autoview.rs index 07255d2d5e..a5087313d0 100644 --- a/crates/nu-cli/src/commands/autoview.rs +++ b/crates/nu-cli/src/commands/autoview.rs @@ -3,7 +3,7 @@ use crate::commands::WholeStreamCommand; use crate::data::value::format_leaf; use crate::prelude::*; use nu_errors::ShellError; -use nu_protocol::{hir, hir::Expression, hir::Literal, hir::SpannedExpression}; +use nu_protocol::hir::{self, Expression, ExternalRedirection, Literal, SpannedExpression}; use nu_protocol::{Primitive, Scope, Signature, UntaggedValue, Value}; use parking_lot::Mutex; use std::sync::atomic::AtomicBool; @@ -328,7 +328,7 @@ fn create_default_command_args(context: &RunnableContextWithoutInput) -> RawComm positional: None, named: None, span, - is_last: true, + external_redirection: ExternalRedirection::Stdout, }, name_tag: context.name.clone(), scope: Scope::new(), diff --git a/crates/nu-cli/src/commands/classified/external.rs b/crates/nu-cli/src/commands/classified/external.rs index 32fbeb4155..a9589c959a 100644 --- a/crates/nu-cli/src/commands/classified/external.rs +++ b/crates/nu-cli/src/commands/classified/external.rs @@ -13,7 +13,7 @@ use futures_codec::FramedRead; use log::trace; use nu_errors::ShellError; -use nu_protocol::hir::ExternalCommand; +use nu_protocol::hir::{ExternalCommand, ExternalRedirection}; use nu_protocol::{Primitive, Scope, ShellTypeName, UntaggedValue, Value}; use nu_source::Tag; @@ -22,7 +22,7 @@ pub(crate) async fn run_external_command( context: &mut Context, input: InputStream, scope: &Scope, - is_last: bool, + external_redirection: ExternalRedirection, ) -> Result { trace!(target: "nu::run::external", "-> {}", command.name); @@ -34,7 +34,7 @@ pub(crate) async fn run_external_command( )); } - run_with_stdin(command, context, input, scope, is_last).await + run_with_stdin(command, context, input, scope, external_redirection).await } async fn run_with_stdin( @@ -42,7 +42,7 @@ async fn run_with_stdin( context: &mut Context, input: InputStream, scope: &Scope, - is_last: bool, + external_redirection: ExternalRedirection, ) -> Result { let path = context.shell_manager.path(); @@ -122,7 +122,14 @@ async fn run_with_stdin( }) .collect::>(); - spawn(&command, &path, &process_args[..], input, is_last, scope) + spawn( + &command, + &path, + &process_args[..], + input, + external_redirection, + scope, + ) } fn spawn( @@ -130,7 +137,7 @@ fn spawn( path: &str, args: &[String], input: InputStream, - is_last: bool, + external_redirection: ExternalRedirection, scope: &Scope, ) -> Result { let command = command.clone(); @@ -166,9 +173,22 @@ fn spawn( // We want stdout regardless of what // we are doing ($it case or pipe stdin) - if !is_last { - process.stdout(Stdio::piped()); - trace!(target: "nu::run::external", "set up stdout pipe"); + match external_redirection { + ExternalRedirection::Stdout => { + process.stdout(Stdio::piped()); + trace!(target: "nu::run::external", "set up stdout pipe"); + } + ExternalRedirection::Stderr => { + process.stderr(Stdio::piped()); + trace!(target: "nu::run::external", "set up stderr pipe"); + } + ExternalRedirection::StdoutAndStderr => { + process.stdout(Stdio::piped()); + trace!(target: "nu::run::external", "set up stdout pipe"); + process.stderr(Stdio::piped()); + trace!(target: "nu::run::external", "set up stderr pipe"); + } + _ => {} } // open since we have some contents for stdin @@ -252,7 +272,9 @@ fn spawn( }); std::thread::spawn(move || { - if !is_last { + if external_redirection == ExternalRedirection::Stdout + || external_redirection == ExternalRedirection::StdoutAndStderr + { let stdout = if let Some(stdout) = child.stdout.take() { stdout } else { @@ -321,6 +343,79 @@ fn spawn( } } } + if external_redirection == ExternalRedirection::Stderr + || external_redirection == ExternalRedirection::StdoutAndStderr + { + let stderr = if let Some(stderr) = child.stderr.take() { + stderr + } else { + let _ = stdout_read_tx.send(Ok(Value { + value: UntaggedValue::Error(ShellError::labeled_error( + "Can't redirect the stderr for external command", + "can't redirect stderr", + &stdout_name_tag, + )), + tag: stdout_name_tag, + })); + return Err(()); + }; + + let file = futures::io::AllowStdIo::new(stderr); + let stream = FramedRead::new(file, MaybeTextCodec::default()); + + for line in block_on_stream(stream) { + match line { + Ok(line) => match line { + StringOrBinary::String(s) => { + let result = stdout_read_tx.send(Ok(Value { + value: UntaggedValue::Error( + ShellError::untagged_runtime_error(s), + ), + tag: stdout_name_tag.clone(), + })); + + if result.is_err() { + break; + } + } + StringOrBinary::Binary(_) => { + let result = stdout_read_tx.send(Ok(Value { + value: UntaggedValue::Error( + ShellError::untagged_runtime_error(""), + ), + tag: stdout_name_tag.clone(), + })); + + if result.is_err() { + break; + } + } + }, + Err(e) => { + // If there's an exit status, it makes sense that we may error when + // trying to read from its stdout pipe (likely been closed). In that + // case, don't emit an error. + let should_error = match child.wait() { + Ok(exit_status) => !exit_status.success(), + Err(_) => true, + }; + + if should_error { + let _ = stdout_read_tx.send(Ok(Value { + value: UntaggedValue::Error(ShellError::labeled_error( + format!("Unable to read from stdout ({})", e), + "unable to read from stdout", + &stdout_name_tag, + )), + tag: stdout_name_tag.clone(), + })); + } + + return Ok(()); + } + } + } + } // We can give an error when we see a non-zero exit code, but this is different // than what other shells will do. @@ -474,16 +569,21 @@ mod tests { #[cfg(feature = "which")] async fn non_existent_run() -> Result<(), ShellError> { + use nu_protocol::hir::ExternalRedirection; let cmd = ExternalBuilder::for_name("i_dont_exist.exe").build(); let input = InputStream::empty(); let mut ctx = Context::basic().expect("There was a problem creating a basic context."); - assert!( - run_external_command(cmd, &mut ctx, input, &Scope::new(), false) - .await - .is_err() - ); + assert!(run_external_command( + cmd, + &mut ctx, + input, + &Scope::new(), + ExternalRedirection::Stdout + ) + .await + .is_err()); Ok(()) } diff --git a/crates/nu-cli/src/commands/classified/internal.rs b/crates/nu-cli/src/commands/classified/internal.rs index 52ecb167c3..bb243fb7a7 100644 --- a/crates/nu-cli/src/commands/classified/internal.rs +++ b/crates/nu-cli/src/commands/classified/internal.rs @@ -4,7 +4,7 @@ use crate::commands::UnevaluatedCallInfo; use crate::prelude::*; use log::{log_enabled, trace}; use nu_errors::ShellError; -use nu_protocol::hir::InternalCommand; +use nu_protocol::hir::{ExternalRedirection, InternalCommand}; use nu_protocol::{CommandAction, Primitive, ReturnSuccess, Scope, UntaggedValue, Value}; pub(crate) async fn run_internal_command( @@ -87,7 +87,7 @@ pub(crate) async fn run_internal_command( positional: None, named: None, span: Span::unknown(), - is_last: false, + external_redirection: ExternalRedirection::Stdout, }, name_tag: Tag::unknown_anchor(command.name_span), scope: (&*scope).clone(), diff --git a/crates/nu-cli/src/commands/do_.rs b/crates/nu-cli/src/commands/do_.rs index e6a9754f55..4ddb0368c9 100644 --- a/crates/nu-cli/src/commands/do_.rs +++ b/crates/nu-cli/src/commands/do_.rs @@ -2,7 +2,9 @@ use crate::commands::classified::block::run_block; use crate::commands::WholeStreamCommand; use crate::prelude::*; use nu_errors::ShellError; -use nu_protocol::{hir::Block, ReturnSuccess, Signature, SyntaxShape, Value}; +use nu_protocol::{ + hir::Block, hir::ExternalRedirection, ReturnSuccess, Signature, SyntaxShape, Value, +}; pub struct Do; @@ -61,7 +63,7 @@ async fn do_( registry: &CommandRegistry, ) -> Result { let registry = registry.clone(); - let is_last = raw_args.call_info.args.is_last; + let external_redirection = raw_args.call_info.args.external_redirection; let mut context = Context::from_raw(&raw_args, ®istry); let scope = raw_args.call_info.scope.clone(); @@ -72,7 +74,26 @@ async fn do_( }, input, ) = raw_args.process(®istry).await?; - block.set_is_last(is_last); + + let block_redirection = match external_redirection { + ExternalRedirection::None => { + if ignore_errors { + ExternalRedirection::Stderr + } else { + ExternalRedirection::None + } + } + ExternalRedirection::Stdout => { + if ignore_errors { + ExternalRedirection::StdoutAndStderr + } else { + ExternalRedirection::Stdout + } + } + x => x, + }; + + block.set_redirect(block_redirection); let result = run_block( &block, @@ -85,6 +106,9 @@ async fn do_( .await; if ignore_errors { + // To properly ignore errors we need to redirect stderr, consume it, and remove + // any errors we see in the process. + match result { Ok(mut stream) => { let output = stream.drain_vec().await; diff --git a/crates/nu-cli/src/commands/enter.rs b/crates/nu-cli/src/commands/enter.rs index 28142fc8a5..69df7dd91c 100644 --- a/crates/nu-cli/src/commands/enter.rs +++ b/crates/nu-cli/src/commands/enter.rs @@ -3,6 +3,7 @@ use crate::commands::WholeStreamCommand; use crate::context::CommandRegistry; use crate::prelude::*; use nu_errors::ShellError; +use nu_protocol::hir::ExternalRedirection; use nu_protocol::{ CommandAction, Primitive, ReturnSuccess, Signature, SyntaxShape, UntaggedValue, Value, }; @@ -145,7 +146,7 @@ async fn enter( positional: None, named: None, span: Span::unknown(), - is_last: false, + external_redirection: ExternalRedirection::Stdout, }, name_tag: tag.clone(), scope: scope.clone(), diff --git a/crates/nu-cli/src/commands/run_alias.rs b/crates/nu-cli/src/commands/run_alias.rs index 6b045f8cf4..74915e0e37 100644 --- a/crates/nu-cli/src/commands/run_alias.rs +++ b/crates/nu-cli/src/commands/run_alias.rs @@ -41,7 +41,7 @@ impl WholeStreamCommand for AliasCommand { let call_info = args.call_info.clone(); let registry = registry.clone(); let mut block = self.block.clone(); - block.set_is_last(call_info.args.is_last); + block.set_redirect(call_info.args.external_redirection); let alias_command = self.clone(); let mut context = Context::from_args(&args, ®istry); diff --git a/crates/nu-cli/src/commands/run_external.rs b/crates/nu-cli/src/commands/run_external.rs index 7f46c8cb27..24e59845d7 100644 --- a/crates/nu-cli/src/commands/run_external.rs +++ b/crates/nu-cli/src/commands/run_external.rs @@ -62,6 +62,8 @@ impl WholeStreamCommand for RunExternalCommand { let mut positionals = positionals.into_iter(); + let external_redirection = args.call_info.args.external_redirection; + let name = positionals .next() .ok_or_else(|| { @@ -130,11 +132,16 @@ impl WholeStreamCommand for RunExternalCommand { } let scope = args.call_info.scope.clone(); - let is_last = args.call_info.args.is_last; + let input = args.input; - let result = - external::run_external_command(command, &mut external_context, input, &scope, is_last) - .await; + let result = external::run_external_command( + command, + &mut external_context, + input, + &scope, + external_redirection, + ) + .await; Ok(result?.to_output_stream()) } diff --git a/crates/nu-cli/src/commands/save.rs b/crates/nu-cli/src/commands/save.rs index 2d77977a3a..864d3f4627 100644 --- a/crates/nu-cli/src/commands/save.rs +++ b/crates/nu-cli/src/commands/save.rs @@ -1,7 +1,10 @@ use crate::commands::{UnevaluatedCallInfo, WholeStreamCommand}; use crate::prelude::*; use nu_errors::ShellError; -use nu_protocol::{Primitive, ReturnSuccess, Signature, SyntaxShape, UntaggedValue, Value}; +use nu_protocol::{ + hir::ExternalRedirection, Primitive, ReturnSuccess, Signature, SyntaxShape, UntaggedValue, + Value, +}; use nu_source::Tagged; use std::path::{Path, PathBuf}; @@ -226,7 +229,7 @@ async fn save( positional: None, named: None, span: Span::unknown(), - is_last: false, + external_redirection: ExternalRedirection::Stdout, }, name_tag: name_tag.clone(), scope, diff --git a/crates/nu-cli/src/evaluate/evaluator.rs b/crates/nu-cli/src/evaluate/evaluator.rs index 1e3cd77608..3641c7c9b8 100644 --- a/crates/nu-cli/src/evaluate/evaluator.rs +++ b/crates/nu-cli/src/evaluate/evaluator.rs @@ -5,7 +5,7 @@ use crate::prelude::*; use async_recursion::async_recursion; use log::trace; use nu_errors::{ArgumentError, ShellError}; -use nu_protocol::hir::{self, Expression, SpannedExpression}; +use nu_protocol::hir::{self, Expression, ExternalRedirection, SpannedExpression}; use nu_protocol::{ ColumnPath, Primitive, RangeInclusion, UnspannedPathMember, UntaggedValue, Value, }; @@ -194,10 +194,10 @@ async fn evaluate_invocation( let mut context = Context::basic()?; context.registry = registry.clone(); - let input = InputStream::empty(); + let input = InputStream::one(it.clone()); let mut block = block.clone(); - block.set_is_last(false); + block.set_redirect(ExternalRedirection::Stdout); let result = run_block(&block, &mut context, input, it, vars, env).await?; diff --git a/crates/nu-parser/src/parse.rs b/crates/nu-parser/src/parse.rs index 620f228ea2..348dd3017f 100644 --- a/crates/nu-parser/src/parse.rs +++ b/crates/nu-parser/src/parse.rs @@ -4,8 +4,8 @@ use log::trace; use nu_errors::{ArgumentError, ParseError}; use nu_protocol::hir::{ self, Binary, Block, ClassifiedBlock, ClassifiedCommand, ClassifiedPipeline, Commands, - Expression, Flag, FlagKind, InternalCommand, Member, NamedArguments, Operator, - SpannedExpression, Unit, + Expression, ExternalRedirection, Flag, FlagKind, InternalCommand, Member, NamedArguments, + Operator, SpannedExpression, Unit, }; use nu_protocol::{NamedType, PositionalType, Signature, SyntaxShape, UnspannedPathMember}; use nu_source::{Span, Spanned, SpannedItem}; @@ -498,7 +498,7 @@ fn parse_interpolated_string( expr: Expression::Synthetic(hir::Synthetic::String("build-string".to_owned())), span: lite_arg.span, }), - is_last: false, + external_redirection: ExternalRedirection::Stdout, named: None, positional: Some(output), span: lite_arg.span, @@ -1359,7 +1359,11 @@ fn classify_pipeline( positional: Some(args), named: None, span: Span::unknown(), - is_last: iter.peek().is_none(), + external_redirection: if iter.peek().is_none() { + ExternalRedirection::None + } else { + ExternalRedirection::Stdout + }, }, })) } else if lite_cmd.name.item == "=" { @@ -1387,7 +1391,11 @@ fn classify_pipeline( parse_internal_command(&lite_cmd, registry, &signature, 1); error = error.or(err); - internal_command.args.is_last = iter.peek().is_none(); + internal_command.args.external_redirection = if iter.peek().is_none() { + ExternalRedirection::None + } else { + ExternalRedirection::Stdout + }; commands.push(ClassifiedCommand::Internal(internal_command)); continue; } @@ -1399,7 +1407,11 @@ fn classify_pipeline( parse_internal_command(&lite_cmd, registry, &signature, 0); error = error.or(err); - internal_command.args.is_last = iter.peek().is_none(); + internal_command.args.external_redirection = if iter.peek().is_none() { + ExternalRedirection::None + } else { + ExternalRedirection::Stdout + }; commands.push(ClassifiedCommand::Internal(internal_command)); continue; } @@ -1437,7 +1449,11 @@ fn classify_pipeline( positional: Some(args), named: None, span: Span::unknown(), - is_last: iter.peek().is_none(), + external_redirection: if iter.peek().is_none() { + ExternalRedirection::None + } else { + ExternalRedirection::Stdout + }, }, })) } diff --git a/crates/nu-protocol/src/hir.rs b/crates/nu-protocol/src/hir.rs index 8405aa1484..543801a09e 100644 --- a/crates/nu-protocol/src/hir.rs +++ b/crates/nu-protocol/src/hir.rs @@ -167,7 +167,7 @@ impl Commands { }), span: self.span, }]), - is_last: false, // FIXME + external_redirection: ExternalRedirection::Stdout, // FIXME }, }) } @@ -200,27 +200,15 @@ impl Block { } } - pub fn set_is_last(&mut self, is_last: bool) { + pub fn set_redirect(&mut self, external_redirection: ExternalRedirection) { if let Some(pipeline) = self.block.last_mut() { if let Some(command) = pipeline.list.last_mut() { if let ClassifiedCommand::Internal(internal) = command { - internal.args.is_last = is_last; + internal.args.external_redirection = external_redirection; } } } } - - pub fn get_is_last(&mut self) -> Option { - if let Some(pipeline) = self.block.last_mut() { - if let Some(command) = pipeline.list.last_mut() { - if let ClassifiedCommand::Internal(internal) = command { - return Some(internal.args.is_last); - } - } - } - - None - } } #[derive(Debug, Ord, PartialOrd, Eq, PartialEq, Clone, Hash, Deserialize, Serialize)] @@ -1114,13 +1102,21 @@ impl PrettyDebugWithSource for NamedValue { } } +#[derive(Debug, Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Hash, Serialize, Deserialize)] +pub enum ExternalRedirection { + None, + Stdout, + Stderr, + StdoutAndStderr, +} + #[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash, Serialize, Deserialize)] pub struct Call { pub head: Box, pub positional: Option>, pub named: Option, pub span: Span, - pub is_last: bool, + pub external_redirection: ExternalRedirection, } impl Call { @@ -1193,7 +1189,7 @@ impl Call { positional: None, named: None, span, - is_last: false, + external_redirection: ExternalRedirection::Stdout, } } }