From b2b3bdeede43c8bfb772f8feddbc3fd829740ce2 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Wed, 16 Feb 2022 14:19:29 -0800 Subject: [PATCH] Consistently use crate::display_error on errors during drain --- src/cargo/core/compiler/job_queue.rs | 60 +++++++++++++++------------- src/cargo/lib.rs | 42 +++++++++---------- src/cargo/util/errors.rs | 33 +++++++++++++++ tests/testsuite/install.rs | 4 +- 4 files changed, 85 insertions(+), 54 deletions(-) diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index c3d76c8b0c7..10e7adeecb3 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -77,6 +77,7 @@ use crate::core::compiler::future_incompat::{ use crate::core::resolver::ResolveBehavior; use crate::core::{PackageId, Shell, TargetKind}; use crate::util::diagnostic_server::{self, DiagnosticPrinter}; +use crate::util::errors::AlreadyPrintedError; use crate::util::machine_message::{self, Message as _}; use crate::util::CargoResult; use crate::util::{self, internal, profile}; @@ -169,6 +170,10 @@ struct DrainState<'cfg> { per_package_future_incompat_reports: Vec, } +pub struct ErrorsDuringDrain { + pub count: usize, +} + #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct JobId(pub u32); @@ -786,14 +791,14 @@ impl<'cfg> DrainState<'cfg> { // After a job has finished we update our internal state if it was // successful and otherwise wait for pending work to finish if it failed // and then immediately return. - let mut error = None; + let mut errors = ErrorsDuringDrain { count: 0 }; // CAUTION! Do not use `?` or break out of the loop early. Every error // must be handled in such a way that the loop is still allowed to // drain event messages. loop { - if error.is_none() { + if errors.count == 0 { if let Err(e) = self.spawn_work_if_possible(cx, jobserver_helper, scope) { - self.handle_error(&mut cx.bcx.config.shell(), &mut error, e); + self.handle_error(&mut cx.bcx.config.shell(), &mut errors, e); } } @@ -804,7 +809,7 @@ impl<'cfg> DrainState<'cfg> { } if let Err(e) = self.grant_rustc_token_requests() { - self.handle_error(&mut cx.bcx.config.shell(), &mut error, e); + self.handle_error(&mut cx.bcx.config.shell(), &mut errors, e); } // And finally, before we block waiting for the next event, drop any @@ -814,7 +819,7 @@ impl<'cfg> DrainState<'cfg> { // to the jobserver itself. for event in self.wait_for_events() { if let Err(event_err) = self.handle_event(cx, jobserver_helper, plan, event) { - self.handle_error(&mut cx.bcx.config.shell(), &mut error, event_err); + self.handle_error(&mut cx.bcx.config.shell(), &mut errors, event_err); } } } @@ -839,30 +844,24 @@ impl<'cfg> DrainState<'cfg> { } let time_elapsed = util::elapsed(cx.bcx.config.creation_time().elapsed()); - if let Err(e) = self.timings.finished(cx, &error) { - if error.is_some() { - crate::display_error(&e, &mut cx.bcx.config.shell()); - } else { - return Some(e); - } + if let Err(e) = self.timings.finished(cx, &errors.to_error()) { + self.handle_error(&mut cx.bcx.config.shell(), &mut errors, e); } if cx.bcx.build_config.emit_json() { let mut shell = cx.bcx.config.shell(); let msg = machine_message::BuildFinished { - success: error.is_none(), + success: errors.count == 0, } .to_json_string(); if let Err(e) = writeln!(shell.out(), "{}", msg) { - if error.is_some() { - crate::display_error(&e.into(), &mut shell); - } else { - return Some(e.into()); - } + self.handle_error(&mut shell, &mut errors, e.into()); } } - if let Some(e) = error { - Some(e) + if let Some(error) = errors.to_error() { + // Any errors up to this point have already been printed via the + // `display_error` inside `handle_error`. + Some(anyhow::Error::new(AlreadyPrintedError::new(error))) } else if self.queue.is_empty() && self.pending_queue.is_empty() { let message = format!( "{} [{}] target(s) in {}", @@ -887,19 +886,14 @@ impl<'cfg> DrainState<'cfg> { fn handle_error( &self, shell: &mut Shell, - err_state: &mut Option, + err_state: &mut ErrorsDuringDrain, new_err: anyhow::Error, ) { - if err_state.is_some() { - // Already encountered one error. - log::warn!("{:?}", new_err); - } else if !self.active.is_empty() { - crate::display_error(&new_err, shell); + crate::display_error(&new_err, shell); + if !self.active.is_empty() && err_state.count == 0 { drop(shell.warn("build failed, waiting for other jobs to finish...")); - *err_state = Some(anyhow::format_err!("build failed")); - } else { - *err_state = Some(new_err); } + err_state.count += 1; } // This also records CPU usage and marks concurrency; we roughly want to do @@ -1216,3 +1210,13 @@ feature resolver. Try updating to diesel 1.4.8 to fix this error. Ok(()) } } + +impl ErrorsDuringDrain { + fn to_error(&self) -> Option { + match self.count { + 0 => None, + 1 => Some(format_err!("1 job failed")), + n => Some(format_err!("{} jobs failed", n)), + } + } +} diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index 17fcf8c0fe1..7ce2ce9dd2e 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -13,7 +13,7 @@ use crate::core::Shell; use anyhow::Error; use log::debug; -pub use crate::util::errors::{InternalError, VerboseError}; +pub use crate::util::errors::{AlreadyPrintedError, InternalError, VerboseError}; pub use crate::util::{indented_lines, CargoResult, CliError, CliResult, Config}; pub use crate::version::version; @@ -76,31 +76,27 @@ pub fn display_warning_with_error(warning: &str, err: &Error, shell: &mut Shell) } fn _display_error(err: &Error, shell: &mut Shell, as_err: bool) -> bool { - let verbosity = shell.verbosity(); - let is_verbose = |e: &(dyn std::error::Error + 'static)| -> bool { - verbosity != Verbose && e.downcast_ref::().is_some() - }; - // Generally the top error shouldn't be verbose, but check it anyways. - if is_verbose(err.as_ref()) { - return true; - } - if as_err { - drop(shell.error(&err)); - } else { - drop(writeln!(shell.err(), "{}", err)); - } - for cause in err.chain().skip(1) { - // If we're not in verbose mode then print remaining errors until one + for (i, err) in err.chain().enumerate() { + // If we're not in verbose mode then only print cause chain until one // marked as `VerboseError` appears. - if is_verbose(cause) { + // + // Generally the top error shouldn't be verbose, but check it anyways. + if shell.verbosity() != Verbose && err.is::() { return true; } - drop(writeln!(shell.err(), "\nCaused by:")); - drop(write!( - shell.err(), - "{}", - indented_lines(&cause.to_string()) - )); + if err.is::() { + break; + } + if i == 0 { + if as_err { + drop(shell.error(&err)); + } else { + drop(writeln!(shell.err(), "{}", err)); + } + } else { + drop(writeln!(shell.err(), "\nCaused by:")); + drop(write!(shell.err(), "{}", indented_lines(&err.to_string()))); + } } false } diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs index 524a5d627c4..3668c92152f 100644 --- a/src/cargo/util/errors.rs +++ b/src/cargo/util/errors.rs @@ -99,6 +99,39 @@ impl fmt::Display for InternalError { } } +// ============================================================================= +// Already printed error + +/// An error that does not need to be printed because it does not add any new +/// information to what has already been printed. +pub struct AlreadyPrintedError { + inner: Error, +} + +impl AlreadyPrintedError { + pub fn new(inner: Error) -> Self { + AlreadyPrintedError { inner } + } +} + +impl std::error::Error for AlreadyPrintedError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.inner.source() + } +} + +impl fmt::Debug for AlreadyPrintedError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.inner.fmt(f) + } +} + +impl fmt::Display for AlreadyPrintedError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.inner.fmt(f) + } +} + // ============================================================================= // Manifest error diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 4ba58c9a3d3..99d4c0b163e 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -879,11 +879,9 @@ fn compile_failure() { .with_status(101) .with_stderr_contains( "\ +[ERROR] could not compile `foo` due to previous error [ERROR] failed to compile `foo v0.0.1 ([..])`, intermediate artifacts can be \ found at `[..]target` - -Caused by: - could not compile `foo` due to previous error ", ) .run();