Skip to content

Avoid downcasting to ProcessError #10163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions crates/cargo-test-support/src/cross_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub fn disabled() -> bool {

let cross_target = alternate();

let run_cross_test = || -> anyhow::Result<Output> {
let run_cross_test = || -> Result<Output, ProcessError> {
let p = project()
.at("cross_test")
.file("Cargo.toml", &basic_manifest("cross_test", "1.0.0"))
Expand Down Expand Up @@ -170,10 +170,7 @@ rustup does not appear to be installed. Make sure that the appropriate
// Show the actual error message.
match run_cross_test() {
Ok(_) => message.push_str("\nUh oh, second run succeeded?\n"),
Err(err) => match err.downcast_ref::<ProcessError>() {
Some(proc_err) => write!(message, "\nTest error: {}\n", proc_err).unwrap(),
None => write!(message, "\nUnexpected non-process error: {}\n", err).unwrap(),
},
Err(proc_err) => write!(message, "\nTest error: {}\n", proc_err).unwrap(),
}

panic!("{}", message);
Expand Down
35 changes: 17 additions & 18 deletions crates/cargo-test-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,7 @@ impl Execs {
self
}

pub fn exec_with_output(&mut self) -> Result<Output> {
pub fn exec_with_output(&mut self) -> Result<Output, ProcessError> {
self.ran = true;
// TODO avoid unwrap
let p = (&self.process_builder).clone().unwrap();
Expand Down Expand Up @@ -879,28 +879,27 @@ impl Execs {
match res {
Ok(out) => {
self.match_output(out.status.code(), &out.stdout, &out.stderr)?;
return Ok(RawOutput {
Ok(RawOutput {
stdout: out.stdout,
stderr: out.stderr,
code: out.status.code(),
});
})
}
Err(e) => {
if let Some(ProcessError {
stdout: Some(stdout),
stderr: Some(stderr),
Err(ProcessError {
stdout: Some(stdout),
stderr: Some(stderr),
code,
..
}) => {
self.match_output(code, &stdout, &stderr)?;
Ok(RawOutput {
stdout: stdout.to_vec(),
stderr: stderr.to_vec(),
code,
..
}) = e.downcast_ref::<ProcessError>()
{
self.match_output(*code, stdout, stderr)?;
return Ok(RawOutput {
stdout: stdout.to_vec(),
stderr: stderr.to_vec(),
code: *code,
});
}
bail!("could not exec process {}: {:?}", process, e)
})
}
Err(e) => {
bail!("could not exec process {}: {:?}", process, e);
}
}
}
Expand Down
53 changes: 30 additions & 23 deletions crates/cargo-util/src/process_builder.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::process_error::ProcessError;
use crate::read2;
use anyhow::{bail, Context, Result};
use anyhow::Result;
use jobserver::Client;
use shell_escape::escape;
use std::collections::BTreeMap;
Expand Down Expand Up @@ -162,10 +162,11 @@ impl ProcessBuilder {
}

/// Runs the process, waiting for completion, and mapping non-success exit codes to an error.
pub fn exec(&self) -> Result<()> {
pub fn exec(&self) -> Result<(), ProcessError> {
let mut command = self.build_command();
let exit = command.status().with_context(|| {
let exit = command.status().map_err(|err| {
ProcessError::new(&format!("could not execute process {}", self), None, None)
.with_source(err)
})?;

if exit.success() {
Expand All @@ -175,8 +176,7 @@ impl ProcessBuilder {
&format!("process didn't exit successfully: {}", self),
Some(exit),
None,
)
.into())
))
}
}

Expand All @@ -195,16 +195,17 @@ impl ProcessBuilder {
/// include our child process. If the child terminates then we'll reap them in Cargo
/// pretty quickly, and if the child handles the signal then we won't terminate
/// (and we shouldn't!) until the process itself later exits.
pub fn exec_replace(&self) -> Result<()> {
pub fn exec_replace(&self) -> Result<(), ProcessError> {
imp::exec_replace(self)
}

/// Executes the process, returning the stdio output, or an error if non-zero exit status.
pub fn exec_with_output(&self) -> Result<Output> {
pub fn exec_with_output(&self) -> Result<Output, ProcessError> {
let mut command = self.build_command();

let output = command.output().with_context(|| {
let output = command.output().map_err(|e| {
ProcessError::new(&format!("could not execute process {}", self), None, None)
.with_source(e)
})?;

if output.status.success() {
Expand All @@ -214,8 +215,7 @@ impl ProcessBuilder {
&format!("process didn't exit successfully: {}", self),
Some(output.status),
Some(&output),
)
.into())
))
}
}

Expand All @@ -233,7 +233,7 @@ impl ProcessBuilder {
on_stdout_line: &mut dyn FnMut(&str) -> Result<()>,
on_stderr_line: &mut dyn FnMut(&str) -> Result<()>,
capture_output: bool,
) -> Result<Output> {
) -> Result<Output, ProcessError> {
let mut stdout = Vec::new();
let mut stderr = Vec::new();

Expand All @@ -245,7 +245,7 @@ impl ProcessBuilder {
let mut callback_error = None;
let mut stdout_pos = 0;
let mut stderr_pos = 0;
let status = (|| {
let res = (|| {
let mut child = cmd.spawn()?;
let out = child.stdout.take().unwrap();
let err = child.stderr.take().unwrap();
Expand Down Expand Up @@ -293,9 +293,11 @@ impl ProcessBuilder {
*pos = 0;
})?;
child.wait()
})()
.with_context(|| {
})();

let status = res.map_err(|err| {
ProcessError::new(&format!("could not execute process {}", self), None, None)
.with_source(err)
})?;
let output = Output {
status,
Expand All @@ -306,14 +308,14 @@ impl ProcessBuilder {
{
let to_print = if capture_output { Some(&output) } else { None };
if let Some(e) = callback_error {
let cx = ProcessError::new(
return Err(ProcessError::new(
&format!("failed to parse process output: {}", self),
Some(output.status),
to_print,
);
bail!(anyhow::Error::new(cx).context(e));
)
.with_source(e));
} else if !output.status.success() {
bail!(ProcessError::new(
return Err(ProcessError::new(
&format!("process didn't exit successfully: {}", self),
Some(output.status),
to_print,
Expand Down Expand Up @@ -388,14 +390,15 @@ mod imp {
use anyhow::Result;
use std::os::unix::process::CommandExt;

pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<()> {
pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<(), ProcessError> {
let mut command = process_builder.build_command();
let error = command.exec();
Err(anyhow::Error::from(error).context(ProcessError::new(
Err(ProcessError::new(
&format!("could not execute process {}", process_builder),
None,
None,
)))
)
.with_source(error))
}
}

Expand All @@ -411,10 +414,14 @@ mod imp {
TRUE
}

pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<()> {
pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<(), ProcessError> {
unsafe {
if SetConsoleCtrlHandler(Some(ctrlc_handler), TRUE) == FALSE {
return Err(ProcessError::new("Could not set Ctrl-C handler.", None, None).into());
return Err(ProcessError::new(
"Could not set Ctrl-C handler.",
None,
None,
));
}
}

Expand Down
20 changes: 19 additions & 1 deletion crates/cargo-util/src/process_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ pub struct ProcessError {
/// This can be `None` if the process failed to launch, or the output was
/// not captured.
pub stderr: Option<Vec<u8>>,

pub source: Option<Box<dyn std::error::Error + Send + Sync>>,
}

impl fmt::Display for ProcessError {
Expand All @@ -35,7 +37,14 @@ impl fmt::Display for ProcessError {
}
}

impl std::error::Error for ProcessError {}
impl std::error::Error for ProcessError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match self.source.as_ref() {
Some(s) => Some(&**s),
None => None,
}
}
}

impl ProcessError {
/// Creates a new [`ProcessError`].
Expand Down Expand Up @@ -93,8 +102,17 @@ impl ProcessError {
code,
stdout: stdout.map(|s| s.to_vec()),
stderr: stderr.map(|s| s.to_vec()),
source: None,
}
}

pub(crate) fn with_source(
mut self,
source: impl Into<Box<dyn std::error::Error + Send + Sync>>,
) -> Self {
self.source = Some(source.into());
self
}
}

/// Converts an [`ExitStatus`] to a human-readable string suitable for
Expand Down
6 changes: 5 additions & 1 deletion src/bin/cargo/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,11 @@ Run with 'cargo -Z [FLAG] [SUBCOMMAND]'",

if let Some(code) = expanded_args.value_of("explain") {
let mut procss = config.load_global_rustc(None)?.process();
procss.arg("--explain").arg(code).exec()?;
procss
.arg("--explain")
.arg(code)
.exec()
.map_err(anyhow::Error::new)?;
return Ok(());
}

Expand Down
10 changes: 4 additions & 6 deletions src/bin/cargo/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use cargo::core::shell::Shell;
use cargo::util::toml::StringOrVec;
use cargo::util::CliError;
use cargo::util::{self, closest_msg, command_prelude, CargoResult, CliResult, Config};
use cargo_util::{ProcessBuilder, ProcessError};
use cargo_util::ProcessBuilder;
use std::collections::BTreeMap;
use std::env;
use std::fs;
Expand Down Expand Up @@ -177,12 +177,10 @@ fn execute_external_subcommand(config: &Config, cmd: &str, args: &[&str]) -> Cli
Err(e) => e,
};

if let Some(perr) = err.downcast_ref::<ProcessError>() {
if let Some(code) = perr.code {
return Err(CliError::code(code));
}
if let Some(code) = err.code {
return Err(CliError::code(code));
}
Err(CliError::new(err, 101))
Err(CliError::new(anyhow::Error::new(err), 101))
}

#[cfg(unix)]
Expand Down
18 changes: 8 additions & 10 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ pub trait Executor: Send + Sync + 'static {
mode: CompileMode,
on_stdout_line: &mut dyn FnMut(&str) -> CargoResult<()>,
on_stderr_line: &mut dyn FnMut(&str) -> CargoResult<()>,
) -> CargoResult<()>;
) -> Result<(), ProcessError>;

/// Queried when queuing each unit of work. If it returns true, then the
/// unit will always be rebuilt, independent of whether it needs to be.
Expand All @@ -130,7 +130,7 @@ impl Executor for DefaultExecutor {
_mode: CompileMode,
on_stdout_line: &mut dyn FnMut(&str) -> CargoResult<()>,
on_stderr_line: &mut dyn FnMut(&str) -> CargoResult<()>,
) -> CargoResult<()> {
) -> Result<(), ProcessError> {
cmd.exec_with_streaming(on_stdout_line, on_stderr_line, false)
.map(drop)
}
Expand Down Expand Up @@ -298,16 +298,14 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc<dyn Executor>) -> Car
}
}

fn verbose_if_simple_exit_code(err: Error) -> Error {
fn verbose_if_simple_exit_code(err: ProcessError) -> Error {
// If a signal on unix (`code == None`) or an abnormal termination
// on Windows (codes like `0xC0000409`), don't hide the error details.
match err
.downcast_ref::<ProcessError>()
.as_ref()
.and_then(|perr| perr.code)
{
Some(n) if cargo_util::is_simple_exit_code(n) => VerboseError::new(err).into(),
_ => err,
match err.code {
Some(n) if cargo_util::is_simple_exit_code(n) => {
VerboseError::new(anyhow::Error::new(err)).into()
}
_ => anyhow::Error::new(err),
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/cargo_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,5 +97,5 @@ pub fn run(

config.shell().status("Running", process.to_string())?;

process.exec_replace()
process.exec_replace().map_err(anyhow::Error::new)
}
2 changes: 0 additions & 2 deletions src/cargo/ops/cargo_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ fn run_unit_tests(
let result = cmd.exec();

if let Err(e) = result {
let e = e.downcast::<ProcessError>()?;
errors.push((
unit.target.kind().clone(),
unit.target.name().to_string(),
Expand Down Expand Up @@ -255,7 +254,6 @@ fn run_doc_tests(
.shell()
.verbose(|shell| shell.status("Running", p.to_string()))?;
if let Err(e) = p.exec() {
let e = e.downcast::<ProcessError>()?;
errors.push(e);
if !options.no_fail_fast {
return Ok((Test::Doc, errors));
Expand Down
3 changes: 1 addition & 2 deletions tests/testsuite/messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
//! Tests for message caching can be found in `cache_messages`.

use cargo_test_support::{process, project, Project};
use cargo_util::ProcessError;

/// Captures the actual diagnostics displayed by rustc. This is done to avoid
/// relying on the exact message formatting in rustc.
Expand All @@ -22,7 +21,7 @@ pub fn raw_rustc_output(project: &Project, path: &str, extra: &[&str]) -> String
.exec_with_output()
{
Ok(output) => output.stderr,
Err(e) => e.downcast::<ProcessError>().unwrap().stderr.unwrap(),
Err(e) => e.stderr.unwrap(),
};
// Do a little dance to remove rustc's "warnings emitted" message and the subsequent newline.
let stderr = std::str::from_utf8(&rustc_output).expect("utf8");
Expand Down
Loading