Improve diagnostic messages when surfacing errors from cargo. (#656)

* Consistently use lowercase when reporting errors.

* Improved error messages for errors from interaction with cargo.

* Improve alignment and deduplicate error output blocks.

* Improve diagnostic's spacing and clarity.

.github/workflows/ci.yml

@@ -1727,8 +1727,8 @@ jobs:
       - name: Check output
         run: |
           cd semver
-          EXPECTED="$(echo -e "Error: rustc version is not high enough: >=1.71.0 needed, got 1.70.0")"
-          RESULT="$(cat output | grep 'Error: rustc version')"
+          EXPECTED="$(echo -e "error: rustc version is not high enough: >=1.71.0 needed, got 1.70.0")"
+          RESULT="$(cat output | grep 'error: rustc version')"
           diff <(echo "$RESULT") <(echo "$EXPECTED")
 
       - name: Cleanup

src/config.rs

@@ -73,6 +73,10 @@ impl GlobalConfig {
         self.level.is_some() && self.level.unwrap() >= log::Level::Info
     }
 
+    pub fn is_error(&self) -> bool {
+        self.level.is_some() && self.level.unwrap() >= log::Level::Error
+    }
+
     pub fn is_verbose(&self) -> bool {
         self.level.is_some() && self.level.unwrap() >= log::Level::Debug
     }
@@ -111,6 +115,16 @@ impl GlobalConfig {
         Ok(())
     }
 
+    pub fn log_error(
+        &mut self,
+        callback: impl Fn(&mut Self) -> anyhow::Result<()>,
+    ) -> anyhow::Result<()> {
+        if self.is_error() {
+            callback(self)?;
+        }
+        Ok(())
+    }
+
     pub fn is_stderr_tty(&self) -> bool {
         self.is_stderr_tty
     }

src/lib.rs

@@ -264,6 +264,13 @@ impl Check {
         self
     }
 
+    /// Get the current log level.
+    /// If set to `None`, logging is disabled.
+    #[inline]
+    pub fn log_level(&self) -> Option<&log::Level> {
+        self.log_level.as_ref()
+    }
+
     pub fn with_release_type(&mut self, release_type: ReleaseType) -> &mut Self {
         self.release_type = Some(release_type);
         self

src/main.rs

@@ -7,7 +7,7 @@ use cargo_semver_checks::{
 };
 use clap::{Args, Parser, Subcommand};
 
-fn main() -> anyhow::Result<()> {
+fn main() {
     human_panic::setup_panic!();
 
     let Cargo::SemverChecks(args) = Cargo::parse();
@@ -22,71 +22,89 @@ fn main() -> anyhow::Result<()> {
             .print::<Markdown>();
         std::process::exit(0);
     } else if args.list {
-        let queries = SemverQuery::all_queries();
-        let mut rows = vec![["id", "type", "description"], ["==", "====", "==========="]];
-        for query in queries.values() {
-            rows.push([
-                query.id.as_str(),
-                query.required_update.as_str(),
-                query.description.as_str(),
-            ]);
-        }
-        let mut widths = [0; 3];
-        for row in &rows {
-            widths[0] = widths[0].max(row[0].len());
-            widths[1] = widths[1].max(row[1].len());
-            widths[2] = widths[2].max(row[2].len());
-        }
-        let stdout = std::io::stdout();
-        let mut stdout = stdout.lock();
-        for row in rows {
-            use std::io::Write;
-            writeln!(
-                stdout,
-                "{0:<1$} {2:<3$} {4:<5$}",
-                row[0], widths[0], row[1], widths[1], row[2], widths[2]
-            )?;
-        }
+        exit_on_error(true, || {
+            let mut config =
+                GlobalConfig::new().set_level(args.check_release.verbosity.log_level());
+            let queries = SemverQuery::all_queries();
+            let mut rows = vec![["id", "type", "description"], ["==", "====", "==========="]];
+            for query in queries.values() {
+                rows.push([
+                    query.id.as_str(),
+                    query.required_update.as_str(),
+                    query.description.as_str(),
+                ]);
+            }
+            let mut widths = [0; 3];
+            for row in &rows {
+                widths[0] = widths[0].max(row[0].len());
+                widths[1] = widths[1].max(row[1].len());
+                widths[2] = widths[2].max(row[2].len());
+            }
+            let stdout = std::io::stdout();
+            let mut stdout = stdout.lock();
+            for row in rows {
+                use std::io::Write;
+                writeln!(
+                    stdout,
+                    "{0:<1$} {2:<3$} {4:<5$}",
+                    row[0], widths[0], row[1], widths[1], row[2], widths[2]
+                )?;
+            }
 
-        let mut config = GlobalConfig::new().set_level(args.check_release.verbosity.log_level());
-        config.shell_note("Use `--explain <id>` to see more details")?;
+            config.shell_note("Use `--explain <id>` to see more details")
+        });
         std::process::exit(0);
     } else if let Some(id) = args.explain.as_deref() {
-        let queries = SemverQuery::all_queries();
-        let query = queries.get(id).ok_or_else(|| {
-            let ids = queries.keys().cloned().collect::<Vec<_>>();
-            anyhow::format_err!(
-                "Unknown id `{}`, available id's:\n  {}",
-                id,
-                ids.join("\n  ")
-            )
-        })?;
-        println!(
-            "{}",
-            query
-                .reference
-                .as_deref()
-                .unwrap_or(query.description.as_str())
-        );
-        if let Some(link) = &query.reference_link {
-            println!();
-            println!("See also {link}");
-        }
+        exit_on_error(true, || {
+            let queries = SemverQuery::all_queries();
+            let query = queries.get(id).ok_or_else(|| {
+                let ids = queries.keys().cloned().collect::<Vec<_>>();
+                anyhow::format_err!(
+                    "Unknown id `{}`, available id's:\n  {}",
+                    id,
+                    ids.join("\n  ")
+                )
+            })?;
+            println!(
+                "{}",
+                query
+                    .reference
+                    .as_deref()
+                    .unwrap_or(query.description.as_str())
+            );
+            if let Some(link) = &query.reference_link {
+                println!();
+                println!("See also {link}");
+            }
+            Ok(())
+        });
         std::process::exit(0);
     }
 
     let check: cargo_semver_checks::Check = match args.command {
         Some(SemverChecksCommands::CheckRelease(args)) => args.into(),
         None => args.check_release.into(),
     };
-    let report = check.check_release()?;
+    let report = exit_on_error(check.log_level().is_some(), || check.check_release());
     if report.success() {
         std::process::exit(0);
     } else {
         std::process::exit(1);
     }
 }
 
+fn exit_on_error<T>(log_errors: bool, inner: impl Fn() -> anyhow::Result<T>) -> T {
+    match inner() {
+        Ok(x) => x,
+        Err(err) => {
+            if log_errors {
+                eprintln!("error: {err:?}");
+            }
+            std::process::exit(1)
+        }
+    }
+}
+
 #[derive(Debug, Parser)]
 #[command(name = "cargo")]
 #[command(bin_name = "cargo")]

src/rustdoc_cmd.rs

@@ -1,6 +1,8 @@
+use std::io::Write as _;
 use std::path::{Path, PathBuf};
 
 use anyhow::Context;
+use itertools::Itertools as _;
 
 use crate::{
     rustdoc_gen::{CrateDataForRustdoc, CrateSource, FeaturesGroup},
@@ -133,17 +135,61 @@ impl RustdocCommand {
         let output = cmd.output()?;
         if !output.status.success() {
             if self.silence {
-                let stderr_output = String::from_utf8_lossy(&output.stderr);
-                anyhow::bail!(
-                    "running cargo-doc failed on {}:\n{stderr_output}",
-                    placeholder_manifest_path.display(),
-                )
+                config.log_error(|config| {
+                    let stderr = config.stderr();
+                    let delimiter = "-----";
+                    writeln!(
+                        stderr,
+                        "error: running cargo-doc on crate {crate_name} failed with output:"
+                    )?;
+                    writeln!(
+                        stderr,
+                        "{delimiter}\n{}\n{delimiter}\n",
+                        String::from_utf8_lossy(&output.stderr)
+                    )?;
+                    writeln!(
+                        stderr,
+                        "error: failed to build rustdoc for crate {crate_name} v{version}"
+                    )?;
+                    Ok(())
+                })?;
             } else {
-                anyhow::bail!(
-                    "running cargo-doc failed on {}. See stderr.",
-                    placeholder_manifest_path.display(),
-                )
+                config.log_error(|config| {
+                    let stderr = config.stderr();
+                    writeln!(
+                        stderr,
+                        "error: running cargo-doc on crate {crate_name} v{version} failed, see stderr output above"
+                    )?;
+                    Ok(())
+                })?;
             }
+            config.log_error(|config| {
+                let features =
+                    crate_source.feature_list_from_config(config, crate_data.feature_config);
+                let stderr = config.stderr();
+                writeln!(
+                    stderr,
+                    "note: this is usually due to a compilation error in the crate,"
+                )?;
+                writeln!(
+                    stderr,
+                    "      and is unlikely to be a bug in cargo-semver-checks"
+                )?;
+                writeln!(
+                    stderr,
+                    "note: running the following command on the crate should reproduce the error:"
+                )?;
+
+                writeln!(
+                    stderr,
+                    "      cargo build --no-default-features --features {}",
+                    features.into_iter().join(","),
+                )?;
+                Ok(())
+            })?;
+            anyhow::bail!(
+                "aborting due to failure to build rustdoc for crate {crate_name} v{version}"
+            );
         }
 
         let rustdoc_dir = if let Some(build_target) = crate_data.build_target {
@@ -174,9 +220,31 @@ impl RustdocCommand {
                 {
                     None
                 } else {
+                    config.log_error(|config| {
+                        let stderr = config.stderr();
+                        let delimiter = "-----";
+                        writeln!(
+                            stderr,
+                            "error: running cargo-config on crate {crate_name} failed with output:"
+                        )?;
+                        writeln!(
+                            stderr,
+                            "{delimiter}\n{}\n{delimiter}\n",
+                            String::from_utf8_lossy(&output.stderr)
+                        )?;
+
+                        writeln!(stderr, "error: unexpected cargo config output for crate {crate_name} v{version}\n")?;
+                        writeln!(stderr, "note: this may be a bug in cargo, or a bug in cargo-semver-checks;")?;
+                        writeln!(stderr, "      if unsure, feel free to open a GitHub issue on cargo-semver-checks")?;
+                        writeln!(stderr, "note: running the following command on the crate should reproduce the error:")?;
+                        writeln!(
+                            stderr,
+                            "      cargo config -Zunstable-options get --format=json-value build.target",
+                        )?;
+                        Ok(())
+                    })?;
                     anyhow::bail!(
-                        "running cargo-config failed:\n{}",
-                        String::from_utf8_lossy(&output.stderr),
+                        "aborting due to cargo-config failure on crate {crate_name} v{version}"
                     )
                 }
             };
@@ -238,7 +306,7 @@ in the metadata and stderr didn't mention it was lacking a lib target. This is p
                 return Ok(json_path);
             } else {
                 anyhow::bail!(
-                    "Could not find expected rustdoc output for `{}`: {}",
+                    "could not find expected rustdoc output for `{}`: {}",
                     crate_name,
                     json_path.display()
                 );
@@ -258,14 +326,14 @@ in the metadata and stderr didn't mention it was lacking a lib target. This is p
                 return Ok(json_path);
             } else {
                 anyhow::bail!(
-                    "Could not find expected rustdoc output for `{}`: {}",
+                    "could not find expected rustdoc output for `{}`: {}",
                     crate_name,
                     json_path.display()
                 );
             }
         }
 
-        anyhow::bail!("No lib or bin targets so nothing to scan for crate {crate_name}")
+        anyhow::bail!("no lib or bin targets so nothing to scan for crate {crate_name}")
     }
 }
 

tests/rustdoc_edge_cases.rs

@@ -33,7 +33,7 @@ fn proc_macro_target() {
         .args(["semver-checks", "check-release", "--baseline-root=."])
         .env_remove("RUST_BACKTRACE")
         .assert()
-        .stderr("Error: no crates with library targets selected, nothing to semver-check\n")
+        .stderr("error: no crates with library targets selected, nothing to semver-check\n")
         .failure();
 }
 
@@ -92,7 +92,7 @@ fn crate_in_workspace() {
         ])
         .env_remove("RUST_BACKTRACE")
         .assert()
-        .stderr("Error: no crates with library targets selected, nothing to semver-check\n")
+        .stderr("error: no crates with library targets selected, nothing to semver-check\n")
         .failure();
 }