Merge pull request #806 from rust-embedded/regress-improvements

regress improvements

.github/workflows/diff.yml

@@ -5,6 +5,8 @@ on:
 
 jobs:
   generate:
+    name: |
+      Generate matrix. ${{ github.event.comment.user.name }}: ${{ github.event.comment.author_association}}
     runs-on: ubuntu-latest
     outputs:
       diffs: ${{ steps.regress-ci.outputs.diffs }}
@@ -25,7 +27,7 @@ jobs:
         id: regress-ci
         env:
           GITHUB_COMMENT: ${{ github.event.comment.body }}
-          GITHUB_COMMENT_PR: ${{ github.event.comment.issue_url }}
+          GITHUB_COMMENT_PR: ${{ github.event.issue.number }}
   diff:
     runs-on: ubuntu-latest
     needs: [generate]
@@ -50,16 +52,17 @@ jobs:
         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
 
-      - run: cargo regress diff --use-pager-directly ${{ matrix.command }}
+      # if a new line is added here, make sure to update the `summary` job to reference the new step index
+      - run: cargo regress diff ${{ matrix.command }}
+      - run: cargo regress diff ${{ matrix.command }}
         env:
           GH_TOKEN: ${{ github.token }}
           GITHUB_PR: ${{ matrix.pr }}
-          GIT_PAGER: delta --hunk-header-style omit
+          GIT_PAGER: delta --raw
   summary:
     runs-on: ubuntu-latest
     needs: [diff, generate]
@@ -68,9 +71,8 @@ jobs:
       - uses: actions/checkout@v4
 
       - run: |
-          PR_ID=$(echo "${{ github.event.comment.issue_url }}" | grep -o '[0-9]\+$')
           gh run view ${{ github.run_id }} --json jobs | \
-          jq -r '"Diff for [comment]("+$comment+")\n\n" + ([.jobs[] | select(.name | startswith("diff")) | "- [" + (.name | capture("\\((?<name>[^,]+),.*") | .name) + "](" + .url + "?pr=" + $pr_id + "#step:7:45)"] | join("\n"))' --arg pr_id "$PR_ID" --arg comment "${{ github.event.comment.url }}"| \
-          gh pr comment "$PR_ID" --body "$(< /dev/stdin)"
+          jq -r '"Diff for [comment]("+$comment+")\n\n" + ([.jobs[] | select(.name | startswith("diff")) | "- [" + (.name | capture("\\((?<name>[^,]+),.*") | .name) + "](" + .url + "?pr=" + $pr_id + "#step:7:47)"] | join("\n"))' --arg pr_id "${{ github.event.issue.number }}" --arg comment "${{ github.event.comment.url }}"| \
+          gh pr comment "${{ github.event.issue.number }}" --body "$(< /dev/stdin)"
         env:
           GH_TOKEN: ${{ github.token }}

Cargo.toml

@@ -73,4 +73,10 @@ features = ["full","extra-traits"]
 [workspace]
 members = ["ci/svd2rust-regress"]
 default-members = ["."]
-exclude = ["output"]
+exclude = [
+    "output",
+    # workaround for https://github.com/rust-lang/cargo/pull/12779, doesn't work for output though
+    # see https://github.com/rust-lang/cargo/issues/6009#issuecomment-1925445245
+    "output/baseline/**",
+    "output/current/**"
+]

ci/svd2rust-regress/Cargo.toml

@@ -20,3 +20,4 @@ wildmatch = "2.1.1"
 which = "5.0.0"
 tracing = "0.1.40"
 tracing-subscriber = { version = "0.3.18", features = ["env-filter", "fmt"] }
+shell-words = "1.1"

ci/svd2rust-regress/src/ci.rs

@@ -11,7 +11,7 @@ pub struct Ci {
     #[clap(env = "GITHUB_COMMENT")]
     pub comment: String,
     #[clap(env = "GITHUB_COMMENT_PR")]
-    pub comment_pr: String,
+    pub comment_pr: usize,
 }
 
 #[derive(serde::Serialize)]
@@ -32,7 +32,7 @@ impl Ci {
             diffs.push(Diff {
                 needs_semver_checks: command.contains("semver"),
                 command: command.to_owned(),
-                pr: self.comment_pr.split('/').last().unwrap().parse()?,
+                pr: self.comment_pr,
             });
         }
         let json = serde_json::to_string(&diffs)?;

ci/svd2rust-regress/src/diff.rs

