Skip to content

Build by PackageIdSpec, not name, to avoid ambiguity#12015

Merged
bors merged 4 commits intorust-lang:masterfrom
tedinski:fix-11999-recursive-deps
May 3, 2023
Merged

Build by PackageIdSpec, not name, to avoid ambiguity#12015
bors merged 4 commits intorust-lang:masterfrom
tedinski:fix-11999-recursive-deps

Conversation

@tedinski
Copy link
Contributor

@tedinski tedinski commented Apr 21, 2023

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.

@rustbot
Copy link
Collaborator

rustbot commented Apr 21, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added Command-install S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2023
@tedinski
Copy link
Contributor Author

tedinski commented Apr 21, 2023

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.

@tedinski tedinski force-pushed the fix-11999-recursive-deps branch from c574ca9 to 9320d91 Compare April 21, 2023 18:51
@tedinski
Copy link
Contributor Author

tedinski commented Apr 21, 2023

Ok, it broke for --git and this make a bit more sense a few lines down:

cargo/src/cargo/ops/cargo_install.rs

Lines 187 to 193 in de80432

let pkg = if source_id.is_git() {
// Don't use ws.current() in order to keep the package source as a git source so that
// install tracking uses the correct source.
pkg
} else {
ws.current()?.clone()
};

So cargo install tracking is trying to retain the source git url, but for the build spec we need to unconditionally use ws.current() to correctly address the checked-out url (path) of the package we're trying to build within the workspace.

So just a little code juggling to fix.

weihanglo
weihanglo reviewed Apr 23, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a 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.

@ehuss
Copy link
Contributor

ehuss commented Apr 25, 2023

r? @weihanglo

@rustbot rustbot assigned weihanglo and unassigned ehuss Apr 25, 2023
@weihanglo
Copy link
Member

weihanglo commented Apr 25, 2023

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2023
@tedinski tedinski force-pushed the fix-11999-recursive-deps branch from 9320d91 to 79bb2d7 Compare May 1, 2023 22:23
@tedinski
Copy link
Contributor Author

tedinski commented May 1, 2023

I've added two more test cases: the one requested, and another slightly less recursive one, where the workspace contains both a package foo and a dependency on a registry package foo (which may just have colliding names, and not really be "recursive".)

@tedinski
Copy link
Contributor Author

tedinski commented May 1, 2023

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels May 1, 2023
weihanglo
weihanglo reviewed May 2, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a 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!

@weihanglo
Copy link
Member

weihanglo commented May 3, 2023

Pardon my correcting the term in a test name. And thank you so much for taking care of this issue!

@bors r+

@bors
Copy link
Contributor

bors commented May 3, 2023

📌 Commit 7fb35c9 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2023
@bors
Copy link
Contributor

bors commented May 3, 2023

⌛ Testing commit 7fb35c9 with merge a9465fe...

@bors
Copy link
Contributor

bors commented May 3, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing a9465fe to master...

@bors bors merged commit a9465fe into rust-lang:master May 3, 2023
@weihanglo weihanglo mentioned this pull request May 5, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2023
@bors
Auto merge of rust-lang#111258 - weihanglo:update-cargo, r=weihanglo
f9a6b71 
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`
@ehuss ehuss added this to the 1.71.0 milestone May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels

Command-install S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.71.0

Development

Successfully merging this pull request may close these issues.

cargo install fails when package uses previous version of its own to build itself

5 participants

@tedinski @rustbot @ehuss @weihanglo @bors