diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index 1ced997bd95..5cd61ef9d0a 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -835,7 +835,7 @@ fn meta_test_multiple_versions_strategy() { .unwrap() .current(); let reg = registry(input.clone()); - for this in input.iter().rev().take(10) { + for this in input.iter().rev().take(11) { let res = resolve( vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 2728660b2bf..d4656dad4d5 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -8,7 +8,8 @@ use cargo_util::is_ci; use resolver_tests::{ assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, loc_names, names, pkg, pkg_id, pkg_loc, registry, registry_strategy, remove_dep, resolve, resolve_and_validated, - resolve_with_global_context, PrettyPrintRegistry, SatResolve, ToDep, ToPkgId, + resolve_with_global_context, resolve_with_global_context_raw, PrettyPrintRegistry, SatResolve, + ToDep, ToPkgId, }; use proptest::prelude::*; @@ -425,6 +426,174 @@ fn test_resolving_maximum_version_with_transitive_deps() { assert!(!res.contains(&("util", "1.1.1").to_pkgid())); } +#[test] +fn test_wildcard_minor() { + let reg = registry(vec![ + pkg!(("util", "0.1.0")), + pkg!(("util", "0.1.1")), + pkg!("foo" => [dep_req("util", "0.1.*")]), + ]); + + let res = resolve_and_validated( + vec![dep_req("util", "=0.1.0"), dep_req("foo", "1.0.0")], + ®, + None, + ) + .unwrap(); + + assert_same( + &res, + &names(&[("root", "1.0.0"), ("foo", "1.0.0"), ("util", "0.1.0")]), + ); +} + +#[test] +fn test_wildcard_major() { + let reg = registry(vec![ + pkg!("foo" => [dep_req("util", "0.*")]), + pkg!(("util", "0.1.0")), + pkg!(("util", "0.2.0")), + ]); + + let res = resolve_and_validated( + vec![dep_req("foo", "1.0.0"), dep_req("util", "=0.1.0")], + ®, + None, + ) + .unwrap(); + + assert_same( + &res, + &names(&[("root", "1.0.0"), ("foo", "1.0.0"), ("util", "0.1.0")]), + ); +} + +#[test] +fn test_range_major() { + let reg = registry(vec![ + pkg!("foo" => [dep_req("util", ">=0.1,<0.3")]), + pkg!(("util", "0.1.0")), + pkg!(("util", "0.2.0")), + ]); + + let res = resolve_and_validated( + vec![dep_req("foo", "1.0.0"), dep_req("util", "0.1.0")], + ®, + None, + ) + .unwrap(); + + assert_same( + &res, + &names(&[("root", "1.0.0"), ("foo", "1.0.0"), ("util", "0.1.0")]), + ); +} + +#[test] +fn test_wildcard_major_duplicate_selection() { + let reg = registry(vec![ + pkg!("foo" => [dep_req("util", "0.*")]), + pkg!("bar" => [dep_req("util", "0.2")]), + pkg!("car" => [dep_req("util", "0.1")]), + pkg!(("util", "0.1.0")), + pkg!(("util", "0.2.0")), + ]); + + let res = resolve_with_global_context_raw( + vec![ + dep_req("foo", "1.0.0"), + dep_req("bar", "1.0.0"), + dep_req("car", "1.0.0"), + ], + ®, + &GlobalContext::default().unwrap(), + ) + .unwrap(); + + // In this case, both 0.1.0 and 0.2.0 satisfy foo. It should pick the highest + // version. + assert_eq!( + res.deps(pkg_id("foo")).next().unwrap().0, + ("util", "0.2.0").to_pkgid() + ); + + let res = res.sort(); + + assert_same( + &res, + &names(&[ + ("root", "1.0.0"), + ("foo", "1.0.0"), + ("bar", "1.0.0"), + ("car", "1.0.0"), + ("util", "0.1.0"), + ("util", "0.2.0"), + ]), + ); +} + +#[test] +fn test_wildcard_major_coerced_by_subdependency() { + let reg = registry(vec![ + pkg!("foo" => [dep_req("util", "0.1")]), + pkg!(("util", "0.1.0")), + pkg!(("util", "0.2.0")), + ]); + + let res = resolve(vec![dep_req("foo", "1.0.0"), dep_req("util", "0.*")], ®).unwrap(); + + // In this case, both 0.1.0 and 0.2.0 satisfy root, but it's being coerced + // by the subdependency of foo. + assert_same( + &res, + &names(&[("root", "1.0.0"), ("foo", "1.0.0"), ("util", "0.1.0")]), + ); +} + +#[test] +fn test_wildcard_major_coerced_by_indirect_subdependency() { + let reg = registry(vec![ + pkg!("foo" => [dep_req("util", "0.1")]), + pkg!("bar" => [dep_req("car", "1.0.0")]), + pkg!("car" => [dep_req("util", "0.2")]), + pkg!(("util", "0.1.0")), + pkg!(("util", "0.2.0")), + pkg!(("util", "0.3.0")), + ]); + + let res = resolve_with_global_context_raw( + vec![ + dep_req("foo", "1.0.0"), + dep_req("bar", "1.0.0"), + dep_req("util", "0.*"), + ], + ®, + &GlobalContext::default().unwrap(), + ) + .unwrap(); + + // In this case, 0.1.0, 0.2.0 and 0.3.0 satisfy root. It should pick the highest + // version that exists in the dependency tree. + assert_eq!( + res.deps(pkg_id("root")).skip(2).next().unwrap().0, + ("util", "0.2.0").to_pkgid() + ); + + let res = res.sort(); + + assert_same( + &res, + &names(&[ + ("root", "1.0.0"), + ("foo", "1.0.0"), + ("bar", "1.0.0"), + ("car", "1.0.0"), + ("util", "0.1.0"), + ("util", "0.2.0"), + ]), + ); +} + #[test] fn test_resolving_minimum_version_with_transitive_deps() { let reg = registry(vec![ @@ -605,6 +774,35 @@ fn resolving_with_deep_backtracking() { ); } +#[test] +fn resolving_with_sys_crates_duplicates() { + // This is based on issues/4902 + // With `l` a normal library we get 2copies so everyone gets the newest compatible. + // But `l-sys` a library with a links attribute we make sure there is only one. + let reg = registry(vec![ + pkg!(("l-sys", "0.9.1")), + pkg!(("l-sys", "0.10.0")), + pkg!(("l", "0.9.1") => [dep_req("l-sys", ">=0.8.0, <=0.10.0")]), + pkg!(("l", "0.10.0") => [dep_req("l-sys", "0.9")]), + pkg!(("d", "1.0.0") => [dep_req("l", "0.10")]), + pkg!(("r", "1.0.0") => [dep_req("l", "0.9")]), + ]); + + let res = resolve(vec![dep_req("d", "1"), dep_req("r", "1")], ®).unwrap(); + + assert_same( + &res, + &names(&[ + ("root", "1.0.0"), + ("d", "1.0.0"), + ("r", "1.0.0"), + ("l-sys", "0.9.1"), + ("l", "0.9.1"), + ("l", "0.10.0"), + ]), + ); +} + #[test] fn resolving_with_sys_crates() { // This is based on issues/4902 @@ -629,7 +827,6 @@ fn resolving_with_sys_crates() { ("r", "1.0.0"), ("l-sys", "0.9.1"), ("l", "0.9.1"), - ("l", "0.10.0"), ]), ); } diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 2c3ee3afb50..03e5f51041d 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -260,16 +260,8 @@ fn activate_deps_loop( .conflicting(&resolver_ctx, &dep) .is_some(); - let mut remaining_candidates = RemainingCandidates::new(&candidates); - - // `conflicting_activations` stores all the reasons we were unable to - // activate candidates. One of these reasons will have to go away for - // backtracking to find a place to restart. It is also the list of - // things to explain in the error message if we fail to resolve. - // - // This is a map of package ID to a reason why that packaged caused a - // conflict for us. - let mut conflicting_activations = ConflictMap::new(); + let (mut remaining_candidates, mut conflicting_activations) = + RemainingCandidates::new(&candidates, &resolver_ctx); // When backtracking we don't fully update `conflicting_activations` // especially for the cases that we didn't make a backtrack frame in the @@ -279,7 +271,7 @@ fn activate_deps_loop( let mut backtracked = false; loop { - let next = remaining_candidates.next(&mut conflicting_activations, &resolver_ctx); + let next = remaining_candidates.next(); let (candidate, has_another) = next.ok_or(()).or_else(|_| { // If we get here then our `remaining_candidates` was just @@ -712,47 +704,38 @@ struct BacktrackFrame { /// more inputs) but in general acts like one. Each `RemainingCandidates` is /// created with a list of candidates to choose from. When attempting to iterate /// over the list of candidates only *valid* candidates are returned. Validity -/// is defined within a `Context`. +/// is defined within a `Context`. In addition, the iterator will return +/// candidates that already have been activated first. /// /// Candidates passed to `new` may not be returned from `next` as they could be -/// filtered out, and as they are filtered the causes will be added to `conflicting_prev_active`. +/// filtered out. The filtered candidates and the causes will be returned by `new`. #[derive(Clone)] struct RemainingCandidates { - remaining: RcVecIter, + prioritized_candidates: RcVecIter, // This is an inlined peekable generator has_another: Option, } impl RemainingCandidates { - fn new(candidates: &Rc>) -> RemainingCandidates { - RemainingCandidates { - remaining: RcVecIter::new(Rc::clone(candidates)), - has_another: None, - } - } - - /// Attempts to find another candidate to check from this list. - /// - /// This method will attempt to move this iterator forward, returning a - /// candidate that's possible to activate. The `cx` argument is the current - /// context which determines validity for candidates returned, and the `dep` - /// is the dependency listing that we're activating for. - /// - /// If successful a `(Candidate, bool)` pair will be returned. The - /// `Candidate` is the candidate to attempt to activate, and the `bool` is - /// an indicator of whether there are remaining candidates to try of if - /// we've reached the end of iteration. - /// - /// If we've reached the end of the iterator here then `Err` will be - /// returned. The error will contain a map of package ID to conflict reason, - /// where each package ID caused a candidate to be filtered out from the - /// original list for the reason listed. - fn next( - &mut self, - conflicting_prev_active: &mut ConflictMap, + /// Prefilters and sorts the list of candidates to determine which ones are + /// valid to activate, and which ones should be prioritized. + fn new( + candidates: &Rc>, cx: &ResolverContext, - ) -> Option<(Summary, bool)> { - for b in self.remaining.by_ref() { + ) -> (RemainingCandidates, ConflictMap) { + // `conflicting_activations` stores all the reasons we were unable to + // activate candidates. One of these reasons will have to go away for + // backtracking to find a place to restart. It is also the list of + // things to explain in the error message if we fail to resolve. + // + // This is a map of package ID to a reason why that packaged caused a + // conflict for us. + let mut conflicting_activations = ConflictMap::new(); + + let mut activated_candidates: Vec = Vec::with_capacity(candidates.len()); + let mut non_activated_candidates: Vec = Vec::with_capacity(candidates.len()); + + for b in candidates.as_ref() { let b_id = b.package_id(); // The `links` key in the manifest dictates that there's only one // package in a dependency graph, globally, with that particular @@ -761,7 +744,7 @@ impl RemainingCandidates { if let Some(link) = b.links() { if let Some(&a) = cx.links.get(&link) { if a != b_id { - conflicting_prev_active + conflicting_activations .entry(a) .or_insert_with(|| ConflictReason::Links(link)); continue; @@ -774,18 +757,50 @@ impl RemainingCandidates { // semver-compatible versions of a crate. For example we can't // simultaneously activate `foo 1.0.2` and `foo 1.2.0`. We can, // however, activate `1.0.2` and `2.0.0`. - // - // Here we throw out our candidate if it's *compatible*, yet not - // equal, to all previously activated versions. if let Some((a, _)) = cx.activations.get(&b_id.as_activations_key()) { - if *a != b { - conflicting_prev_active + // If this candidate is already activated, then we want to put + // it in our prioritized list to try first. + if a == b { + activated_candidates.push(b.clone()); + continue; + } + // Here we throw out our candidate if it's *compatible*, yet not + // equal, to all previously activated versions. + else { + conflicting_activations .entry(a.package_id()) .or_insert(ConflictReason::Semver); continue; } + } else { + non_activated_candidates.push(b.clone()); } + } + // Combine the prioritized and non-prioritized candidates into one list + // such that the prioritized candidates are tried first. + activated_candidates.append(&mut non_activated_candidates); + + ( + RemainingCandidates { + prioritized_candidates: RcVecIter::new(Rc::new(activated_candidates)), + has_another: None, + }, + conflicting_activations, + ) + } + + /// Attempts to find another candidate to check from this list. + /// + /// This method will attempt to move this iterator forward, returning a + /// candidate that's possible to activate. + /// + /// If successful a `(Candidate, bool)` pair will be returned. The + /// `Candidate` is the candidate to attempt to activate, and the `bool` is + /// an indicator of whether there are remaining candidates to try of if + /// we've reached the end of iteration. + fn next(&mut self) -> Option<(Summary, bool)> { + for b in self.prioritized_candidates.by_ref() { // Well if we made it this far then we've got a valid dependency. We // want this iterator to be inherently "peekable" so we don't // necessarily return the item just yet. Instead we stash it away to @@ -961,9 +976,7 @@ fn find_candidate( }; while let Some(mut frame) = backtrack_stack.pop() { - let next = frame - .remaining_candidates - .next(&mut frame.conflicting_activations, &frame.context); + let next = frame.remaining_candidates.next(); let Some((candidate, has_another)) = next else { continue; };