Fix workspace handling CLI bugs. (#381)

* Fix baseline path handling and duplicated `Parsing` output. * `--exclude` implies `--workspace`. * Fix `publish = false` item handling. * Fix `publish = false` handling when a crate is specifically selected. * Add manufactured semver break test from GitHub Action. * Don't show empty list of possible causes for missing packages. * Fix `--exclude` handling on crate manifests within a workspace. * Redirect and `tee` output. * Strip ANSI escapes for colors and bold text. * Remove stray quote. * Only run tokio checks pre-publish, not on every PR. * Look for fragment in output file.
obi1kenobi · Feb 22, 2023 · c774e98 · c774e98
1 parent 9a1cf6d
commit c774e98
Show file tree

Hide file tree

Showing 5 changed files with 164 additions and 42 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -28,6 +28,7 @@ jobs:
       - run-on-bevy-gltf
       - run-on-clap
       - run-on-sqllogictest
+      - run-on-ref-slice-fork
     steps:
       - run: exit 0
 
@@ -414,6 +415,54 @@ jobs:
       - name: Run via package Cargo.toml
         run: cargo run semver-checks check-release --manifest-path subject/sqllogictest/Cargo.toml
 
+  run-on-tokio:
+    # cargo-semver-checks crashed here due to improper CLI argument handling:
+    # https://github.com/obi1kenobi/cargo-semver-checks/issues/380
+    name: Run cargo-semver-checks on tokio ~v1.25.0
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout cargo-semver-checks
+        uses: actions/checkout@v3
+        with:
+          persist-credentials: false
+
+      - name: Checkout tokio
+        uses: actions/checkout@v3
+        with:
+          persist-credentials: false
+          repository: 'tokio-rs/tokio'
+          ref: 'd7b7c6131774ab631be6529fef3680abfeeb4781'
+          path: 'subject'
+
+      - name: Install rust
+        uses: actions-rs/toolchain@v1
+        with:
+          toolchain: stable
+          profile: minimal
+          override: true
+
+      - uses: Swatinem/rust-cache@v2
+        with:
+          key: tokio
+
+      # This test caught two bugs:
+      # - The default baseline was set to the current path instead of the default registry version.
+      # - Specifying `--exclude` together with a crate manifest that is within a workspace
+      #   (but doesn't *itself* define the workspace) would cause the entire workspace to
+      #   get tested, even though only a single crate's manifest was specified.
+      - name: Run semver-checks on tokio-stream crate manifest only
+        run: cargo run semver-checks check-release --manifest-path="subject/tokio-stream/Cargo.toml" --release-type minor --exclude benches --exclude examples --exclude stress-test --exclude tests-build --exclude tests-integration
+
+      # This test caught a bug where `--exclude` was silently ignored
+      # if `--workspace` wasn't set at the same time.
+      - name: Run semver-checks on workspace manifest with explicit exclusions
+        run: cargo run semver-checks check-release --manifest-path="subject/Cargo.toml" --release-type minor --exclude examples --exclude stress-test --exclude tests-build --exclude tests-integration
+
+      # This test caught a bug where `publish = false` items in a workspace were semver-checked
+      # unless either explicit `--workspace` was present or was implied e.g. via `--exclude`.
+      - name: Run semver-checks on workspace manifest with implicit exclusions
+        run: cargo run semver-checks check-release --manifest-path="subject/Cargo.toml" --release-type minor
+
   run-on-clap:
     # clap v3.2.0 added a semver violation
     # check whether cargo-semver-checks detects the issue
@@ -496,6 +545,74 @@ jobs:
           RESULT="$(cat output | grep failure)"
           diff <(echo "$RESULT") <(echo "$EXPECTED")
 
+  run-on-ref-slice-fork:
+    # Simulate testing an as-yet-unreleased crate version without an explicit baseline,
+    # i.e. with the expectation that the largest crates.io version is used as a baseline.
+    # This broke when a PR accidentally changed the default baseline behavior.
+    # https://github.com/obi1kenobi/cargo-semver-checks/issues/382
+    #
+    # To perform this test, we forked a stable crate and made a semver-major change in
+    # an unreleased patch version (v1.2.2). In this test, we make sure that:
+    # - v1.2.1 is chosen as the baseline, as the largest crates.io version,
+    # - cargo-semver-checks detects the semver break we've manufactured.
+    name: Run cargo-semver-checks on ref-slice fork
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout cargo-semver-checks
+        uses: actions/checkout@v3
+        with:
+          persist-credentials: true
+
+      - name: Checkout ref-slice fork semver break
+        uses: actions/checkout@v3
+        with:
+          persist-credentials: false
+          repository: 'mgr0dzicki/cargo-semver-action-ref-slice'
+          ref: 'major_change'
+          path: 'subject'
+
+      - name: Install rust
+        uses: actions-rs/toolchain@v1
+        with:
+          toolchain: stable
+          profile: minimal
+          override: true
+
+      - uses: Swatinem/rust-cache@v2
+        with:
+          key: ref-slice-fork
+
+      - name: Run semver-checks
+        continue-on-error: true
+        run: |
+          set -euo pipefail
+          cargo run semver-checks check-release --manifest-path="subject/Cargo.toml" 2>&1 | tee output
+          touch unexpectedly_did_not_fail
+
+      - name: Check whether it failed
+        run: |
+          if [ -f unexpectedly_did_not_fail ]; then exit 1; else exit 0; fi
+
+      - name: Baseline is correct
+        run: |
+          EXPECTED="$(echo -e "     Parsing ref_slice v1.2.2 (current)\n     Parsing ref_slice v1.2.1 (baseline)\n    Checking ref_slice v1.2.1 -> v1.2.2 (patch change)")"
+          # Strip ANSI escapes for colors and bold text before comparing.
+          RESULT="$(cat output | grep 'ref_slice v1.' | sed "s,\x1B\[[0-9;]*[a-zA-Z],,g")"
+          diff <(echo "$RESULT") <(echo "$EXPECTED")
+
+      - name: Semver break found
+        run: |
+          EXPECTED="$(echo -e "--- failure function_missing: pub fn removed or renamed ---")"
+          RESULT="$(cat output | grep failure)"
+          diff <(echo "$RESULT") <(echo "$EXPECTED")
+          # Ensure the following fragment (not full line!) is in the output file:
+          grep '  function ref_slice::ref_slice, previously in file' output
+
+      - name: Cleanup
+        run: |
+          rm output
+          rm -f unexpectedly_did_not_fail
+
   init-release:
     name: Run the release workflow
     needs:
@@ -556,6 +673,7 @@ jobs:
     needs:
       - ci-everything
       - should-publish
+      - run-on-tokio  # this check is too expensive on every PR, so we do it pre-publish instead
     if: needs.should-publish.outputs.is_new_version == 'yes'
     steps:
       - name: Checkout

diff --git a/.gitignore b/.gitignore
@@ -15,4 +15,4 @@ localdata/
 .vscode/
 
 # Unnecessary test data
-test_crates/*/*/Cargo.lock
+test_crates/**/Cargo.lock
diff --git a/src/lib.rs b/src/lib.rs
@@ -367,13 +367,13 @@ impl Check {
                         let crate_name = &selected.name;
                         let version = &selected.version;
 
-                        let is_implied = matches!(
-                            self.scope.mode,
-                            ScopeMode::DenyList(PackageSelection {
-                                selection: ScopeSelection::Workspace,
-                                ..
-                            })
-                        ) && selected.publish == Some(vec![]);
+                        // If the manifest we're using points to a workspace, then
+                        // ignore `publish = false` crates unless they are specifically selected.
+                        // If the manifest points to a specific crate, then check the crate
+                        // even if `publish = false` is set.
+                        let is_implied = matches!(self.scope.mode, ScopeMode::DenyList(..))
+                            && metadata.workspace_members.len() > 1
+                            && selected.publish == Some(vec![]);
                         if is_implied {
                             config.verbose(|config| {
                                 config.shell_status(
@@ -383,11 +383,6 @@ impl Check {
                             })?;
                             Ok(true)
                         } else {
-                            config.shell_status(
-                                "Parsing",
-                                format_args!("{crate_name} v{version} (current)"),
-                            )?;
-
                             let (current_crate, baseline_crate) = generate_versioned_crates(
                                 &mut config,
                                 &rustdoc_cmd,

diff --git a/src/main.rs b/src/main.rs
@@ -91,15 +91,15 @@ fn main() -> anyhow::Result<()> {
     }
 }
 
-#[derive(Parser)]
+#[derive(Debug, Parser)]
 #[command(name = "cargo")]
 #[command(bin_name = "cargo")]
 #[command(version, propagate_version = true)]
 enum Cargo {
     SemverChecks(SemverChecks),
 }
 
-#[derive(Args)]
+#[derive(Debug, Args)]
 #[command(args_conflicts_with_subcommands = true)]
 struct SemverChecks {
     #[arg(long, global = true, exclusive = true)]
@@ -119,13 +119,13 @@ struct SemverChecks {
 }
 
 /// Check your crate for semver violations.
-#[derive(Subcommand)]
+#[derive(Debug, Subcommand)]
 enum SemverChecksCommands {
     #[command(alias = "diff-files")]
     CheckRelease(CheckRelease),
 }
 
-#[derive(Args)]
+#[derive(Debug, Args)]
 struct CheckRelease {
     #[command(flatten, next_help_heading = "Current")]
     pub manifest: clap_cargo::Manifest,
@@ -216,15 +216,24 @@ impl From<CheckRelease> for cargo_semver_checks::Check {
         };
         let mut check = Self::new(current);
         if value.workspace.all || value.workspace.workspace {
+            // Specified explicit `--workspace` or `--all`.
             let mut selection = PackageSelection::new(ScopeSelection::Workspace);
             selection.with_excluded_packages(value.workspace.exclude);
             check.with_package_selection(selection);
         } else if !value.workspace.package.is_empty() {
+            // Specified explicit `--package`.
             check.with_packages(value.workspace.package);
+        } else if !value.workspace.exclude.is_empty() {
+            // Specified `--exclude` without `--workspace/--all`.
+            // Leave the scope selection to the default ("workspace if the manifest is a workspace")
+            // while excluding any specified packages.
+            let mut selection = PackageSelection::new(ScopeSelection::DefaultMembers);
+            selection.with_excluded_packages(value.workspace.exclude);
+            check.with_package_selection(selection);
         }
-        let baseline = {
+        let custom_baseline = {
             if let Some(baseline_version) = value.baseline_version {
-                Rustdoc::from_registry(baseline_version)
+                Some(Rustdoc::from_registry(baseline_version))
             } else if let Some(baseline_rev) = value.baseline_rev {
                 let root = if let Some(baseline_root) = value.baseline_root {
                     baseline_root
@@ -233,22 +242,22 @@ impl From<CheckRelease> for cargo_semver_checks::Check {
                 } else {
                     std::env::current_dir().expect("can't determine current directory")
                 };
-                Rustdoc::from_git_revision(root, baseline_rev)
+                Some(Rustdoc::from_git_revision(root, baseline_rev))
             } else if let Some(baseline_rustdoc) = value.baseline_rustdoc {
-                Rustdoc::from_path(baseline_rustdoc)
+                Some(Rustdoc::from_path(baseline_rustdoc))
             } else {
-                let root = if let Some(baseline_root) = value.baseline_root {
-                    baseline_root
-                } else {
-                    std::env::current_dir().expect("can't determine current directory")
-                };
-                Rustdoc::from_root(root)
+                // Either there's a manually-set baseline root path, or fall through
+                // to the default behavior.
+                value.baseline_root.map(Rustdoc::from_root)
             }
         };
-        check.with_baseline(baseline);
+        if let Some(baseline) = custom_baseline {
+            check.with_baseline(baseline);
+        }
         if let Some(log_level) = value.verbosity.log_level() {
             check.with_log_level(log_level);
         }
+
         check
     }
 }

diff --git a/src/rustdoc_gen.rs b/src/rustdoc_gen.rs
@@ -376,24 +376,24 @@ impl RustdocGenerator for RustdocFromProjectRoot {
         rustdoc_cmd: &RustdocCommand,
         crate_data: CrateDataForRustdoc,
     ) -> anyhow::Result<PathBuf> {
-        let manifest: &Manifest = self
-            .manifests
-            .get(crate_data.name)
-            .with_context(|| {
-                let errors = self
+        let manifest: &Manifest = self.manifests.get(crate_data.name).ok_or_else(|| {
+            let err = anyhow::anyhow!(
+                "package `{}` not found in {}",
+                crate_data.name,
+                self.project_root.display(),
+            );
+            if self.manifest_errors.is_empty() {
+                err
+            } else {
+                let cause_list = self
                     .manifest_errors
                     .values()
                     .map(|error| format!("  {error:#},"))
                     .join("\n");
-                format!("possibly due to errors: [\n{errors}\n]")
-            })
-            .with_context(|| {
-                format!(
-                    "package `{}` not found in {}",
-                    crate_data.name,
-                    self.project_root.display(),
-                )
-            })?;
+                let possible_causes = format!("possibly due to errors: [\n{cause_list}\n]");
+                err.context(possible_causes)
+            }
+        })?;
         generate_rustdoc(
             config,
             rustdoc_cmd,