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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion crates/resolver-tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ pub fn resolve_with_global_context_raw(
struct MyRegistry<'a> {
list: &'a [Summary],
used: HashSet<PackageId>,
version_prefs: VersionPreferences,
}
impl<'a> Registry for MyRegistry<'a> {
fn query(
Expand Down Expand Up @@ -135,6 +136,14 @@ pub fn resolve_with_global_context_raw(
fn block_until_ready(&mut self) -> CargoResult<()> {
Ok(())
}

fn version_prefs(&self) -> &VersionPreferences {
&self.version_prefs
}

fn set_version_prefs(&mut self, version_prefs: VersionPreferences) {
self.version_prefs = version_prefs;
}
}
impl<'a> Drop for MyRegistry<'a> {
fn drop(&mut self) {
Expand All @@ -157,6 +166,7 @@ pub fn resolve_with_global_context_raw(
let mut registry = MyRegistry {
list: registry,
used: HashSet::new(),
version_prefs: VersionPreferences::default(),
};
let summary = Summary::new(
pkg_id("root"),
Expand All @@ -172,11 +182,11 @@ pub fn resolve_with_global_context_raw(
if gctx.cli_unstable().minimal_versions {
version_prefs.version_ordering(VersionOrdering::MinimumVersionsFirst)
}
registry.set_version_prefs(version_prefs);
let resolve = resolver::resolve(
&[(summary, opts)],
&[],
&mut registry,
&version_prefs,
ResolveVersion::with_rust_version(None),
Some(gctx),
);
Expand Down
71 changes: 62 additions & 9 deletions src/cargo/core/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use std::collections::{HashMap, HashSet};
use std::task::{ready, Poll};

use crate::core::resolver::VersionPreferences;
use crate::core::PackageSet;
use crate::core::{Dependency, PackageId, SourceId, Summary};
use crate::sources::config::SourceConfigMap;
Expand Down Expand Up @@ -63,6 +64,12 @@ pub trait Registry {

/// Block until all outstanding [`Poll::Pending`] requests are [`Poll::Ready`].
fn block_until_ready(&mut self) -> CargoResult<()>;

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

/// Set version preferences.
fn set_version_prefs(&mut self, version_prefs: VersionPreferences);
Comment on lines +67 to +72
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.

}

/// This structure represents a registry of known packages. It internally
Expand Down Expand Up @@ -131,6 +138,9 @@ pub struct PackageRegistry<'gctx> {
/// This is constructed during calls to [`PackageRegistry::patch`],
/// along with the `patches` field, thoough these entries never get locked.
patches_available: HashMap<CanonicalUrl, Vec<PackageId>>,

/// Version preferences.
version_prefs: VersionPreferences,
}

/// A map of all "locked packages" which is filled in when parsing a lock file
Expand Down Expand Up @@ -209,6 +219,7 @@ impl<'gctx> PackageRegistry<'gctx> {
patches: HashMap::new(),
patches_locked: false,
patches_available: HashMap::new(),
version_prefs: VersionPreferences::default(),
})
}

Expand Down Expand Up @@ -503,7 +514,12 @@ impl<'gctx> PackageRegistry<'gctx> {
for summaries in self.patches.values_mut() {
for summary in summaries {
debug!("locking patch {:?}", summary);
*summary = lock(&self.locked, &self.patches_available, summary.clone());
*summary = lock(
&self.locked,
&self.patches_available,
summary.clone(),
&self.version_prefs,
);
}
}
self.patches_locked = true;
Expand Down Expand Up @@ -581,7 +597,12 @@ impl<'gctx> PackageRegistry<'gctx> {
/// through.
pub fn lock(&self, summary: Summary) -> Summary {
assert!(self.patches_locked);
lock(&self.locked, &self.patches_available, summary)
lock(
&self.locked,
&self.patches_available,
summary,
&self.version_prefs,
)
}

fn warn_bad_override(
Expand Down Expand Up @@ -734,7 +755,12 @@ impl<'gctx> Registry for PackageRegistry<'gctx> {
}
}
let summary = summary.into_summary();
f(IndexSummary::Candidate(lock(locked, all_patches, summary)))
f(IndexSummary::Candidate(lock(
locked,
all_patches,
summary,
&self.version_prefs,
)))
};
return source.query(dep, kind, callback);
}
Expand Down Expand Up @@ -799,13 +825,22 @@ impl<'gctx> Registry for PackageRegistry<'gctx> {
}
Ok(())
}

fn version_prefs(&self) -> &VersionPreferences {
&self.version_prefs
}

fn set_version_prefs(&mut self, version_prefs: VersionPreferences) {
self.version_prefs = version_prefs;
}
}

/// See [`PackageRegistry::lock`].
fn lock(
locked: &LockedMap,
patches: &HashMap<CanonicalUrl, Vec<PackageId>>,
summary: Summary,
version_prefs: &VersionPreferences,
) -> Summary {
let pair = locked
.get(&(summary.source_id(), summary.name()))
Expand Down Expand Up @@ -892,12 +927,30 @@ fn lock(
}
}

// If this dependency did not have a locked version, then we query
// all known locked packages to see if they match this dependency.
// If anything does then we lock it to that and move on.
let v = locked
.get(&(dep.source_id(), dep.package_name()))
.and_then(|vec| vec.iter().find(|&&(id, _)| dep.matches_id(id)));
// If this dependency did not have a locked version, we check if multiple
// versions of the dependency already exists in the lockfile. In this case we want
// 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.

let compatible_versions_count = version_prefs
.get_try_to_use()
.iter()
.filter(|&&id| {
id.source_id() == dep.source_id()
&& id.name() == dep.package_name()
&& dep.matches_id(id)
})
.count();

let v = if compatible_versions_count > 1 {
None
} else {
locked
.get(&(dep.source_id(), dep.package_name()))
.and_then(|vec| vec.iter().find(|&&(id, _)| dep.matches_id(id)))
};

if let Some(&(id, _)) = v {
trace!("\tsecond hit on {}", id);
let mut dep = dep;
Expand Down
8 changes: 3 additions & 5 deletions src/cargo/core/resolver/dep_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use crate::core::resolver::errors::describe_path_in_context;
use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet};
use crate::core::resolver::{
ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts, VersionOrdering,
VersionPreferences,
};
use crate::core::{
Dependency, FeatureValue, PackageId, PackageIdSpec, PackageIdSpecQuery, Registry, Summary,
Expand All @@ -32,7 +31,6 @@ use tracing::debug;
pub struct RegistryQueryer<'a> {
pub registry: &'a mut (dyn Registry + 'a),
replacements: &'a [(PackageIdSpec, Dependency)],
version_prefs: &'a VersionPreferences,
/// a cache of `Candidate`s that fulfil a `Dependency` (and whether `first_version`)
registry_cache: HashMap<(Dependency, Option<VersionOrdering>), Poll<Rc<Vec<Summary>>>>,
/// a cache of `Dependency`s that are required for a `Summary`
Expand All @@ -52,12 +50,10 @@ impl<'a> RegistryQueryer<'a> {
pub fn new(
registry: &'a mut dyn Registry,
replacements: &'a [(PackageIdSpec, Dependency)],
version_prefs: &'a VersionPreferences,
) -> Self {
RegistryQueryer {
registry,
replacements,
version_prefs,
registry_cache: HashMap::new(),
summary_cache: HashMap::new(),
used_replacements: HashMap::new(),
Expand Down Expand Up @@ -208,7 +204,9 @@ impl<'a> RegistryQueryer<'a> {
}

let first_version = first_version;
self.version_prefs.sort_summaries(&mut ret, first_version);
self.registry
.version_prefs()
.sort_summaries(&mut ret, first_version);

let out = Poll::Ready(Rc::new(ret));

Expand Down
3 changes: 1 addition & 2 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ pub fn resolve(
summaries: &[(Summary, ResolveOpts)],
replacements: &[(PackageIdSpec, Dependency)],
registry: &mut dyn Registry,
version_prefs: &VersionPreferences,
resolve_version: ResolveVersion,
gctx: Option<&GlobalContext>,
) -> CargoResult<Resolve> {
Expand All @@ -136,7 +135,7 @@ pub fn resolve(
}
_ => None,
};
let mut registry = RegistryQueryer::new(registry, replacements, version_prefs);
let mut registry = RegistryQueryer::new(registry, replacements);
let resolver_ctx = loop {
let resolver_ctx = ResolverContext::new();
let resolver_ctx =
Expand Down
4 changes: 4 additions & 0 deletions src/cargo/core/resolver/version_prefs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ pub enum VersionOrdering {
}

impl VersionPreferences {
pub fn get_try_to_use(&self) -> &HashSet<PackageId> {
&self.try_to_use
}

/// Indicate that the given package (specified as a [`PackageId`]) should be preferred.
pub fn prefer_package_id(&mut self, pkg_id: PackageId) {
self.try_to_use.insert(pkg_id);
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ pub fn resolve_to_string(ws: &Workspace<'_>, resolve: &Resolve) -> CargoResult<S
Ok(out)
}

/// Ensure the resolve result is written to fisk
/// Ensure the resolve result is written to disk
///
/// Returns `true` if the lockfile changed
#[tracing::instrument(skip_all)]
Expand Down
5 changes: 3 additions & 2 deletions src/cargo/ops/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,14 @@ use crate::core::resolver::{
self, HasDevUnits, Resolve, ResolveOpts, ResolveVersion, VersionOrdering, VersionPreferences,
};
use crate::core::summary::Summary;
use crate::core::Dependency;
use crate::core::GitReference;
use crate::core::PackageId;
use crate::core::PackageIdSpec;
use crate::core::PackageIdSpecQuery;
use crate::core::PackageSet;
use crate::core::SourceId;
use crate::core::Workspace;
use crate::core::{Dependency, Registry};
use crate::ops;
use crate::sources::RecursivePathSource;
use crate::util::cache_lock::CacheLockMode;
Expand Down Expand Up @@ -398,6 +398,8 @@ pub fn resolve_with_previous<'gctx>(
registry.lock_patches();
}

registry.set_version_prefs(version_prefs);

let summaries: Vec<(Summary, ResolveOpts)> = {
let _span = tracing::span!(tracing::Level::TRACE, "registry.lock").entered();
ws.members_with_features(specs, cli_features)?
Expand All @@ -421,7 +423,6 @@ pub fn resolve_with_previous<'gctx>(
&summaries,
&replace,
registry,
&version_prefs,
ResolveVersion::with_rust_version(ws.rust_version()),
Some(ws.gctx()),
)?;
Expand Down
Loading