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.

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

fix(update): don't reuse a previously locked dependency if multiple major versions are compatible #14582

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

stormshield-kg
Copy link

What does this PR try to resolve?

Fixes #5529.
Fixes #14115.
Fixes #14446.

How should we test and review this PR?

This first commit adds a test documenting the current behavior, and the second commit implements the fix and updates the test.

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Sep 23, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (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 A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2024
@epage
Copy link
Contributor

epage commented Sep 23, 2024

As a heads up, none of these issues are marked as A-accepted but instead S-needs-design (see Issues in contrib docs). It would generally be best to explore the high level ideas in the Issue before moving on to a PR (even if playing with an implementation locally) so we keep the solution space exploration in one place and leave the PR to focus on the implementation. Sometimes PRs come and go for the same issue and if we explore designs in PRs, it fractures the conversation.

@Eh2406 can better speak to whether we can shortcircuit that and move forward with this.

r? @Eh2406

btw another issue with multi-major version reqs is #9029 with #13535 as a proposed change for it.

@rustbot rustbot assigned Eh2406 and unassigned epage Sep 23, 2024
Comment on lines +67 to +72

/// Get version preferences.
fn version_prefs(&self) -> &VersionPreferences;

/// Set version preferences.
fn set_version_prefs(&mut self, version_prefs: VersionPreferences);
Copy link
Contributor

Choose a reason for hiding this comment

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

For myself, I feel uncomfortable how much resolver logic lives in PackageRegistry, I feel even more uncomfortable adding VersionPreferences to it and to trait Registry as well.

@bors
Copy link
Collaborator

bors commented Sep 24, 2024

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

// to do the full resolving instead of locking a specific version.
// Otherwise, we query all known locked packages to see if they match this dependency,
// and if anything does then we lock it to that and move on.

Copy link
Contributor

Choose a reason for hiding this comment

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

My instinct has long been that this heuristic should just be removed. Apparently I tried that when I was a little baby contributor new to open source in #5718. A lot has changed since then, so it might work now.

Another approach discussed in that PR is to remove this heuristic and double our calls to resolve. We try resolving with --offline and only try normal resolution after the --offline did not work. But now that I say that (I think, without having try it), that would lead to cargo update randomly updating unrelated GIT dependencies. In addition to the performance impacts.

Copy link
Contributor

Choose a reason for hiding this comment

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

After some more investigation, this heuristic seems to be loadbearing for cleaning up different ways to refer to the same git package. For example a dependency might require the default branch (or the master branch) but what's available in the lock file is a particular checkout. This heuristic identifies that the particular hash could match and uses it. This seems accidental to me. I think there must be a better way of handling this. If we had a matches_with_compatible_git_source when iterating over locked_deps things could be solved much more elegantly. In the time I had available I did not find one laying around, but this still seems a promising direction.

Copy link
Author

@stormshield-kg stormshield-kg Sep 27, 2024

Choose a reason for hiding this comment

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

This test checks this heuristic and is not related to git dependencies:

fn add_dep_dont_update_registry_http() {

So passing a matches_with_compatible_git_source parameter will not be enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct. I forgot to mention that case. That test was added at the same time as the heuristic. Based on the PR that added it was to allow use of cargo with fewer network requests. This was a big issue at the time because we did not yet have --offline. So if cargo decided that you needed a network request and you did not have network access you were stuck. I have not formally discussed it with the team, but my vote would be that we just remove that test.

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 A-lockfile Area: Cargo.lock issues A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
5 participants