cargo-semver-checks icon indicating copy to clipboard operation
cargo-semver-checks copied to clipboard

can fail when projects of same name are in a subdirectory

Open ehuss opened this issue 2 years ago • 7 comments

Steps to reproduce the bug with the above code

  1. git clone https://github.com/rust-lang/mdbook.git
  2. cd mdbook
  3. git worktree add mdbook-0.2 v0.2.0
  4. cargo semver-checks check-release

Actual Behaviour

    Updating index
     Parsing mdbook v0.2.0 (current)
Error: `cargo metadata` exited with an error:     Updating crates.io index
error: failed to select a version for the requirement `ammonia = "^1.1"`
candidate versions found which didn't match: 3.3.0, 3.2.1, 3.2.0, ...
location searched: crates.io index
required by package `mdbook v0.2.0 (/Users/eric/Temp/z10/mdbook/mdbook-0.2)`
    ... which satisfies path dependency `mdbook` of package `rustdoc v0.0.0 (/Users/eric/Temp/z10/mdbook/target/semver-checks/local-mdbook-0_2_0)`
perhaps a crate was updated and forgotten to be re-vendored?

Expected Behaviour

It should run the semver checks for the package in the current directory. I would not expect it to go looking outside of the current directory for the project. It should follow the same behavior as cargo for finding the package to use.

(I likely don't know why it is walking the directory, but it is not clear to me why it would behave differently from other cargo commands.)

Generated System Information

Software version

cargo-semver-checks 0.20.1

Operating system

macOS 13.4 (Darwin 22.5.0)

Command-line

/Users/eric/.cargo/bin/cargo-semver-checks semver-checks --bugreport

cargo version

> cargo -V
cargo 1.71.0-nightly (09276c703 2023-05-16)

Compile time information

  • Profile: release
  • Target triple: aarch64-apple-darwin
  • Family: unix
  • OS: macos
  • Architecture: aarch64
  • Pointer width: 64
  • Endian: little
  • CPU features: aes,crc,dit,dotprod,dpb,dpb2,fcma,fhm,flagm,fp16,frintts,jsconv,lor,lse,neon,paca,pacg,pan,pmuv3,ras,rcpc,rcpc2,rdm,sb,sha2,sha3,ssbs,v8.1a,v8.2a,v8.3a,v8.4a,vh
  • Host: aarch64-apple-darwin

Build Configuration

No response

Additional Context

No response

ehuss avatar May 30 '23 21:05 ehuss

Thanks for the bug report.

Triaged the bug down to this block. Here's roughly what's happening:

  • no explicitly specified Cargo.toml, so attempt to find crates to check by looking for Cargo.toml files within the directory
  • crates ["mdbook-wordcount", "mdbook"] found, but the worktree causes them to be inserted twice each (once for the main repo and once for the worktree)
  • the later inserts (worktree) clobber the earlier ones

In retrospect this is clearly buggy.

How do cargo commands figure out the package or workspace a command applies to?

obi1kenobi avatar May 30 '23 22:05 obi1kenobi

It's a bit complicated, but without -p or --manifest-path, it looks for the first Cargo.toml starting in the current directory and walks upwards. It can be a little complicated with things like workspaces with default members and such. The linked code seems to be walk down the tree to find all Cargo.toml files, instead of up, so I'm a bit unclear why it takes that approach.

I think perhaps cargo locate-project can be used to find the first manifest. It would still need to handle virtual workspaces, and things like default-members to be consistent with cargo's behavior. I don't think there is a way to easily ask cargo what it's "default" match will be. Another alternative is cargo pkgid which will usually work except for the root of a virtual workspace, in which case you have to read the workspace_default_members from cargo metadata.

I think I would recommend something like:

  1. Run cargo metadata
  2. If -p is used, select packages from packages that match that. (I don't think there is an easy way to simulate cargo's glob matching, unfortunately.)
  3. If --workspace is used, select packages from workspace_members.
  4. If CWD is equal to workspace_root, select packages from workspace_default_members.
  5. Else, run cargo locate-project to figure out which package is "closest", and match that with packages.manifest_path in the metadata. (Alternatively, just walk upwards to find the first Cargo.toml instead of using locate-project.)

ehuss avatar May 31 '23 02:05 ehuss

Thanks for the awesome explanation and suggestions. Much appreciated! I'll look into the approach you recommended.

obi1kenobi avatar May 31 '23 04:05 obi1kenobi