Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly included packages with --package don't run with the --workspace flag when they are publish = false. #868

Open
suaviloquence opened this issue Aug 16, 2024 · 4 comments
Labels
C-bug Category: doesn't meet expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.

Comments

@suaviloquence
Copy link
Contributor

suaviloquence commented Aug 16, 2024

Steps to reproduce the bug with the above code

In pwd test_crates/manifest_tests/workspace_all_publish_false:

cargo semver-checks --manifest-path new --baseline-root old --workspace --package a --verbose

Package a (marked as publish = false in Cargo.toml) is explicitly included with the --package a flag.

Actual Behaviour

a is not semver-checked:

    Skipping a v0.1.0 (current)
    Skipping b v0.1.0 (current)

Expected Behaviour

a is semver-checked, similar to running the command without --workspace:

     Parsing a v0.1.0 (current)
      Parsed [   0.584s] (current)
     Parsing a v0.1.0 (baseline)
      Parsed [   0.558s] (baseline)
    Checking a v0.1.0 -> v0.1.0 (no change)
    Starting 80 checks, 0 unnecessary on 16 threads
        PASS [   0.001s]       major        auto_trait_impl_removed
--- snip ---
        PASS [   0.001s]       major        variant_marked_non_exhaustive
     Checked [   0.013s] 80 checks: 79 pass, 1 fail, 0 warn, 0 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/function_missing.ron

Failed in:
  function a::should_not_run, previously in file /home/m/dev/cargo-semver-checks/test_crates/manifest_tests/workspace_all_publish_false/old/a/src/lib.rs:4

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [   1.170s] a

while semver-checking the other crates in the workspace like --workspace does - in this example, with:

    Skipping b v0.1.0 (current)

since b is also marked as publish = false.

Generated System Information

Software version

cargo-semver-checks 0.34.0 (a0c0a37)

Operating system

Linux 6.10.3-zen1-2-zen

Command-line

/home/m/dev/cargo-semver-checks/target/debug/cargo-semver-checks semver-checks --bugreport

cargo version

> cargo -V
cargo 1.80.0 (376290515 2024-07-16)

Compile time information

  • Profile: debug
  • Target triple: x86_64-unknown-linux-gnu
  • Family: unix
  • OS: linux
  • Architecture: x86_64
  • Pointer width: 64
  • Endian: little
  • CPU features: fxsr,sse,sse2
  • Host: x86_64-unknown-linux-gnu

Build Configuration

No response

Additional Context

Originally in #866 (comment)

@suaviloquence suaviloquence added the C-bug Category: doesn't meet expectations label Aug 16, 2024
@obi1kenobi obi1kenobi added E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue. labels Aug 16, 2024
@suaviloquence
Copy link
Contributor Author

if value.workspace.all || value.workspace.workspace {
// Specified explicit `--workspace` or `--all`.
let mut selection = PackageSelection::new(ScopeSelection::Workspace);
selection.set_excluded_packages(value.workspace.exclude);
check.set_package_selection(selection);
} else if !value.workspace.package.is_empty() {
// Specified explicit `--package`.
check.set_packages(value.workspace.package);

The --package and --workspace flags are being handled as exclusive here, where --workspace takes precedence. Do we want to change this behavior?

@obi1kenobi
Copy link
Owner

Just to make sure, let's check with @epage. Context for you, Ed: some of our users expressed confusion that:

  • If using --workspace, the --package flag is silently ignored.
  • If they have a publish = false package in a workspace, attempting to scan it with --workspace --package X still results in the package being skipped. Normally, --package X means the package gets scanned even if it's publish = false.

This behavior seems suboptimal no matter how we interpret the meaning of the flags.

What behavior would you recommend so that we stay as close as possible to cargo? Should we error out if a --workspace --package combo is used, or obey --package and scan the package despite its publish = false?

@suaviloquence
Copy link
Contributor Author

There is also the --all flag which is currently equivalent to --workspace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: doesn't meet expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.
Projects
None yet
Development

No branches or pull requests

3 participants