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.

Sign up for GitHub

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

Jump to bottom

Don't squash path dependencies with same package name #13863

Closed
wants to merge 2 commits into from
Closed

Don't squash path dependencies with same package name #13863

wants to merge 2 commits into from

Conversation

ldm0
Copy link

@ldm0 ldm0 commented May 4, 2024

Fixes #13862

Dependencies with same package name in the Cargo.lock was squashed in EncodableResolve::into_resolve().

e.g. In the problem reproduction repo's Cargo.lock, there are two entries:

[[package]]
name = "once_cell"
version = "0.1.0"

[[package]]
name = "once_cell"
version = "1.19.0"

However, they were squashed in build_path_deps(), then we endup getting two package ids with same path like this:

PackageId { name: "once_cell", version: "0.1.0", source: "pwd/vendor/once_cell_1_19" }
PackageId { name: "once_cell", version: "1.19.0", source: "pwd/vendor/once_cell_1_19" }

This package version inconsistency triggers patch update introduced in #8248 everytime you compile this crate.

@rustbot
Copy link
Collaborator

rustbot commented May 4, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-dependency-resolution Area: dependency resolution and the resolver A-lockfile Area: Cargo.lock issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2024
ldm0
ldm0 commented May 4, 2024
View reviewed changes
src/cargo/core/resolver/encode.rs
.find(|package_id| {
package_id.version().cmp_precedence(&pkg_version) == std::cmp::Ordering::Equal
})
.or_else(|| versions.last())
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use last() for behaviour consistency with old cargo. Old cargo use HashMap and insert for path deps construction, therefore the last inserted package was picked.

@ldm0 ldm0 force-pushed the ldm_path_dep_fix branch 2 times, most recently from 4787160 to b451f15 Compare May 4, 2024 23:51
@epage
Copy link
Contributor

epage commented May 6, 2024

Note: I just did a quick scan of the issue (and commit structure). I've not looked at the test or code change in detail yet

@ldm0 ldm0 force-pushed the ldm_path_dep_fix branch 2 times, most recently from feb39cf to 6ef2209 Compare May 6, 2024 04:58
@ldm0
Copy link
Author

ldm0 commented May 6, 2024

It seems that CI failure is caused by check-cfg https://github.com/rust-lang/cargo/actions/runs/8964218728/job/24615675343?pr=13863
Related: #13571

@ldm0
Copy link
Author

ldm0 commented May 18, 2024

ping @weihanglo

@bors
Copy link
Collaborator

bors commented May 18, 2024

☔ The latest upstream changes (presumably #13931) made this pull request unmergeable. Please resolve the merge conflicts.

@ldm0 ldm0 closed this May 19, 2024
@weihanglo
Copy link
Member

Sorry for the late reply. Thought epage was on it. Feel free to open a new one whenever ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@weihanglo weihanglo

Labels
A-dependency-resolution Area: dependency resolution and the resolver A-lockfile Area: Cargo.lock issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unnecessary index fetching upon patching two dependencies with same package name
5 participants
@ldm0 @rustbot @epage @bors @weihanglo