@@ -8,14 +8,15 @@ use crate::Opts;
 #[derive(clap::Parser, Debug)]
 #[clap(name = "diff")]
 pub struct Diffing {
-    /// The base version of svd2rust to use and the command input, defaults to latest master build
+    /// The base version of svd2rust to use and the command input, defaults to latest master build: `@master`
     ///
     /// 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 = "baseline", alias = "base")]
     pub baseline: Option<String>,
 
+    /// The head version of svd2rust to use and the command input, defaults to current pr: `@pr`
     #[clap(global = true, long = "current", alias = "head")]
     pub current: Option<String>,
 
@@ -27,14 +28,12 @@ pub struct Diffing {
     #[clap(global = true, long)]
     pub form_split: bool,
 
-    #[clap(subcommand)]
-    pub sub: Option<DiffingMode>,
-
     #[clap(global = true, long, short = 'c')]
     pub chip: Vec<String>,
 
     /// Filter by manufacturer, case sensitive, may be combined with other filters
     #[clap(
+            global = true,
             short = 'm',
             long = "manufacturer",
             ignore_case = true,
@@ -44,6 +43,7 @@ pub struct Diffing {
 
     /// Filter by architecture, case sensitive, may be combined with other filters
     #[clap(
+            global = true,
             short = 'a',
             long = "architecture",
             ignore_case = true,
@@ -54,20 +54,24 @@ pub struct Diffing {
     #[clap(global = true, long)]
     pub diff_folder: Option<PathBuf>,
 
-    #[clap(hide = true, env = "GITHUB_PR")]
+    /// The pr number to use for `@pr`. If not set will try to get the current pr with the command `gh pr`
+    #[clap(env = "GITHUB_PR", global = true, long)]
     pub pr: Option<usize>,
 
-    #[clap(env = "GIT_PAGER", long)]
+    #[clap(env = "GIT_PAGER", global = true, long)]
     pub pager: Option<String>,
 
     /// if set, will use pager directly instead of `git -c core.pager`
-    #[clap(long, short = 'P')]
+    #[clap(global = true, long, short = 'P')]
     pub use_pager_directly: bool,
 
     /// URL for SVD to download
     #[clap(global = true, long)]
     pub url: Option<String>,
 
+    #[clap(subcommand)]
+    pub sub: Option<DiffingMode>,
+
     #[clap(last = true)]
     pub last_args: Option<String>,
 }

ci/svd2rust-regress/src/main.rs

@@ -50,7 +50,7 @@ pub fn get_cargo_workspace() -> &'static std::path::Path {
 }
 
 #[derive(clap::Parser, Debug)]
-pub struct TestOpts {
+pub struct TestAll {
     /// Run a long test (it's very long)
     #[clap(short = 'l', long)]
     pub long_test: bool,
@@ -59,7 +59,7 @@ pub struct TestOpts {
     #[clap(short = 'c', long)]
     pub chip: Vec<String>,
 
-    /// Filter by manufacturer, case sensitive, may be combined with other filters
+    /// Filter by manufacturer, may be combined with other filters
     #[clap(
     short = 'm',
     long = "manufacturer",
@@ -68,7 +68,7 @@ pub struct TestOpts {
 )]
     pub mfgr: Option<String>,
 
-    /// Filter by architecture, case sensitive, may be combined with other filters
+    /// Filter by architecture, may be combined with other filters
     #[clap(
     short = 'a',
     long = "architecture",
@@ -104,7 +104,97 @@ pub struct TestOpts {
     // TODO: Compile svd2rust?
 }
 
-impl TestOpts {
+#[derive(clap::Parser, Debug)]
+// TODO: Replace with https://github.com/clap-rs/clap/issues/2621 when available
+#[group(id = "svd_source", required = true)]
+pub struct Test {
+    /// Enable formatting with `rustfmt`
+    #[arg(short = 'f', long)]
+    pub format: bool,
+
+    #[arg(long)]
+    /// Enable splitting `lib.rs` with `form`
+    pub form_lib: bool,
+
+    #[arg(
+        short = 'm',
+        long = "manufacturer",
+        ignore_case = true,
+        value_parser = manufacturers(),
+    )]
+    /// Manufacturer
+    pub mfgr: Option<String>,
+    #[arg(
+        short = 'a',
+        long = "architecture",
+        ignore_case = true,
+        value_parser = architectures(),
+    )]
+    /// Architecture
+    pub arch: Option<String>,
+    #[arg(long, group = "svd_source", conflicts_with_all = ["svd_file"], requires = "arch")]
+    /// URL to SVD file to test
+    pub url: Option<String>,
+    #[arg(long = "svd", group = "svd_source")]
+    /// Path to SVD file to test
+    pub svd_file: Option<PathBuf>,
+    #[arg(long, group = "svd_source")]
+    /// Chip to use, use `--url` or `--svd-file` for another way to specify svd
+    pub chip: Option<String>,
+
+    /// Path to an `svd2rust` binary, relative or absolute.
+    /// Defaults to `target/release/svd2rust[.exe]` of this repository
+    /// (which must be already built)
+    #[clap(short = 'p', long = "svd2rust-path", default_value = default_svd2rust())]
+    pub current_bin_path: PathBuf,
+    #[clap(last = true)]
+    pub command: Option<String>,
+}
+
+impl Test {
+    fn run(&self, opts: &Opts) -> Result<(), anyhow::Error> {
+        match self {
+            Self { url: Some(url), .. } => {}
+            Self {
+                svd_file: Some(svd_file),
+                ..
+            } => {}
+            Self {
+                chip: Some(chip), ..
+            } => {}
+            _ => unreachable!("clap should not allow this"),
+        }
+        let test = if let (Some(url), Some(arch)) = (&self.url, &self.arch) {
+            tests::TestCase {
+                arch: svd2rust::Target::parse(&arch)?,
+                mfgr: tests::Manufacturer::Unknown,
+                chip: self
+                    .chip
+                    .as_deref()
+                    .or_else(|| url.rsplit('/').next().and_then(|s| s.strip_suffix(".svd")))
+                    .ok_or_else(|| {
+                        anyhow::anyhow!(
+                            "could not figure out chip name, specify with `--chip <name>`",
+                        )
+                    })?
+                    .to_owned(),
+                svd_url: Some(url.clone()),
+                should_pass: true,
+                run_when: tests::RunWhen::default(),
+            }
+        } else {
+            tests::tests(Some(&opts.test_cases))?
+                .iter()
+                .find(|t| self.chip.iter().any(|c| WildMatch::new(c).matches(&t.chip)))
+                .ok_or_else(|| anyhow::anyhow!("no test found for chip"))?
+                .to_owned()
+        };
+        test.test(opts, &self.current_bin_path, self.command.as_deref())?;
+        Ok(())
+    }
+}
+
+impl TestAll {
     fn run(&self, opt: &Opts) -> Result<(), anyhow::Error> {
         let tests = tests::tests(Some(&opt.test_cases))?
             .iter()
@@ -152,7 +242,7 @@ impl TestOpts {
         tests.par_iter().for_each(|t| {
             let start = Instant::now();
 
-            match t.test(opt, self) {
+            match t.test(opt, &self.current_bin_path, self.command.as_deref()) {
                 Ok(s) => {
                     if let Some(stderrs) = s {
                         let mut buf = String::new();
@@ -217,7 +307,8 @@ impl TestOpts {
 #[derive(clap::Subcommand, Debug)]
 pub enum Subcommand {
     Diff(Diffing),
-    Tests(TestOpts),
+    Tests(TestAll),
+    Test(Test),
     Ci(Ci),
 }
 
@@ -256,15 +347,17 @@ pub struct Opts {
 impl Opts {
     const fn use_rustfmt(&self) -> bool {
         match self.subcommand {
-            Subcommand::Tests(TestOpts { format, .. })
+            Subcommand::Tests(TestAll { format, .. })
+            | Subcommand::Test(Test { format, .. })
             | Subcommand::Diff(Diffing { format, .. })
             | Subcommand::Ci(Ci { format, .. }) => format,
         }
     }
 
     const fn use_form(&self) -> bool {
         match self.subcommand {
-            Subcommand::Tests(TestOpts { form_lib, .. })
+            Subcommand::Tests(TestAll { form_lib, .. })
+            | Subcommand::Test(Test { form_lib, .. })
             | Subcommand::Diff(Diffing {
                 form_split: form_lib,
                 ..
@@ -278,13 +371,10 @@ impl Opts {
 fn default_test_cases() -> std::ffi::OsString {
     std::env::var_os("CARGO_MANIFEST_DIR").map_or_else(
         || std::ffi::OsString::from("tests.yml".to_owned()),
-        |mut e| {
-            e.extend([std::ffi::OsStr::new("/tests.yml")]);
-            std::path::PathBuf::from(e)
-                .strip_prefix(std::env::current_dir().unwrap())
-                .unwrap()
-                .to_owned()
-                .into_os_string()
+        |path| {
+            let path = std::path::PathBuf::from(path);
+            let path = path.join("tests.yml");
+            path.to_owned().into_os_string()
         },
     )
 }
@@ -414,6 +504,13 @@ fn main() -> Result<(), anyhow::Error> {
         }
         Subcommand::Diff(diff) => diff.run(&opt).with_context(|| "failed to run diff"),
         Subcommand::Ci(ci) => ci.run(&opt).with_context(|| "failed to run ci"),
+        Subcommand::Test(test) => {
+            anyhow::ensure!(
+                test.current_bin_path.exists(),
+                "svd2rust binary does not exist"
+            );
+            test.run(&opt).with_context(|| "failed to run test")
+        }
     }
 }