Avoid churning unrelated dependency edges

[konstin]

View all comments

When updating a dependency, its dependencies get recursively unlocked, while preferring lockfile versions. The edges however were not preserved, causing unrelated dependency edges to toggle between multiple compatible versions available in the lockfile (#5529).

foo
├── shared =0.1.0
└── c 1.0.0
    └── b 1.0.0
        └── shared >=0.1,<0.3 # Locked to 0.2.0

In the example above, cargo update -p c --precise 1.0.1 would change the version of shared that b uses to 0.1.0. This causes churn when updating dependency as packages with multiple versions in the lockfile and wide ranges, such as the windows-* crates, toggle back and forth through unrelated updates.

This PR adds tracking of preferences for each package id, so b 1.0.0 tracks a preference for shared 0.2.0 specifically, which is set as version to try first. We introduce a third mode between locked and skipped dependencies in register_previous_locks: Packages such as b 1.0.0 remain unlocked, but aren't skipped so that their dependency edges remain tracked.

Fixes #5529

[rustbot]

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ehuss, @epage, @weihanglo
• @ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
• Random selection from ehuss, epage, weihanglo

[epage]

We've generally found that isolating commits makes reviewing easier and faster. In a PR touching tricky areas, like the resolver, it becomes even more important to make the actual change easier to see, now and in the future.

As for the change itself, I would definitely want @Eh2406 to look over it. I called out some vague thoughts but I'm not familiar enough to be more concrete and worry I'm missing some subtle interaction.

View changes since this review

[epage]

This signature feels hard to understand and get right but unsure if there is a better way.

[konstin]

We could rename deps: Vec<PackageId> into locked_deps: Vec<PackageId> here and in LockedPackage, would that help?

[Eh2406]

As for the change itself, I would definitely want @Eh2406 to look over it. I called out some vague thoughts but I'm not familiar enough to be more concrete and worry I'm missing some subtle interaction.

If it works I'm not going to argue with it too much. But this definitely makes me nervous. I worry that there is some scenario in which this ends up picking the existing version before it evaluates the update, and then the update cannot succeed because it's incompatible with that selected version, and instead of backtracking on our lock/preferred we end up backtracking on update. But having articulated that, the order of Package selection is not dependent on this change and the existing packages are already preferred. So I don't see how this problematic case could happen. So 🤷

[konstin]

I found a case this PR regresses based on @Eh2406's comment: If we have root -> A, root -> B, and an A -> S 1.0 edge that's resolved before the dependency update, an update of B may do nothing if it requires S 1.1 as S was then already locked to 1.1. This is a tradeoff in the way update are currently done (packages are either locked or unlocked, with this PR adding preferences, but there's no backtracking if we discover that something blocks an update, and we want that update), we trade a more stable lockfile for imprecise updates doing nothing. This doesn't block precise updates however, as the preference edges don't block backtracking. A hacky solution would be restarting the resolution with less preferences if detect this scenario, but that sounds way worse for general resolver understandability.

The current state the update resolver "over-changes", it causes churn in precise updates, while this version slightly "under-changes": The resolver sometimes doesn't do updates when they'd be possible, unless you force them with --precise.

[Eh2406]

If we can build a regression test for that case, I would love to have it added to our code base.

I wonder if there's some way to change https://github.com/rust-lang/cargo/blob/master/src/cargo/core/resolver/types.rs#L299 so that the package we are trying to update is picked earlier in the decision tree. That might help mitigate this problem.

[konstin]

I added a test in 824662c. In uv, changing the order of packages could change the whole resolution due to forks and such, but if cargo is resilient to order change with a preexisting lockfile, we could try that.

[Eh2406]

I mean, it could theoretically have cascading failures that significantly change resolution. But between locking and preferences and this new kind of preferences, I don't think it's very likely in practice. And unexpected unrelated changes is the status quo. So it seems worth a try.