rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.29k stars 2.32k forks source link

`cargo vendor --sync` fails inconsistently, depending on workspaces used or not. #13588

Open msirringhaus opened 3 months ago

msirringhaus commented 3 months ago

Problem

cargo vendor fails, if it is called from inside a repository, and --sync is used to point to another repository outside. This only fails, if one of the repos is using a workspace, while it works if no workspace is present.

As examples, I used these three small crates to reproduce: cbindgen, rkv, and wardstone. Only wardstone uses a workspace.

The error-message is

error: current package believes it's in a workspace when it's not:
current:   sources/wardstone/../rkv/Cargo.toml
workspace: sources/wardstone/Cargo.toml

this may be fixable by adding `../rkv` to the `workspace.members` array of the manifest located at: sources/wardstone/Cargo.toml
Alternatively, to keep it out of the workspace, add the package to the `workspace.exclude` array, or add an empty `[workspace]` table to the package's manifest.

Steps

  1. mkdir sources && cd sources
  2. git clone https://github.com/mozilla/cbindgen && git clone https://github.com/mozilla/rkv && git clone https://github.com/tshakalekholoane/wardstone
  3. Execute operations as below:
    CWD         | cmd                                                               | success?
    ./          | vendor --manifest-path rkv/Cargo.toml --sync cbindgen/Cargo.toml  | Yes
    ./          | vendor --manifest-path rkv/Cargo.toml --sync wardstone/Cargo.toml | Yes
    ./rkv/      | vendor --sync ../cbindgen/Cargo.toml                              | Yes
    ./rkv/      | vendor --manifest-path Cargo.toml --sync ../cbindgen/Cargo.toml   | Yes
    ./rkv/      | vendor --sync ../wardstone/Cargo.toml                             | No
    ./rkv/      | vendor --manifest-path Cargo.toml --sync ../wardstone/Cargo.toml  | No
    ./wardstone/| vendor --sync ../rkv/Cargo.toml                                   | No
    ./wardstone/| vendor --manifest-path Cargo.toml --sync ../rkv/Cargo.toml        | No

Vendoring rkv+cbindgen from inside ./rkv works fine. But vendoring rkv+wardstone only works from the top-level dir, and fails otherwise.

Possible Solution(s)

cargo vendor bails in these two functions: https://github.com/rust-lang/cargo/blob/master/src/cargo/core/workspace.rs#L840-L841 (self.error_if_manifest_not_in_members() only, if self.validate_members() is commented out, obviously).

That function doesn't distinguish between vendoring and other operations.

Notes

I am not 100% certain, if this is a bug or expected behavior. But I do feel that the results are inconsistent. Either --sync should never allow to vendor from inside a repository, or it should also work with repos using workspaces.

I'd be willing to look into this, once it's clear what the desired behavior is.

Version

cargo 1.76.0 (c84b36747 2024-01-18)
release: 1.76.0
commit-hash: c84b367471a2db61d2c2c6aab605b14130b8a31b
commit-date: 2024-01-18
host: x86_64-unknown-linux-gnu
libgit2: 1.7.1 (sys:0.18.1 vendored)
libcurl: 8.5.0-DEV (sys:0.4.70+curl-8.5.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: openSUSE 20240310 [64-bit]
weihanglo commented 1 month ago

Thanks for the report! And really sorry for the late response.

Haven't got more time looking into this, but found more inconsistencies:

vendor --manifest-path wardstone/Cargo.toml --sync rkv/Cargo.toml # ❌
vendor --manifest-path wardstone/Cargo.toml --sync cbindgen/Cargo.toml # ✅
vendor --manifest-path cbindgen/Cargo.toml --sync wardstone/Cargo.toml # ❌
vendor --manifest-path cbindgen/Cargo.toml --sync rkv/Cargo.toml # ✅

Though, the errors from above are different from the “current package believes it's in a workspace” one. It might be that the paths were not normalized (not calling paths::normalize), so the comparison failed. I forgot the reason Cargo chose not to normalize paths for workspace members.

or it should also work with repos using workspaces.

I feel like this should be supported.