diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fa13d5e4..4ddb297a 100644 --- 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 index 8fae43e1..b32f48c2 100644 --- 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 index e9fa241e..6163a633 100644 --- 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 index d15407c7..986e3ebe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -91,7 +91,7 @@ fn main() -> anyhow::Result<()> { } } -#[derive(Parser)] +#[derive(Debug, Parser)] #[command(name = "cargo")] #[command(bin_name = "cargo")] #[command(version, propagate_version = true)] @@ -99,7 +99,7 @@ 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 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 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 index 7cc62477..8490e32a 100644 --- 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 { - 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,