From 95d221dee504d3fe1cd2dda10df771bce9a545c8 Mon Sep 17 00:00:00 2001 From: Kevin GRONDIN Date: Mon, 23 Sep 2024 17:39:08 +0200 Subject: [PATCH 1/2] test(update): add test case updating package compatible with multiple major versions --- tests/testsuite/update.rs | 102 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index 90525920692..d0d8ca60949 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -361,6 +361,108 @@ fn change_package_version() { p.cargo("check").run(); } +#[cargo_test] +fn update_choose_between_multiple_major_versions() { + Package::new("log", "0.1.0").publish(); + Package::new("log", "0.2.0").publish(); + Package::new("log", "0.3.0").publish(); + Package::new("log", "0.4.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "bar" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + log1 = { package = "log", version = "0.1" } + log2 = { package = "log", version = "0.2" } + log3 = { package = "log", version = "0.3" } + log4 = { package = "log", version = "0.4" } + foo = { path = "foo" } + "#, + ) + .file("src/lib.rs", "") + .file( + "foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + log = ">=0.1, <0.5" + "#, + ) + .file("foo/src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile").run(); + + assert!(p.read_lockfile().contains( + r#"[[package]] +name = "foo" +version = "0.0.1" +dependencies = [ + "log 0.4.0", +]"# + )); + + p.cargo("update foo").run(); + + assert!(p.read_lockfile().contains( + r#"[[package]] +name = "foo" +version = "0.0.1" +dependencies = [ + "log 0.1.0", +]"# + )); + + p.cargo("update foo -Zminimal-versions") + .masquerade_as_nightly_cargo(&["minimal-versions"]) + .run(); + + assert!(p.read_lockfile().contains( + r#"[[package]] +name = "foo" +version = "0.0.1" +dependencies = [ + "log 0.2.0", +]"# + )); + + p.cargo("update foo -Zminimal-versions") + .masquerade_as_nightly_cargo(&["minimal-versions"]) + .run(); + + assert!(p.read_lockfile().contains( + r#"[[package]] +name = "foo" +version = "0.0.1" +dependencies = [ + "log 0.1.0", +]"# + )); + + p.cargo("update foo").run(); + + assert!(p.read_lockfile().contains( + r#"[[package]] +name = "foo" +version = "0.0.1" +dependencies = [ + "log 0.2.0", +]"# + )); +} + #[cargo_test] fn update_precise() { Package::new("serde", "0.1.0").publish(); From 61113b48b16a801a44e170c32f6f5fdaa0742780 Mon Sep 17 00:00:00 2001 From: Kevin GRONDIN Date: Mon, 23 Sep 2024 17:42:07 +0200 Subject: [PATCH 2/2] fix(update): don't reuse a previously locked dependency if multiple major versions are compatible --- crates/resolver-tests/src/lib.rs | 12 +++- src/cargo/core/registry.rs | 71 +++++++++++++++++++++--- src/cargo/core/resolver/dep_cache.rs | 8 +-- src/cargo/core/resolver/mod.rs | 3 +- src/cargo/core/resolver/version_prefs.rs | 4 ++ src/cargo/ops/lockfile.rs | 2 +- src/cargo/ops/resolve.rs | 5 +- tests/testsuite/update.rs | 6 +- 8 files changed, 88 insertions(+), 23 deletions(-) diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index 43dc49dd339..cad1e4d3412 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -102,6 +102,7 @@ pub fn resolve_with_global_context_raw( struct MyRegistry<'a> { list: &'a [Summary], used: HashSet, + version_prefs: VersionPreferences, } impl<'a> Registry for MyRegistry<'a> { fn query( @@ -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) { @@ -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"), @@ -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), ); diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index 37af3d7a130..22aca86db84 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -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; @@ -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); } /// This structure represents a registry of known packages. It internally @@ -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>, + + /// Version preferences. + version_prefs: VersionPreferences, } /// A map of all "locked packages" which is filled in when parsing a lock file @@ -209,6 +219,7 @@ impl<'gctx> PackageRegistry<'gctx> { patches: HashMap::new(), patches_locked: false, patches_available: HashMap::new(), + version_prefs: VersionPreferences::default(), }) } @@ -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; @@ -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( @@ -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); } @@ -799,6 +825,14 @@ 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`]. @@ -806,6 +840,7 @@ fn lock( locked: &LockedMap, patches: &HashMap>, summary: Summary, + version_prefs: &VersionPreferences, ) -> Summary { let pair = locked .get(&(summary.source_id(), summary.name())) @@ -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. + + 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; diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index e0964fcc07d..5c614908c0b 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -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, @@ -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), Poll>>>, /// a cache of `Dependency`s that are required for a `Summary` @@ -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(), @@ -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)); diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 010c03bb85b..1ac0d87f1af 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -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 { @@ -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 = diff --git a/src/cargo/core/resolver/version_prefs.rs b/src/cargo/core/resolver/version_prefs.rs index c89c13fdd4b..5725fe2f99b 100644 --- a/src/cargo/core/resolver/version_prefs.rs +++ b/src/cargo/core/resolver/version_prefs.rs @@ -32,6 +32,10 @@ pub enum VersionOrdering { } impl VersionPreferences { + pub fn get_try_to_use(&self) -> &HashSet { + &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); diff --git a/src/cargo/ops/lockfile.rs b/src/cargo/ops/lockfile.rs index d865a7186f6..65afa388040 100644 --- a/src/cargo/ops/lockfile.rs +++ b/src/cargo/ops/lockfile.rs @@ -35,7 +35,7 @@ pub fn resolve_to_string(ws: &Workspace<'_>, resolve: &Resolve) -> CargoResult( 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)? @@ -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()), )?; diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index d0d8ca60949..5f246bfb77e 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -421,7 +421,7 @@ dependencies = [ name = "foo" version = "0.0.1" dependencies = [ - "log 0.1.0", + "log 0.4.0", ]"# )); @@ -434,7 +434,7 @@ dependencies = [ name = "foo" version = "0.0.1" dependencies = [ - "log 0.2.0", + "log 0.1.0", ]"# )); @@ -458,7 +458,7 @@ dependencies = [ name = "foo" version = "0.0.1" dependencies = [ - "log 0.2.0", + "log 0.4.0", ]"# )); }