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

can fail when projects of same name are in a subdirectory #462

Open
ehuss opened this issue May 30, 2023 · 7 comments
Open

can fail when projects of same name are in a subdirectory #462

ehuss opened this issue May 30, 2023 · 7 comments
Labels
C-bug Category: doesn't meet expectations

Comments

@ehuss
Copy link

ehuss commented May 30, 2023

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 ehuss added the C-bug Category: doesn't meet expectations label May 30, 2023
@obi1kenobi
Copy link
Owner

obi1kenobi commented May 30, 2023

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?

@ehuss
Copy link
Author

ehuss commented May 31, 2023

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.)

@obi1kenobi
Copy link
Owner

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

@dmatos2012

This comment was marked as outdated.

@obi1kenobi

This comment was marked as outdated.

@dmatos2012

This comment was marked as outdated.

@obi1kenobi
Copy link
Owner

There's still an issue here, even though the original repro didn't work.

I have ameliorated the situation somewhat, in that instead of corrupting our data and possibly throwing an obscure/unrelated error, c-s-c will now explicitly check if the same package name is used in multiple manifests within the same directory (possibly in multiple workspaces): #887

I don't consider this issue resolved, since c-s-c still fails if projects of the same name are in the same directory tree. A full solution will require a change in semantics and the use of cargo locate-project as @ehuss' earlier post described.

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
Projects
None yet
Development

No branches or pull requests

3 participants