Consistently use crate::display_error on errors during drain

Author: dtolnay

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<FutureIncompatReportPackage>,
 }
 
+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<anyhow::Error>,
+        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<anyhow::Error> {
+        match self.count {
+            0 => None,
+            1 => Some(format_err!("1 job failed")),
+            n => Some(format_err!("{} jobs failed", n)),
+        }
+    }
+}

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::<VerboseError>().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::<VerboseError>() {
             return true;
         }
-        drop(writeln!(shell.err(), "\nCaused by:"));
-        drop(write!(
-            shell.err(),
-            "{}",
-            indented_lines(&cause.to_string())
-        ));
+        if err.is::<AlreadyPrintedError>() {
+            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
 }

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
 

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();