extern/nushell
1
0
Fork 1
mirror of https://github.com/nushell/nushell.git synced 2025-08-19 22:08:41 +02:00
Code Issues Packages Projects Releases Wiki Activity

Error on non-zero exit statuses (#13515)

Browse Source 
# Description
This PR makes it so that non-zero exit codes and termination by signal
are treated as a normal `ShellError`. Currently, these are silent
errors. That is, if an external command fails, then it's code block is
aborted, but the parent block can sometimes continue execution. E.g.,
see #8569 and this example:
```nushell
[1 2] | each { ^false }
```

Before this would give:
```
╭───┬──╮
│ 0 │  │
│ 1 │  │
╰───┴──╯
```

Now, this shows an error:
```
Error: nu:🐚:eval_block_with_input

  × Eval block failed with pipeline input
   ╭─[entry #1:1:2]
 1 │ [1 2] | each { ^false }
   ·  ┬
   ·  ╰── source value
   ╰────

Error: nu:🐚:non_zero_exit_code

  × External command had a non-zero exit code
   ╭─[entry #1:1:17]
 1 │ [1 2] | each { ^false }
   ·                 ──┬──
   ·                   ╰── exited with code 1
   ╰────
```

This PR fixes #12874, fixes #5960, fixes #10856, and fixes #5347. This
PR also partially addresses #10633 and #10624 (only the last command of
a pipeline is currently checked). It looks like #8569 is already fixed,
but this PR will make sure it is definitely fixed (fixes #8569).

# User-Facing Changes
- Non-zero exit codes and termination by signal now cause an error to be
thrown.
- The error record value passed to a `catch` block may now have an
`exit_code` column containing the integer exit code if the error was due
to an external command.
- Adds new config values, `display_errors.exit_code` and
`display_errors.termination_signal`, which determine whether an error
message should be printed in the respective error cases. For
non-interactive sessions, these are set to `true`, and for interactive
sessions `display_errors.exit_code` is false (via the default config).

# Tests
Added a few tests.

# After Submitting
- Update docs and book.
- Future work:
- Error if other external commands besides the last in a pipeline exit
with a non-zero exit code. Then, deprecate `do -c` since this will be
the default behavior everywhere.
- Add a better mechanism for exit codes and deprecate
`$env.LAST_EXIT_CODE` (it's buggy).
This commit is contained in:
Ian Manske
2024-09-06 23:44:26 -07:00
committed by GitHub
parent 6c1c7f9509
commit 3d008e2c4e
54 changed files with 566 additions and 650 deletions
Download Patch File Download Diff File Expand all files Collapse all files

54
crates/nu-protocol/src/pipeline/byte_stream.rs
View File

@@ -1,6 +1,6 @@
//! Module managing the streaming of raw bytes between pipeline elements
use crate::{
process::{ChildPipe, ChildProcess, ExitStatus},
process::{ChildPipe, ChildProcess},
ErrSpan, IntoSpanned, OutDest, PipelineData, ShellError, Signals, Span, Type, Value,
};
use serde::{Deserialize, Serialize};
@@ -548,25 +548,19 @@ impl ByteStream {
}
/// Consume and drop all bytes of the [`ByteStream`].
///
/// If the source of the [`ByteStream`] is [`ByteStreamSource::Child`],
/// then the [`ExitStatus`] of the [`ChildProcess`] is returned.
pub fn drain(self) -> Result<Option<ExitStatus>, ShellError> {
pub fn drain(self) -> Result<(), ShellError> {
match self.stream {
ByteStreamSource::Read(read) => {
copy_with_signals(read, io::sink(), self.span, &self.signals)?;
Ok(None)
Ok(())
}
ByteStreamSource::File(_) => Ok(None),
ByteStreamSource::Child(child) => Ok(Some(child.wait()?)),
ByteStreamSource::File(_) => Ok(()),
ByteStreamSource::Child(child) => child.wait(),
}
}
/// Print all bytes of the [`ByteStream`] to stdout or stderr.
///
/// If the source of the [`ByteStream`] is [`ByteStreamSource::Child`],
/// then the [`ExitStatus`] of the [`ChildProcess`] is returned.
pub fn print(self, to_stderr: bool) -> Result<Option<ExitStatus>, ShellError> {
pub fn print(self, to_stderr: bool) -> Result<(), ShellError> {
if to_stderr {
self.write_to(&mut io::stderr())
} else {
@@ -575,20 +569,15 @@ impl ByteStream {
}
/// Write all bytes of the [`ByteStream`] to `dest`.
///
/// If the source of the [`ByteStream`] is [`ByteStreamSource::Child`],
/// then the [`ExitStatus`] of the [`ChildProcess`] is returned.
pub fn write_to(self, dest: impl Write) -> Result<Option<ExitStatus>, ShellError> {
pub fn write_to(self, dest: impl Write) -> Result<(), ShellError> {
let span = self.span;
let signals = &self.signals;
match self.stream {
ByteStreamSource::Read(read) => {
copy_with_signals(read, dest, span, signals)?;
Ok(None)
}
ByteStreamSource::File(file) => {
copy_with_signals(file, dest, span, signals)?;
Ok(None)
}
ByteStreamSource::Child(mut child) => {
// All `OutDest`s except `OutDest::Capture` will cause `stderr` to be `None`.
@@ -606,36 +595,33 @@ impl ByteStream {
}
}
}
Ok(Some(child.wait()?))
child.wait()?;
}
}
Ok(())
}
pub(crate) fn write_to_out_dests(
self,
stdout: &OutDest,
stderr: &OutDest,
) -> Result<Option<ExitStatus>, ShellError> {
) -> Result<(), ShellError> {
let span = self.span;
let signals = &self.signals;
match self.stream {
ByteStreamSource::Read(read) => {
write_to_out_dest(read, stdout, true, span, signals)?;
Ok(None)
}
ByteStreamSource::File(file) => {
match stdout {
OutDest::Pipe | OutDest::Capture | OutDest::Null => {}
OutDest::Inherit => {
copy_with_signals(file, io::stdout(), span, signals)?;
}
OutDest::File(f) => {
copy_with_signals(file, f.as_ref(), span, signals)?;
}
ByteStreamSource::File(file) => match stdout {
OutDest::Pipe | OutDest::Capture | OutDest::Null => {}
OutDest::Inherit => {
copy_with_signals(file, io::stdout(), span, signals)?;
}
Ok(None)
}
OutDest::File(f) => {
copy_with_signals(file, f.as_ref(), span, signals)?;
}
},
ByteStreamSource::Child(mut child) => {
match (child.stdout.take(), child.stderr.take()) {
(Some(out), Some(err)) => {
@@ -682,9 +668,11 @@ impl ByteStream {
}
(None, None) => {}
}
Ok(Some(child.wait()?))
child.wait()?;
}
}
Ok(())
}
}

113
crates/nu-protocol/src/pipeline/pipeline_data.rs
View File

@@ -1,12 +1,11 @@
use crate::{
ast::{Call, PathMember},
engine::{EngineState, Stack},
process::{ChildPipe, ChildProcess, ExitStatus},
ByteStream, ByteStreamType, Config, ErrSpan, ListStream, OutDest, PipelineMetadata, Range,
ShellError, Signals, Span, Type, Value,
ByteStream, ByteStreamType, Config, ListStream, OutDest, PipelineMetadata, Range, ShellError,
Signals, Span, Type, Value,
};
use nu_utils::{stderr_write_all_and_flush, stdout_write_all_and_flush};
use std::io::{Cursor, Read, Write};
use std::io::Write;
const LINE_ENDING_PATTERN: &[char] = &['\r', '\n'];
@@ -52,16 +51,6 @@ impl PipelineData {
PipelineData::Empty
}
/// create a `PipelineData::ByteStream` with proper exit_code
///
/// It's useful to break running without raising error at user level.
pub fn new_external_stream_with_only_exit_code(exit_code: i32) -> PipelineData {
let span = Span::unknown();
let mut child = ChildProcess::from_raw(None, None, None, span);
child.set_exit_code(exit_code);
PipelineData::ByteStream(ByteStream::child(child, span), None)
}
pub fn metadata(&self) -> Option<PipelineMetadata> {
match self {
PipelineData::Empty => None,
@@ -182,22 +171,17 @@ impl PipelineData {
/// without consuming input and without writing anything.
///
/// For the other [`OutDest`]s, the given `PipelineData` will be completely consumed
/// and `PipelineData::Empty` will be returned, unless the data is from an external stream,
/// in which case an external stream containing only that exit code will be returned.
/// and `PipelineData::Empty` will be returned (assuming no errors).
pub fn write_to_out_dests(
self,
engine_state: &EngineState,
stack: &mut Stack,
) -> Result<PipelineData, ShellError> {
match (self, stack.stdout()) {
(PipelineData::ByteStream(stream, ..), stdout) => {
if let Some(exit_status) = stream.write_to_out_dests(stdout, stack.stderr())? {
return Ok(PipelineData::new_external_stream_with_only_exit_code(
exit_status.code(),
));
}
}
(data, OutDest::Pipe | OutDest::Capture) => return Ok(data),
(PipelineData::ByteStream(stream, ..), stdout) => {
stream.write_to_out_dests(stdout, stack.stderr())?;
}
(PipelineData::Empty, ..) => {}
(PipelineData::Value(..), OutDest::Null) => {}
(PipelineData::ListStream(stream, ..), OutDest::Null) => {
@@ -227,15 +211,12 @@ impl PipelineData {
Ok(PipelineData::Empty)
}
pub fn drain(self) -> Result<Option<ExitStatus>, ShellError> {
pub fn drain(self) -> Result<(), ShellError> {
match self {
PipelineData::Empty => Ok(None),
PipelineData::Empty => Ok(()),
PipelineData::Value(Value::Error { error, .. }, ..) => Err(*error),
PipelineData::Value(..) => Ok(None),
PipelineData::ListStream(stream, ..) => {
stream.drain()?;
Ok(None)
}
PipelineData::Value(..) => Ok(()),
PipelineData::ListStream(stream, ..) => stream.drain(),
PipelineData::ByteStream(stream, ..) => stream.drain(),
}
}
@@ -496,68 +477,6 @@ impl PipelineData {
}
}
/// Try to catch the external command exit status and detect if it failed.
///
/// This is useful for external commands with semicolon, we can detect errors early to avoid
/// commands after the semicolon running.
///
/// Returns `self` and a flag that indicates if the external command run failed. If `self` is
/// not [`PipelineData::ByteStream`], the flag will be `false`.
///
/// Currently this will consume an external command to completion.
pub fn check_external_failed(self) -> Result<(Self, bool), ShellError> {
if let PipelineData::ByteStream(stream, metadata) = self {
// Preserve stream attributes
let span = stream.span();
let type_ = stream.type_();
let known_size = stream.known_size();
match stream.into_child() {
Ok(mut child) => {
// Only check children without stdout. This means that nothing
// later in the pipeline can possibly consume output from this external command.
if child.stdout.is_none() {
// Note:
// In run-external's implementation detail, the result sender thread
// send out stderr message first, then stdout message, then exit_code.
//
// In this clause, we already make sure that `stdout` is None
// But not the case of `stderr`, so if `stderr` is not None
// We need to consume stderr message before reading external commands' exit code.
//
// Or we'll never have a chance to read exit_code if stderr producer produce too much stderr message.
// So we consume stderr stream and rebuild it.
let stderr = child
.stderr
.take()
.map(|mut stderr| {
let mut buf = Vec::new();
stderr.read_to_end(&mut buf).err_span(span)?;
Ok::<_, ShellError>(buf)
})
.transpose()?;
let code = child.wait()?.code();
let mut child = ChildProcess::from_raw(None, None, None, span);
if let Some(stderr) = stderr {
child.stderr = Some(ChildPipe::Tee(Box::new(Cursor::new(stderr))));
}
child.set_exit_code(code);
let stream = ByteStream::child(child, span).with_type(type_);
Ok((PipelineData::ByteStream(stream, metadata), code != 0))
} else {
let stream = ByteStream::child(child, span)
.with_type(type_)
.with_known_size(known_size);
Ok((PipelineData::ByteStream(stream, metadata), false))
}
}
Err(stream) => Ok((PipelineData::ByteStream(stream, metadata), false)),
}
} else {
Ok((self, false))
}
}
/// Try to convert Value from Value::Range to Value::List.
/// This is useful to expand Value::Range into array notation, specifically when
/// converting `to json` or `to nuon`.
@@ -613,7 +532,7 @@ impl PipelineData {
stack: &mut Stack,
no_newline: bool,
to_stderr: bool,
) -> Result<Option<ExitStatus>, ShellError> {
) -> Result<(), ShellError> {
match self {
// Print byte streams directly as long as they aren't binary.
PipelineData::ByteStream(stream, ..) if stream.type_() != ByteStreamType::Binary => {
@@ -650,14 +569,14 @@ impl PipelineData {
engine_state: &EngineState,
no_newline: bool,
to_stderr: bool,
) -> Result<Option<ExitStatus>, ShellError> {
) -> Result<(), ShellError> {
if let PipelineData::Value(Value::Binary { val: bytes, .. }, _) = self {
if to_stderr {
stderr_write_all_and_flush(bytes)?;
} else {
stdout_write_all_and_flush(bytes)?;
}
Ok(None)
Ok(())
} else {
self.write_all_and_flush(engine_state, no_newline, to_stderr)
}
@@ -668,7 +587,7 @@ impl PipelineData {
engine_state: &EngineState,
no_newline: bool,
to_stderr: bool,
) -> Result<Option<ExitStatus>, ShellError> {
) -> Result<(), ShellError> {
if let PipelineData::ByteStream(stream, ..) = self {
// Copy ByteStreams directly
stream.print(to_stderr)
@@ -692,7 +611,7 @@ impl PipelineData {
}
}
Ok(None)
Ok(())
}
}