-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Build by PackageIdSpec, not name, to avoid ambiguity #12015
Conversation
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
Ah, drat, I neglected to retry the complete test suite after this change. It would seem the full pkg spec does not work in some cases, for some reason. |
c574ca9
to
9320d91
Compare
Ok, it broke for cargo/src/cargo/ops/cargo_install.rs Lines 187 to 193 in de80432
So cargo install tracking is trying to retain the source git url, but for the build spec we need to unconditionally use So just a little code juggling to fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix. Could you add a test like path_install_workspace_root_despite_default_members
but for git dependency? I feel like we missed that one.
r? @weihanglo |
@rustbot author |
9320d91
to
79bb2d7
Compare
I've added two more test cases: the one requested, and another slightly less recursive one, where the workspace contains both a package |
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! Just one question. Other parts look pretty great!
Pardon my correcting the term in a test name. And thank you so much for taking care of this issue! @bors r+ |
☀️ Test successful - checks-actions |
Update cargo 10 commits in ac84010322a31f4a581dafe26258aa4ac8dea9cd..569b648b5831ae8a515e90c80843a5287c3304ef 2023-05-02 13:41:16 +0000 to 2023-05-05 15:49:44 +0000 - xtask-unpublished: output a markdown table (rust-lang/cargo#12085) - fix: hack around `libsysroot` instead of `libtest` (rust-lang/cargo#12088) - Optimize usage under rustup. (rust-lang/cargo#11917) - Update lock to normalize `home` dep (rust-lang/cargo#12084) - fix: doc-test failures (rust-lang/cargo#12055) - feat(cargo-metadata): add `workspace_default_members` (rust-lang/cargo#11978) - doc: clarify implications of `cargo-yank` (rust-lang/cargo#11862) - chore: Use `[workspace.dependencies]` (rust-lang/cargo#12057) - support for shallow clones and fetches with `gitoxide` (rust-lang/cargo#11840) - Build by PackageIdSpec, not name, to avoid ambiguity (rust-lang/cargo#12015) r? `@ghost`
What does this PR try to resolve?
Fixes #11999
I previously added this code to ensure
cargo install
will build the intended package, but it turns out to cause ambiguity in the case where a package depends on prior versions of itself.This ambiguity can be resolved by identifying the package to build by its pkg-spec, not name.
How should we test and review this PR?
A test case is included.
I have additionally manually tested that, with this patch, the issue in #11999 is fixed and now installs correctly.