Skip to content

Conversation

@Jackhr-arch
Copy link

@Jackhr-arch Jackhr-arch commented Dec 20, 2025

What does this PR try to resolve?

it changes the parts of the resolver that generate Cargo.lock to not include weak dependencies.

Instead of treating weak feature as strong one, now resolver keeps the weak dependency features until the very dependency is checked. Then it combines the weak features with the original ones.

Related to #10801

How to test and review this PR?

I've enabled some tests in crates/resolver-tests relating to Weak dependencies and looks they work fine.

Here is an example repo https://github.com/Jackhr-arch/cargo-weak-dep-feat-example

@rustbot rustbot added A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 20, 2025

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@Jackhr-arch Jackhr-arch marked this pull request as draft December 21, 2025 14:52
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 21, 2025
@Jackhr-arch Jackhr-arch marked this pull request as ready for review December 21, 2025 14:59
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 21, 2025
@Jackhr-arch

This comment has been minimized.

@epage
Copy link
Contributor

epage commented Dec 22, 2025

I won't be up for reviewing the core part of this, so my comments are a pre-review.

Please clean up commits for how they should be reviewed and merged.

r? @Eh2406

@rustbot rustbot assigned Eh2406 and unassigned weihanglo Dec 22, 2025
@est31
Copy link
Member

est31 commented Dec 23, 2025

I haven't done a review of the approach, I wonder whether this supports the weak_namespaced_v4 test from my earlier PR, or if it fails similarly (see ehuss comment and discussion immediately below).

@Jackhr-arch
Copy link
Author

Jackhr-arch commented Dec 24, 2025

I wonder whether this supports the weak_namespaced_v4 test from my earlier PR

It passed. I port 4 tests from your pull request, and deferred_v5 failed. I'm trying to fix this.

EDIT: it's now done.

@Jackhr-arch Jackhr-arch force-pushed the weak_dep_in_lock branch 3 times, most recently from 53f926c to 40618b9 Compare December 25, 2025 07:54
@rustbot
Copy link
Collaborator

rustbot commented Dec 25, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@epage
Copy link
Contributor

epage commented Dec 26, 2025

Please clean up commits for how they should be reviewed and merged.

Thank you for cleaning up your commits!

btw something we've found helpful is to add all tests in one or more commits before the fix, with them passing (showing the existing behavior). The commit that introduces the change then updates the test. The diff of the commits then helps to show how behavior changed. See https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request

@Jackhr-arch Jackhr-arch marked this pull request as draft December 29, 2025 08:30
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 29, 2025
@Jackhr-arch Jackhr-arch marked this pull request as ready for review January 27, 2026 15:18
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2026
@epage
Copy link
Contributor

epage commented Jan 27, 2026

image

Note that we expect commits to be atomic which includes tests are passing. Please squash the test update commits into the associated feature work commit.

Comment on lines 723 to 724
// bar is inside lockfile.
assert_contains!(lockfile, r#"version = 4"#, r#"name = "bar""#);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend following the existing testing patterns, rather than creating new ones. We tend to use snapshot tests of the complete output.

Copy link
Author

Choose a reason for hiding this comment

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

Weak dependency feature related packages usually only appear in Cargo.lock and [Downloading], cargo tree or cargo check won't show they are here or not, thus I read Cargo.lock.

Anyway, I've changed to snapshot.

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

Labels

A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants