improve output

Author: Emilgardis

.cargo/config.toml

@@ -2,4 +2,4 @@
 linker = "aarch64-linux-gnu-gcc"
 
 [alias]
-regress = "run -p svd2rust-regress --"
+regress = "run -p svd2rust-regress --"

.github/workflows/diff.yml

@@ -18,6 +18,8 @@ jobs:
 
       - name: Cache
         uses: Swatinem/rust-cache@v2
+        with:
+          shared-key: "diff"
 
       - run: cargo regress ci
         id: regress-ci
@@ -41,13 +43,14 @@ jobs:
       - name: Cache
         uses: Swatinem/rust-cache@v2
         with:
-          cache-on-failure: true
+          shared-key: "diff"
 
       - uses: taiki-e/install-action@v2
         if: matrix.needs_semver_checks
         with:
           tool: cargo-semver-checks
 
+      # if a new line is added here, make sure to update the `summary` job to reference the new step index
       - uses: taiki-e/install-action@v2
         with:
           tool: git-delta
@@ -59,8 +62,8 @@ jobs:
           GIT_PAGER: delta --hunk-header-style omit
   summary:
     runs-on: ubuntu-latest
-    needs: [diff]
-    if: always()
+    needs: [diff, generate]
+    if: always() && needs.generate.outputs.diffs != '{}' && needs.generate.outputs.diffs != '[]' && needs.generate.outputs.diffs != ''
     steps:
       - uses: actions/checkout@v4
 

ci/svd2rust-regress/src/command.rs

@@ -7,7 +7,7 @@ pub trait CommandExt {
     fn run(&mut self, hide: bool) -> Result<(), anyhow::Error>;
 
     #[track_caller]
-    fn get_output(&mut self) -> Result<std::process::Output, anyhow::Error>;
+    fn get_output(&mut self, can_fail: bool) -> Result<std::process::Output, anyhow::Error>;
 
     #[track_caller]
     fn get_output_string(&mut self) -> Result<String, anyhow::Error>;
@@ -33,17 +33,11 @@ impl CommandExt for Command {
     }
 
     #[track_caller]
-    fn get_output(&mut self) -> Result<std::process::Output, anyhow::Error> {
-        let output = self.output().with_context(|| {
-            format!(
-                "command `{}{}` couldn't be run",
-                self.get_current_dir()
-                    .map(|d| format!("{} ", d.display()))
-                    .unwrap_or_default(),
-                self.display()
-            )
-        })?;
-        if output.status.success() {
+    fn get_output(&mut self, can_fail: bool) -> Result<std::process::Output, anyhow::Error> {
+        let output = self
+            .output()
+            .with_context(|| format!("command `{}` couldn't be run", self.display()))?;
+        if output.status.success() || can_fail {
             Ok(output)
         } else {
             anyhow::bail!(
@@ -57,12 +51,15 @@ impl CommandExt for Command {
 
     #[track_caller]
     fn get_output_string(&mut self) -> Result<String, anyhow::Error> {
-        String::from_utf8(self.get_output()?.stdout).map_err(Into::into)
+        String::from_utf8(self.get_output(true)?.stdout).map_err(Into::into)
     }
 
     fn display(&self) -> String {
         format!(
-            "{} {}",
+            "{}{} {}",
+            self.get_current_dir()
+                .map(|d| format!("{} ", d.display()))
+                .unwrap_or_default(),
             self.get_program().to_string_lossy(),
             self.get_args()
                 .map(|s| s.to_string_lossy())

ci/svd2rust-regress/src/diff.rs

@@ -13,10 +13,10 @@ pub struct Diffing {
     /// Change the base version by starting with `@` followed by the source.
     ///
     /// supports `@pr` for current pr, `@master` for latest master build, or a version tag like `@v0.30.0`
-    #[clap(global = true, long, alias = "base")]
+    #[clap(global = true, long = "baseline", alias = "base")]
     pub baseline: Option<String>,
 
-    #[clap(global = true, long, alias = "head")]
+    #[clap(global = true, long = "current", alias = "head")]
     pub current: Option<String>,
 
     /// Enable formatting with `rustfmt`
@@ -72,6 +72,17 @@ pub struct Diffing {
 pub enum DiffingMode {
     Semver,
     Diff,
+    Pr,
+}
+
+impl DiffingMode {
+    /// Returns `true` if the diffing mode is [`Pr`].
+    ///
+    /// [`Pr`]: DiffingMode::Pr
+    #[must_use]
+    pub fn is_pr(&self) -> bool {
+        matches!(self, Self::Pr)
+    }
 }
 
 type Source<'s> = Option<&'s str>;
@@ -83,7 +94,7 @@ impl Diffing {
             .make_case(opts)
             .with_context(|| "couldn't setup test case")?;
         match self.sub.unwrap_or(DiffingMode::Diff) {
-            DiffingMode::Diff => {
+            DiffingMode::Diff | DiffingMode::Pr => {
                 let mut command;
                 if let Some(pager) = &self.pager {
                     if self.use_pager_directly {
@@ -156,27 +167,34 @@ impl Diffing {
                 }
             })
             .collect::<Vec<_>>();
-        if tests.len() != 1 {
-            let error = anyhow::anyhow!("diff requires exactly one test case");
-            let len = tests.len();
-            return Err(match len {
-                0 => error.context("matched no tests"),
-                10.. => error.context(format!("matched multiple ({len}) tests")),
-                _ => error.context(format!(
-                    "matched multiple ({len}) tests\n{:?}",
-                    tests.iter().map(|t| t.name()).collect::<Vec<_>>()
-                )),
-            });
-        }
+        let test = match (tests.len(), self.sub) {
+            (1, _) => tests[0],
+            (1.., Some(DiffingMode::Pr)) => tests
+                .iter()
+                .find(|t| t.chip == "STM32F401")
+                .unwrap_or(&tests[0]),
+            _ => {
+                let error = anyhow::anyhow!("diff requires exactly one test case");
+                let len = tests.len();
+                return Err(match len {
+                    0 => error.context("matched no tests"),
+                    10.. => error.context(format!("matched multiple ({len}) tests")),
+                    _ => error.context(format!(
+                        "matched multiple ({len}) tests\n{:?}",
+                        tests.iter().map(|t| t.name()).collect::<Vec<_>>()
+                    )),
+                });
+            }
+        };
 
-        let baseline = tests[0]
+        let baseline = test
             .setup_case(
                 &opts.output_dir.join("baseline"),
                 &baseline_bin,
                 baseline_cmd,
             )
             .with_context(|| "couldn't create head")?;
-        let current = tests[0]
+        let current = test
             .setup_case(&opts.output_dir.join("current"), &current_bin, current_cmd)
             .with_context(|| "couldn't create base")?;
 

ci/svd2rust-regress/src/github.rs

@@ -148,7 +148,7 @@ pub fn get_release_binary_artifact(
                 Command::new("gzip")
                     .arg("-d")
                     .arg(output_dir.join(artifact))
-                    .get_output()?;
+                    .get_output(false)?;
             }
         }
         _ => {

ci/svd2rust-regress/src/main.rs

@@ -348,6 +348,7 @@ fn main() -> Result<(), anyhow::Error> {
     tracing_subscriber::fmt()
         .pretty()
         .with_target(false)
+        .with_writer(std::io::stderr)
         .with_env_filter(
             tracing_subscriber::EnvFilter::builder()
                 .with_default_directive(tracing::level_filters::LevelFilter::INFO.into())

ci/svd2rust-regress/src/svd_test.rs

@@ -6,6 +6,7 @@ use std::io::prelude::*;
 use std::path::PathBuf;
 use std::process::{Command, Output};
 use std::{
+    fmt::Write as _,
     fs::{self, File, OpenOptions},
     path::Path,
 };
@@ -67,7 +68,7 @@ impl std::fmt::Debug for ProcessFailed {
 
 trait CommandHelper {
     fn capture_outputs(
-        &self,
+        &mut self,
         cant_fail: bool,
         name: &str,
         stdout: Option<&PathBuf>,
@@ -76,26 +77,48 @@ trait CommandHelper {
     ) -> Result<(), TestError>;
 }
 
-impl CommandHelper for Output {
+impl CommandHelper for Command {
+    #[tracing::instrument(skip_all, fields(stdout = tracing::field::Empty, stderr = tracing::field::Empty))]
     fn capture_outputs(
-        &self,
+        &mut self,
         cant_fail: bool,
         name: &str,
         stdout: Option<&PathBuf>,
         stderr: Option<&PathBuf>,
         previous_processes_stderr: &[PathBuf],
     ) -> Result<(), TestError> {
+        let output = self.get_output(true)?;
+        let out_payload = String::from_utf8_lossy(&output.stdout);
         if let Some(out) = stdout {
-            let out_payload = String::from_utf8_lossy(&self.stdout);
             file_helper(&out_payload, out)?;
         };
 
+        let err_payload = String::from_utf8_lossy(&output.stderr);
         if let Some(err) = stderr {
-            let err_payload = String::from_utf8_lossy(&self.stderr);
             file_helper(&err_payload, err)?;
         };
-
-        if cant_fail && !self.status.success() {
+        if cant_fail && !output.status.success() {
+            let span = tracing::Span::current();
+            let mut message = format!("Process failed: {}", self.display());
+            if !out_payload.trim().is_empty() {
+                span.record(
+                    "stdout",
+                    tracing::field::display(
+                        stdout.map(|p| p.display().to_string()).unwrap_or_default(),
+                    ),
+                );
+                write!(message, "\nstdout: \n{}", out_payload).unwrap();
+            }
+            if !err_payload.trim().is_empty() {
+                span.record(
+                    "stderr",
+                    tracing::field::display(
+                        stderr.map(|p| p.display().to_string()).unwrap_or_default(),
+                    ),
+                );
+                write!(message, "\nstderr: \n{}", err_payload).unwrap();
+            }
+            tracing::error!(message=%message);
             return Err(ProcessFailed {
                 command: name.into(),
                 stdout: stdout.cloned(),
@@ -125,18 +148,17 @@ impl TestCase {
             .with_context(|| anyhow!("when setting up case for {}", self.name()))?;
         // Run `cargo check`, capturing stderr to a log file
         let cargo_check_err_file = path_helper_base(&chip_dir, &["cargo-check.err.log"]);
-        let output = Command::new("cargo")
+        Command::new("cargo")
             .arg("check")
             .current_dir(&chip_dir)
-            .output()
+            .capture_outputs(
+                true,
+                "cargo check",
+                None,
+                Some(&cargo_check_err_file),
+                &process_stderr_paths,
+            )
             .with_context(|| "failed to check")?;
-        output.capture_outputs(
-            true,
-            "cargo check",
-            None,
-            Some(&cargo_check_err_file),
-            &process_stderr_paths,
-        )?;
         process_stderr_paths.push(cargo_check_err_file);
         Ok(if opts.verbose > 1 {
             Some(process_stderr_paths)
@@ -180,9 +202,8 @@ impl TestCase {
             .arg("--vcs")
             .arg("none")
             .arg(&chip_dir)
-            .output()
-            .with_context(|| "Failed to cargo init")?
-            .capture_outputs(true, "cargo init", None, None, &[])?;
+            .capture_outputs(true, "cargo init", None, None, &[])
+            .with_context(|| "Failed to cargo init")?;
         let svd_toml = path_helper_base(&chip_dir, &["Cargo.toml"]);
         let mut file = OpenOptions::new()
             .write(true)
@@ -242,23 +263,22 @@ impl TestCase {
         if let Some(command) = command {
             svd2rust_bin.arg(command);
         }
-        let output = svd2rust_bin
+        svd2rust_bin
             .args(["-i", &chip_svd])
             .args(["--target", target])
             .current_dir(&chip_dir)
-            .get_output()?;
-        output.capture_outputs(
-            true,
-            "svd2rust",
-            Some(&lib_rs_file).filter(|_| {
-                !matches!(
-                    self.arch,
-                    Target::CortexM | Target::Msp430 | Target::XtensaLX
-                )
-            }),
-            Some(&svd2rust_err_file),
-            &[],
-        )?;
+            .capture_outputs(
+                true,
+                "svd2rust",
+                Some(&lib_rs_file).filter(|_| {
+                    !matches!(
+                        self.arch,
+                        Target::CortexM | Target::Msp430 | Target::XtensaLX
+                    )
+                }),
+                Some(&svd2rust_err_file),
+                &[],
+            )?;
         process_stderr_paths.push(svd2rust_err_file);
         match self.arch {
             Target::CortexM | Target::Mips | Target::Msp430 | Target::XtensaLX => {
@@ -287,20 +307,19 @@ impl TestCase {
             let new_lib_rs_file = path_helper_base(&chip_dir, &["lib.rs"]);
             std::fs::rename(lib_rs_file, &new_lib_rs_file)
                 .with_context(|| "While moving lib.rs file")?;
-            let output = Command::new(form_bin_path)
+            Command::new(form_bin_path)
                 .arg("--input")
                 .arg(&new_lib_rs_file)
                 .arg("--outdir")
                 .arg(&src_dir)
-                .output()
+                .capture_outputs(
+                    true,
+                    "form",
+                    None,
+                    Some(&form_err_file),
+                    &process_stderr_paths,
+                )
                 .with_context(|| "failed to form")?;
-            output.capture_outputs(
-                true,
-                "form",
-                None,
-                Some(&form_err_file),
-                &process_stderr_paths,
-            )?;
             std::fs::remove_file(&new_lib_rs_file)
                 .with_context(|| "While removing lib.rs file after form")?;
         }
@@ -322,15 +341,14 @@ impl TestCase {
                 let output = Command::new(rustfmt_bin_path)
                     .arg(entry)
                     .args(["--edition", "2021"])
-                    .output()
+                    .capture_outputs(
+                        false,
+                        "rustfmt",
+                        None,
+                        Some(&rustfmt_err_file),
+                        &process_stderr_paths,
+                    )
                     .with_context(|| "failed to format")?;
-                output.capture_outputs(
-                    false,
-                    "rustfmt",
-                    None,
-                    Some(&rustfmt_err_file),
-                    &process_stderr_paths,
-                )?;
             }
 
             process_stderr_paths.push(rustfmt_err_file);