From aece1f46237fc49e244d84122784a540c89dfdf5 Mon Sep 17 00:00:00 2001 From: x-hgg-x <39058530+x-hgg-x@users.noreply.github.com> Date: Tue, 24 Sep 2024 23:07:32 +0200 Subject: [PATCH 1/2] test: refactor resolver test functions --- crates/resolver-tests/src/lib.rs | 156 ++++++++++++++----------- crates/resolver-tests/tests/resolve.rs | 78 ++++++------- src/cargo/core/summary.rs | 3 +- 3 files changed, 126 insertions(+), 111 deletions(-) diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index 43dc49dd339..78b026f8799 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -1,11 +1,9 @@ #![allow(clippy::print_stderr)] -use std::cell::RefCell; use std::cmp::{max, min}; use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt; use std::fmt::Write; -use std::rc::Rc; use std::sync::OnceLock; use std::task::Poll; use std::time::Instant; @@ -19,7 +17,6 @@ use cargo::core::{GitReference, SourceId}; use cargo::sources::source::QueryKind; use cargo::sources::IndexSummary; use cargo::util::{CargoResult, GlobalContext, IntoUrl}; -use cargo_util_schemas::manifest::RustVersion; use proptest::collection::{btree_map, vec}; use proptest::prelude::*; @@ -35,18 +32,17 @@ pub fn resolve(deps: Vec, registry: &[Summary]) -> CargoResult, registry: &[Summary], - sat_resolve: Option, + sat_resolver: &mut SatResolver, ) -> CargoResult> { let resolve = resolve_with_global_context_raw(deps.clone(), registry, &GlobalContext::default().unwrap()); match resolve { Err(e) => { - let sat_resolve = sat_resolve.unwrap_or_else(|| SatResolve::new(registry)); - if sat_resolve.sat_resolve(&deps) { + if sat_resolver.sat_resolve(&deps) { panic!( - "the resolve err but the sat_resolve thinks this will work:\n{}", - sat_resolve.use_packages().unwrap() + "`resolve()` returned an error but the sat resolver thinks this will work:\n{}", + sat_resolver.used_packages().unwrap() ); } Err(e) @@ -73,11 +69,10 @@ pub fn resolve_and_validated( let out = resolve.sort(); assert_eq!(out.len(), used.len()); - let sat_resolve = sat_resolve.unwrap_or_else(|| SatResolve::new(registry)); - if !sat_resolve.sat_is_valid_solution(&out) { + if !sat_resolver.sat_is_valid_solution(&out) { panic!( - "the sat_resolve err but the resolve thinks this will work:\n{:?}", - resolve + "`resolve()` thinks this will work, but the solution is \ + invalid according to the sat resolver:\n{resolve:?}", ); } Ok(out) @@ -158,22 +153,26 @@ pub fn resolve_with_global_context_raw( list: registry, used: HashSet::new(), }; - let summary = Summary::new( + + let root_summary = Summary::new( pkg_id("root"), deps, &BTreeMap::new(), None::<&String>, - None::, + None, ) .unwrap(); + let opts = ResolveOpts::everything(); + let start = Instant::now(); let mut version_prefs = VersionPreferences::default(); if gctx.cli_unstable().minimal_versions { version_prefs.version_ordering(VersionOrdering::MinimumVersionsFirst) } + let resolve = resolver::resolve( - &[(summary, opts)], + &[(root_summary, opts)], &[], &mut registry, &version_prefs, @@ -181,8 +180,8 @@ pub fn resolve_with_global_context_raw( Some(gctx), ); - // The largest test in our suite takes less then 30 sec. - // So lets fail the test if we have ben running for two long. + // The largest test in our suite takes less then 30 secs. + // So let's fail the test if we have been running for more than 60 secs. assert!(start.elapsed().as_secs() < 60); resolve } @@ -249,18 +248,15 @@ fn sat_at_most_one_by_key( /// find a valid resolution if one exists. The big thing that the real resolver does, /// that this one does not do is work with features and optional dependencies. /// -/// The SAT library dose not optimize for the newer version, +/// The SAT library does not optimize for the newer version, /// so the selected packages may not match the real resolver. -#[derive(Clone)] -pub struct SatResolve(Rc>); - -struct SatResolveInner { +pub struct SatResolver { solver: varisat::Solver<'static>, var_for_is_packages_used: HashMap, by_name: HashMap<&'static str, Vec>, } -impl SatResolve { +impl SatResolver { pub fn new(registry: &[Summary]) -> Self { let mut cnf = varisat::CnfFormula::new(); // That represents each package version which is set to "true" if the packages in the lock file and "false" if it is unused. @@ -325,27 +321,28 @@ impl SatResolve { solver .solve() .expect("docs say it can't error in default config"); - SatResolve(Rc::new(RefCell::new(SatResolveInner { + + SatResolver { solver, var_for_is_packages_used, by_name, - }))) + } } - pub fn sat_resolve(&self, deps: &[Dependency]) -> bool { - let mut s = self.0.borrow_mut(); + + pub fn sat_resolve(&mut self, root_dependencies: &[Dependency]) -> bool { let mut assumption = vec![]; let mut this_call = None; // the starting `deps` need to be satisfied - for dep in deps.iter() { + for dep in root_dependencies { let empty_vec = vec![]; - let matches: Vec = s + let matches: Vec = self .by_name .get(dep.package_name().as_str()) .unwrap_or(&empty_vec) .iter() .filter(|&p| dep.matches_id(*p)) - .map(|p| s.var_for_is_packages_used[p].positive()) + .map(|p| self.var_for_is_packages_used[p].positive()) .collect(); if matches.is_empty() { return false; @@ -353,43 +350,44 @@ impl SatResolve { assumption.extend_from_slice(&matches) } else { if this_call.is_none() { - let new_var = s.solver.new_var(); + let new_var = self.solver.new_var(); this_call = Some(new_var); assumption.push(new_var.positive()); } let mut matches = matches; matches.push(this_call.unwrap().negative()); - s.solver.add_clause(&matches); + self.solver.add_clause(&matches); } } - s.solver.assume(&assumption); + self.solver.assume(&assumption); - s.solver + self.solver .solve() .expect("docs say it can't error in default config") } - pub fn sat_is_valid_solution(&self, pids: &[PackageId]) -> bool { - let mut s = self.0.borrow_mut(); + + pub fn sat_is_valid_solution(&mut self, pids: &[PackageId]) -> bool { for p in pids { - if p.name().as_str() != "root" && !s.var_for_is_packages_used.contains_key(p) { + if p.name().as_str() != "root" && !self.var_for_is_packages_used.contains_key(p) { return false; } } - let assumption: Vec<_> = s + let assumption: Vec<_> = self .var_for_is_packages_used .iter() .map(|(p, v)| v.lit(pids.contains(p))) .collect(); - s.solver.assume(&assumption); + self.solver.assume(&assumption); - s.solver + self.solver .solve() .expect("docs say it can't error in default config") } - fn use_packages(&self) -> Option { - self.0.borrow().solver.model().map(|lits| { + + fn used_packages(&self) -> Option { + self.solver.model().map(|lits| { let lits: HashSet<_> = lits .iter() .filter(|l| l.is_positive()) @@ -397,7 +395,7 @@ impl SatResolve { .collect(); let mut out = String::new(); out.push_str("used:\n"); - for (p, v) in self.0.borrow().var_for_is_packages_used.iter() { + for (p, v) in self.var_for_is_packages_used.iter() { if lits.contains(v) { writeln!(&mut out, " {}", p).unwrap(); } @@ -409,18 +407,40 @@ impl SatResolve { pub trait ToDep { fn to_dep(self) -> Dependency; + fn to_opt_dep(self) -> Dependency; + fn to_dep_with(self, features: &[&'static str]) -> Dependency; } impl ToDep for &'static str { fn to_dep(self) -> Dependency { Dependency::parse(self, Some("1.0.0"), registry_loc()).unwrap() } + fn to_opt_dep(self) -> Dependency { + let mut dep = self.to_dep(); + dep.set_optional(true); + dep + } + fn to_dep_with(self, features: &[&'static str]) -> Dependency { + let mut dep = self.to_dep(); + dep.set_default_features(false); + dep.set_features(features.into_iter().copied()); + dep + } } impl ToDep for Dependency { fn to_dep(self) -> Dependency { self } + fn to_opt_dep(mut self) -> Dependency { + self.set_optional(true); + self + } + fn to_dep_with(mut self, features: &[&'static str]) -> Dependency { + self.set_default_features(false); + self.set_features(features.into_iter().copied()); + self + } } pub trait ToPkgId { @@ -448,8 +468,8 @@ impl, U: AsRef> ToPkgId for (T, U) { #[macro_export] macro_rules! pkg { - ($pkgid:expr => [$($deps:expr),+ $(,)* ]) => ({ - let d: Vec = vec![$($deps.to_dep()),+]; + ($pkgid:expr => [$($deps:expr),* $(,)? ]) => ({ + let d: Vec = vec![$($deps.to_dep()),*]; $crate::pkg_dep($pkgid, d) }); @@ -473,18 +493,29 @@ pub fn pkg(name: T) -> Summary { pub fn pkg_dep(name: T, dep: Vec) -> Summary { let pkgid = name.to_pkgid(); let link = if pkgid.name().ends_with("-sys") { - Some(pkgid.name().as_str()) + Some(pkgid.name()) } else { None }; - Summary::new( - name.to_pkgid(), - dep, - &BTreeMap::new(), - link, - None::, - ) - .unwrap() + Summary::new(name.to_pkgid(), dep, &BTreeMap::new(), link, None).unwrap() +} + +pub fn pkg_dep_with( + name: T, + dep: Vec, + features: &[(&'static str, &[&'static str])], +) -> Summary { + let pkgid = name.to_pkgid(); + let link = if pkgid.name().ends_with("-sys") { + Some(pkgid.name()) + } else { + None + }; + let features = features + .into_iter() + .map(|&(name, values)| (name.into(), values.into_iter().map(|&v| v.into()).collect())) + .collect(); + Summary::new(name.to_pkgid(), dep, &features, link, None).unwrap() } pub fn pkg_id(name: &str) -> PackageId { @@ -510,7 +541,7 @@ pub fn pkg_loc(name: &str, loc: &str) -> Summary { Vec::new(), &BTreeMap::new(), link, - None::, + None, ) .unwrap() } @@ -519,14 +550,7 @@ pub fn remove_dep(sum: &Summary, ind: usize) -> Summary { let mut deps = sum.dependencies().to_vec(); deps.remove(ind); // note: more things will need to be copied over in the future, but it works for now. - Summary::new( - sum.package_id(), - deps, - &BTreeMap::new(), - sum.links().map(|a| a.as_str()), - None::, - ) - .unwrap() + Summary::new(sum.package_id(), deps, &BTreeMap::new(), sum.links(), None).unwrap() } pub fn dep(name: &str) -> Dependency { @@ -629,7 +653,7 @@ fn meta_test_deep_pretty_print_registry() { pkg!(("foo", "1.0.0") => [dep_req("bar", "2")]), pkg!(("foo", "2.0.0") => [dep_req("bar", "*")]), pkg!(("bar", "1.0.0") => [dep_req("baz", "=1.0.2"), - dep_req("other", "1")]), + dep_req("other", "1")]), pkg!(("bar", "2.0.0") => [dep_req("baz", "=1.0.1")]), pkg!(("baz", "1.0.2") => [dep_req("other", "2")]), pkg!(("baz", "1.0.1")), @@ -654,8 +678,8 @@ fn meta_test_deep_pretty_print_registry() { } /// This generates a random registry index. -/// Unlike vec((Name, Ver, vec((Name, VerRq), ..), ..) -/// This strategy has a high probability of having valid dependencies +/// Unlike `vec((Name, Ver, vec((Name, VerRq), ..), ..)`, +/// this strategy has a high probability of having valid dependencies. pub fn registry_strategy( max_crates: usize, max_versions: usize, diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 2728660b2bf..4d254a0c69e 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -8,7 +8,7 @@ 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, PrettyPrintRegistry, SatResolver, ToDep, ToPkgId, }; use proptest::prelude::*; @@ -40,15 +40,15 @@ proptest! { PrettyPrintRegistry(input) in registry_strategy(50, 20, 60) ) { let reg = registry(input.clone()); - let sat_resolve = SatResolve::new(®); - // there is only a small chance that any one - // crate will be interesting. + let mut sat_resolver = SatResolver::new(®); + + // There is only a small chance that a crate will be interesting. // So we try some of the most complicated. for this in input.iter().rev().take(20) { let _ = resolve_and_validated( vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, - Some(sat_resolve.clone()), + &mut sat_resolver, ); } } @@ -75,23 +75,15 @@ proptest! { .unwrap(); let reg = registry(input.clone()); - // there is only a small chance that any one - // crate will be interesting. + + // There is only a small chance that a crate will be interesting. // So we try some of the most complicated. for this in input.iter().rev().take(10) { - // minimal-versions change what order the candidates - // are tried but not the existence of a solution - let res = resolve( - vec![dep_req(&this.name(), &format!("={}", this.version()))], - ®, - ); - - let mres = resolve_with_global_context( - vec![dep_req(&this.name(), &format!("={}", this.version()))], - ®, - &gctx, - ); + let deps = vec![dep_req(&this.name(), &format!("={}", this.version()))]; + let res = resolve(deps.clone(), ®); + let mres = resolve_with_global_context(deps, ®, &gctx); + // `minimal-versions` changes what order the candidates are tried but not the existence of a solution. prop_assert_eq!( res.is_ok(), mres.is_ok(), @@ -124,23 +116,16 @@ proptest! { .unwrap(); let reg = registry(input.clone()); - // there is only a small chance that any one - // crate will be interesting. + + // There is only a small chance that a crate will be interesting. // So we try some of the most complicated. for this in input.iter().rev().take(10) { - // direct-minimal-versions reduces the number of available solutions, so we verify that - // we do not come up with solutions maximal versions does not - let res = resolve( - vec![dep_req(&this.name(), &format!("={}", this.version()))], - ®, - ); - - let mres = resolve_with_global_context( - vec![dep_req(&this.name(), &format!("={}", this.version()))], - ®, - &gctx, - ); + let deps = vec![dep_req(&this.name(), &format!("={}", this.version()))]; + let res = resolve(deps.clone(), ®); + let mres = resolve_with_global_context(deps, ®, &gctx); + // `direct-minimal-versions` reduces the number of available solutions, + // so we verify that we do not come up with solutions not seen in `maximal-versions`. if res.is_err() { prop_assert!( mres.is_err(), @@ -179,8 +164,8 @@ proptest! { } } let removed_reg = registry(removed_input); - // there is only a small chance that any one - // crate will be interesting. + + // There is only a small chance that a crate will be interesting. // So we try some of the most complicated. for this in input.iter().rev().take(10) { if resolve( @@ -207,8 +192,8 @@ proptest! { indexes_to_unpublish in prop::collection::vec(any::(), ..10) ) { let reg = registry(input.clone()); - // there is only a small chance that any one - // crate will be interesting. + + // There is only a small chance that a crate will be interesting. // So we try some of the most complicated. for this in input.iter().rev().take(10) { let res = resolve( @@ -225,6 +210,7 @@ proptest! { .cloned() .filter(|x| !r.contains(&x.package_id())) .collect(); + if !not_selected.is_empty() { let indexes_to_unpublish: Vec<_> = indexes_to_unpublish.iter().map(|x| x.get(¬_selected)).collect(); @@ -659,8 +645,8 @@ fn resolving_with_constrained_sibling_backtrack_parent() { let vsn = format!("1.0.{}", i); reglist.push( pkg!(("bar", vsn.clone()) => [dep_req("backtrack_trap1", "1.0.2"), - dep_req("backtrack_trap2", "1.0.2"), - dep_req("constrained", "1.0.1")]), + dep_req("backtrack_trap2", "1.0.2"), + dep_req("constrained", "1.0.1")]), ); reglist.push(pkg!(("backtrack_trap1", vsn.clone()))); reglist.push(pkg!(("backtrack_trap2", vsn.clone()))); @@ -1227,7 +1213,8 @@ fn off_by_one_bug() { ]; let reg = registry(input); - let _ = resolve_and_validated(vec![dep("f")], ®, None); + let mut sat_resolver = SatResolver::new(®); + let _ = resolve_and_validated(vec![dep("f")], ®, &mut sat_resolver); } #[test] @@ -1267,7 +1254,8 @@ fn conflict_store_bug() { ]; let reg = registry(input); - let _ = resolve_and_validated(vec![dep("j")], ®, None); + let mut sat_resolver = SatResolver::new(®); + let _ = resolve_and_validated(vec![dep("j")], ®, &mut sat_resolver); } #[test] @@ -1295,7 +1283,8 @@ fn conflict_store_more_then_one_match() { ]), ]; let reg = registry(input); - let _ = resolve_and_validated(vec![dep("nA")], ®, None); + let mut sat_resolver = SatResolver::new(®); + let _ = resolve_and_validated(vec![dep("nA")], ®, &mut sat_resolver); } #[test] @@ -1304,7 +1293,7 @@ fn bad_lockfile_from_8249() { pkg!(("a-sys", "0.2.0")), pkg!(("a-sys", "0.1.0")), pkg!(("b", "0.1.0") => [ - dep_req("a-sys", "0.1"), // should be optional: true, but not deeded for now + dep_req("a-sys", "0.1"), // should be optional: true, but not needed for now ]), pkg!(("c", "1.0.0") => [ dep_req("b", "=0.1.0"), @@ -1320,7 +1309,8 @@ fn bad_lockfile_from_8249() { ]), ]; let reg = registry(input); - let _ = resolve_and_validated(vec![dep("foo")], ®, None); + let mut sat_resolver = SatResolver::new(®); + let _ = resolve_and_validated(vec![dep("foo")], ®, &mut sat_resolver); } #[test] diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 424328fa6d1..1b07bcb9b4c 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -216,6 +216,7 @@ fn build_feature_map( .flatten() .filter_map(|fv| fv.explicit_dep_name()) .collect(); + for dep in dependencies { if !dep.is_optional() { continue; @@ -242,7 +243,7 @@ fn build_feature_map( if !is_any_dep { bail!( "feature `{feature}` includes `{fv}` which is neither a dependency \ - nor another feature" + nor another feature" ); } if is_optional_dep { From 6f1315be84b9eb047aa77df48574b0f6744fa3b0 Mon Sep 17 00:00:00 2001 From: x-hgg-x <39058530+x-hgg-x@users.noreply.github.com> Date: Tue, 24 Sep 2024 23:12:18 +0200 Subject: [PATCH 2/2] test: add support for features in the sat resolver --- crates/resolver-tests/src/lib.rs | 394 +++++++++++++++++++------ crates/resolver-tests/tests/resolve.rs | 112 ++++++- 2 files changed, 404 insertions(+), 102 deletions(-) diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index 78b026f8799..aa9c8a56ce8 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -1,7 +1,7 @@ #![allow(clippy::print_stderr)] use std::cmp::{max, min}; -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::fmt; use std::fmt::Write; use std::sync::OnceLock; @@ -10,12 +10,14 @@ use std::time::Instant; use cargo::core::dependency::DepKind; use cargo::core::resolver::{self, ResolveOpts, VersionOrdering, VersionPreferences}; -use cargo::core::Resolve; +use cargo::core::FeatureMap; use cargo::core::ResolveVersion; use cargo::core::{Dependency, PackageId, Registry, Summary}; +use cargo::core::{FeatureValue, Resolve}; use cargo::core::{GitReference, SourceId}; use cargo::sources::source::QueryKind; use cargo::sources::IndexSummary; +use cargo::util::interning::{InternedString, INTERNED_DEFAULT}; use cargo::util::{CargoResult, GlobalContext, IntoUrl}; use proptest::collection::{btree_map, vec}; @@ -25,7 +27,12 @@ use proptest::string::string_regex; use varisat::ExtendFormula; pub fn resolve(deps: Vec, registry: &[Summary]) -> CargoResult> { - resolve_with_global_context(deps, registry, &GlobalContext::default().unwrap()) + Ok( + resolve_with_global_context(deps, registry, &GlobalContext::default().unwrap())? + .into_iter() + .map(|(pkg, _)| pkg) + .collect(), + ) } // Verify that the resolution of cargo resolver can pass the verification of SAT @@ -33,7 +40,7 @@ pub fn resolve_and_validated( deps: Vec, registry: &[Summary], sat_resolver: &mut SatResolver, -) -> CargoResult> { +) -> CargoResult)>> { let resolve = resolve_with_global_context_raw(deps.clone(), registry, &GlobalContext::default().unwrap()); @@ -66,7 +73,7 @@ pub fn resolve_and_validated( })); } } - let out = resolve.sort(); + let out = collect_features(&resolve); assert_eq!(out.len(), used.len()); if !sat_resolver.sat_is_valid_solution(&out) { @@ -80,13 +87,21 @@ pub fn resolve_and_validated( } } +fn collect_features(resolve: &Resolve) -> Vec<(PackageId, Vec)> { + resolve + .sort() + .iter() + .map(|&pkg| (pkg, resolve.features(pkg).to_vec())) + .collect() +} + pub fn resolve_with_global_context( deps: Vec, registry: &[Summary], gctx: &GlobalContext, -) -> CargoResult> { +) -> CargoResult)>> { let resolve = resolve_with_global_context_raw(deps, registry, gctx)?; - Ok(resolve.sort()) + Ok(collect_features(&resolve)) } pub fn resolve_with_global_context_raw( @@ -201,7 +216,7 @@ fn log_bits(x: usize) -> usize { // At this point is possible to select every version of every package. // So we need to mark certain versions as incompatible with each other. // We could add a clause not A, not B for all A and B that are incompatible, -fn sat_at_most_one(solver: &mut impl varisat::ExtendFormula, vars: &[varisat::Var]) { +fn sat_at_most_one(solver: &mut varisat::Solver<'_>, vars: &[varisat::Var]) { if vars.len() <= 1 { return; } else if vars.len() == 2 { @@ -226,48 +241,201 @@ fn sat_at_most_one(solver: &mut impl varisat::ExtendFormula, vars: &[varisat::Va } fn sat_at_most_one_by_key( - cnf: &mut impl varisat::ExtendFormula, + solver: &mut varisat::Solver<'_>, data: impl Iterator, ) -> HashMap> { - // no two packages with the same links set + // no two packages with the same keys set let mut by_keys: HashMap> = HashMap::new(); for (p, v) in data { by_keys.entry(p).or_default().push(v) } for key in by_keys.values() { - sat_at_most_one(cnf, key); + sat_at_most_one(solver, key); } by_keys } +fn find_compatible_dep_summaries_by_name_in_toml( + pkg_dependencies: &[Dependency], + by_name: &HashMap>, +) -> HashMap> { + let empty_vec = vec![]; + + pkg_dependencies + .iter() + .map(|dep| { + let name_in_toml = dep.name_in_toml(); + + let compatible_summaries = by_name + .get(&dep.package_name()) + .unwrap_or(&empty_vec) + .iter() + .filter(|s| dep.matches_id(s.package_id())) + .filter(|s| dep.features().iter().all(|f| s.features().contains_key(f))) + .cloned() + .collect::>(); + + (name_in_toml, compatible_summaries) + }) + .collect() +} + +fn process_pkg_features( + solver: &mut varisat::Solver<'_>, + var_for_is_packages_used: &HashMap, + var_for_is_packages_features_used: &HashMap>, + pkg_feature_var_map: &HashMap, + pkg_features: &FeatureMap, + compatible_dep_summaries_by_name_in_toml: &HashMap>, +) { + // add clauses for package features + for (&feature_name, feature_values) in pkg_features { + for feature_value in feature_values { + let pkg_feature_var = pkg_feature_var_map[&feature_name]; + + match *feature_value { + FeatureValue::Feature(other_feature_name) => { + solver.add_clause(&[ + pkg_feature_var.negative(), + pkg_feature_var_map[&other_feature_name].positive(), + ]); + } + FeatureValue::Dep { dep_name } => { + let dep_clause = compatible_dep_summaries_by_name_in_toml[&dep_name] + .iter() + .map(|dep| var_for_is_packages_used[&dep.package_id()].positive()) + .chain([pkg_feature_var.negative()]) + .collect::>(); + + solver.add_clause(&dep_clause); + } + FeatureValue::DepFeature { + dep_name, + dep_feature: dep_feature_name, + weak, + } => { + for dep in &compatible_dep_summaries_by_name_in_toml[&dep_name] { + let dep_var = var_for_is_packages_used[&dep.package_id()]; + let dep_feature_var = + var_for_is_packages_features_used[&dep.package_id()][&dep_feature_name]; + + solver.add_clause(&[ + pkg_feature_var.negative(), + dep_var.negative(), + dep_feature_var.positive(), + ]); + } + + if !weak { + let dep_clause = compatible_dep_summaries_by_name_in_toml[&dep_name] + .iter() + .map(|dep| var_for_is_packages_used[&dep.package_id()].positive()) + .chain([pkg_feature_var.negative()]) + .collect::>(); + + solver.add_clause(&dep_clause); + } + } + } + } + } +} + +fn process_pkg_dependencies( + solver: &mut varisat::Solver<'_>, + var_for_is_packages_used: &HashMap, + var_for_is_packages_features_used: &HashMap>, + pkg_var: varisat::Var, + pkg_dependencies: &[Dependency], + compatible_dep_summaries_by_name_in_toml: &HashMap>, +) { + for dep in pkg_dependencies { + let compatible_dep_summaries = + &compatible_dep_summaries_by_name_in_toml[&dep.name_in_toml()]; + + // add clauses for package dependency features + for dep_summary in compatible_dep_summaries { + let dep_package_id = dep_summary.package_id(); + + let default_feature = if dep.uses_default_features() + && dep_summary.features().contains_key(&*INTERNED_DEFAULT) + { + Some(&INTERNED_DEFAULT) + } else { + None + }; + + for &feature_name in default_feature.into_iter().chain(dep.features()) { + solver.add_clause(&[ + pkg_var.negative(), + var_for_is_packages_used[&dep_package_id].negative(), + var_for_is_packages_features_used[&dep_package_id][&feature_name].positive(), + ]); + } + } + + // active packages need to activate each of their non-optional dependencies + if !dep.is_optional() { + let dep_clause = compatible_dep_summaries + .iter() + .map(|d| var_for_is_packages_used[&d.package_id()].positive()) + .chain([pkg_var.negative()]) + .collect::>(); + + solver.add_clause(&dep_clause); + } + } +} + /// Resolution can be reduced to the SAT problem. So this is an alternative implementation /// of the resolver that uses a SAT library for the hard work. This is intended to be easy to read, /// as compared to the real resolver. /// -/// For the subset of functionality that are currently made by `registry_strategy` this will, -/// find a valid resolution if one exists. The big thing that the real resolver does, -/// that this one does not do is work with features and optional dependencies. +/// For the subset of functionality that are currently made by `registry_strategy`, +/// this will find a valid resolution if one exists. /// /// The SAT library does not optimize for the newer version, /// so the selected packages may not match the real resolver. pub struct SatResolver { solver: varisat::Solver<'static>, + old_root_vars: Vec, var_for_is_packages_used: HashMap, - by_name: HashMap<&'static str, Vec>, + var_for_is_packages_features_used: HashMap>, + by_name: HashMap>, } impl SatResolver { pub fn new(registry: &[Summary]) -> Self { - let mut cnf = varisat::CnfFormula::new(); + let mut solver = varisat::Solver::new(); + // That represents each package version which is set to "true" if the packages in the lock file and "false" if it is unused. - let var_for_is_packages_used: HashMap = registry + let var_for_is_packages_used = registry + .iter() + .map(|s| (s.package_id(), solver.new_var())) + .collect::>(); + + // That represents each feature of each package version, which is set to "true" if the package feature is used. + let var_for_is_packages_features_used = registry .iter() - .map(|s| (s.package_id(), cnf.new_var())) - .collect(); + .map(|s| { + ( + s.package_id(), + (s.features().keys().map(|&f| (f, solver.new_var()))).collect(), + ) + }) + .collect::>>(); + + // if a package feature is used, then the package is used + for (package, pkg_feature_var_map) in &var_for_is_packages_features_used { + for (_, package_feature_var) in pkg_feature_var_map { + let package_var = var_for_is_packages_used[package]; + solver.add_clause(&[package_feature_var.negative(), package_var.positive()]); + } + } // no two packages with the same links set sat_at_most_one_by_key( - &mut cnf, + &mut solver, registry .iter() .map(|s| (s.links(), var_for_is_packages_used[&s.package_id()])) @@ -276,47 +444,46 @@ impl SatResolver { // no two semver compatible versions of the same package sat_at_most_one_by_key( - &mut cnf, + &mut solver, var_for_is_packages_used .iter() .map(|(p, &v)| (p.as_activations_key(), v)), ); - let mut by_name: HashMap<&'static str, Vec> = HashMap::new(); + let mut by_name: HashMap> = HashMap::new(); - for p in registry.iter() { - by_name - .entry(p.name().as_str()) - .or_default() - .push(p.package_id()) + for p in registry { + by_name.entry(p.name()).or_default().push(p.clone()) } - let empty_vec = vec![]; - - // Now different versions can conflict, but dependencies might not be selected. - // So we need to add clauses that ensure that if a package is selected for each dependency a version - // that satisfies that dependency is selected. - // - // active packages need each of there `deps` to be satisfied - for p in registry.iter() { - for dep in p.dependencies() { - let mut matches: Vec = by_name - .get(dep.package_name().as_str()) - .unwrap_or(&empty_vec) - .iter() - .filter(|&p| dep.matches_id(*p)) - .map(|p| var_for_is_packages_used[&p].positive()) - .collect(); - // ^ the `dep` is satisfied or `p` is not active - matches.push(var_for_is_packages_used[&p.package_id()].negative()); - cnf.add_clause(&matches); - } - } + for pkg in registry { + let pkg_id = pkg.package_id(); + let pkg_dependencies = pkg.dependencies(); + let pkg_features = pkg.features(); + + let compatible_dep_summaries_by_name_in_toml = + find_compatible_dep_summaries_by_name_in_toml(pkg_dependencies, &by_name); + + process_pkg_features( + &mut solver, + &var_for_is_packages_used, + &var_for_is_packages_features_used, + &var_for_is_packages_features_used[&pkg_id], + pkg_features, + &compatible_dep_summaries_by_name_in_toml, + ); - let mut solver = varisat::Solver::new(); - solver.add_formula(&cnf); + process_pkg_dependencies( + &mut solver, + &var_for_is_packages_used, + &var_for_is_packages_features_used, + var_for_is_packages_used[&pkg_id], + pkg_dependencies, + &compatible_dep_summaries_by_name_in_toml, + ); + } - // We dont need to `solve` now. We know that "use nothing" will satisfy all the clauses so far. + // We don't need to `solve` now. We know that "use nothing" will satisfy all the clauses so far. // But things run faster if we let it spend some time figuring out how the constraints interact before we add assumptions. solver .solve() @@ -324,60 +491,80 @@ impl SatResolver { SatResolver { solver, + old_root_vars: Vec::new(), var_for_is_packages_used, + var_for_is_packages_features_used, by_name, } } pub fn sat_resolve(&mut self, root_dependencies: &[Dependency]) -> bool { - let mut assumption = vec![]; - let mut this_call = None; - - // the starting `deps` need to be satisfied - for dep in root_dependencies { - let empty_vec = vec![]; - let matches: Vec = self - .by_name - .get(dep.package_name().as_str()) - .unwrap_or(&empty_vec) - .iter() - .filter(|&p| dep.matches_id(*p)) - .map(|p| self.var_for_is_packages_used[p].positive()) - .collect(); - if matches.is_empty() { - return false; - } else if matches.len() == 1 { - assumption.extend_from_slice(&matches) - } else { - if this_call.is_none() { - let new_var = self.solver.new_var(); - this_call = Some(new_var); - assumption.push(new_var.positive()); - } - let mut matches = matches; - matches.push(this_call.unwrap().negative()); - self.solver.add_clause(&matches); - } - } + let SatResolver { + solver, + old_root_vars, + var_for_is_packages_used, + var_for_is_packages_features_used, + by_name, + } = self; - self.solver.assume(&assumption); + let root_var = solver.new_var(); - self.solver + // root package is always used + // root vars from previous runs are deactivated + let assumption = old_root_vars + .iter() + .map(|v| v.negative()) + .chain([root_var.positive()]) + .collect::>(); + + old_root_vars.push(root_var); + + let compatible_dep_summaries_by_name_in_toml = + find_compatible_dep_summaries_by_name_in_toml(root_dependencies, &by_name); + + process_pkg_dependencies( + solver, + var_for_is_packages_used, + var_for_is_packages_features_used, + root_var, + root_dependencies, + &compatible_dep_summaries_by_name_in_toml, + ); + + solver.assume(&assumption); + + solver .solve() .expect("docs say it can't error in default config") } - pub fn sat_is_valid_solution(&mut self, pids: &[PackageId]) -> bool { - for p in pids { - if p.name().as_str() != "root" && !self.var_for_is_packages_used.contains_key(p) { + pub fn sat_is_valid_solution(&mut self, pkgs: &[(PackageId, Vec)]) -> bool { + let contains_pkg = |pkg| pkgs.iter().any(|(p, _)| p == pkg); + let contains_pkg_feature = + |pkg, f| pkgs.iter().any(|(p, flist)| p == pkg && flist.contains(f)); + + for (p, _) in pkgs { + if p.name() != "root" && !self.var_for_is_packages_used.contains_key(p) { return false; } } - let assumption: Vec<_> = self - .var_for_is_packages_used - .iter() - .map(|(p, v)| v.lit(pids.contains(p))) - .collect(); + + // root vars from previous runs are deactivated + let assumption = (self.old_root_vars.iter().map(|v| v.negative())) + .chain( + self.var_for_is_packages_used + .iter() + .map(|(p, v)| v.lit(contains_pkg(p))), + ) + .chain( + self.var_for_is_packages_features_used + .iter() + .flat_map(|(p, fmap)| { + fmap.iter() + .map(move |(f, v)| v.lit(contains_pkg_feature(p, f))) + }), + ) + .collect::>(); self.solver.assume(&assumption); @@ -393,13 +580,32 @@ impl SatResolver { .filter(|l| l.is_positive()) .map(|l| l.var()) .collect(); - let mut out = String::new(); - out.push_str("used:\n"); - for (p, v) in self.var_for_is_packages_used.iter() { + + let mut used_packages = BTreeMap::>::new(); + for (&p, v) in self.var_for_is_packages_used.iter() { if lits.contains(v) { - writeln!(&mut out, " {}", p).unwrap(); + used_packages.entry(p).or_default(); } } + for (&p, map) in &self.var_for_is_packages_features_used { + for (&f, v) in map { + if lits.contains(v) { + used_packages + .get_mut(&p) + .expect("the feature is activated without the package being activated") + .insert(f); + } + } + } + + let mut out = String::from("used:\n"); + for (package, feature_names) in used_packages { + writeln!(&mut out, " {package}").unwrap(); + for feature_name in feature_names { + writeln!(&mut out, " + {feature_name}").unwrap(); + } + } + out }) } diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 4d254a0c69e..5ac0a83dcbb 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -6,9 +6,10 @@ use cargo::util::GlobalContext; 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, SatResolver, ToDep, ToPkgId, + assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, loc_names, names, pkg, pkg_dep, + pkg_dep_with, pkg_id, pkg_loc, registry, registry_strategy, remove_dep, resolve, + resolve_and_validated, resolve_with_global_context, PrettyPrintRegistry, SatResolver, ToDep, + ToPkgId, }; use proptest::prelude::*; @@ -446,7 +447,10 @@ fn test_resolving_minimum_version_with_transitive_deps() { ®, &gctx, ) - .unwrap(); + .unwrap() + .into_iter() + .map(|(pkg, _)| pkg) + .collect::>(); assert_contains( &res, @@ -1214,7 +1218,7 @@ fn off_by_one_bug() { let reg = registry(input); let mut sat_resolver = SatResolver::new(®); - let _ = resolve_and_validated(vec![dep("f")], ®, &mut sat_resolver); + assert!(resolve_and_validated(vec![dep("f")], ®, &mut sat_resolver).is_ok()); } #[test] @@ -1255,7 +1259,7 @@ fn conflict_store_bug() { let reg = registry(input); let mut sat_resolver = SatResolver::new(®); - let _ = resolve_and_validated(vec![dep("j")], ®, &mut sat_resolver); + assert!(resolve_and_validated(vec![dep("j")], ®, &mut sat_resolver).is_err()); } #[test] @@ -1284,7 +1288,7 @@ fn conflict_store_more_then_one_match() { ]; let reg = registry(input); let mut sat_resolver = SatResolver::new(®); - let _ = resolve_and_validated(vec![dep("nA")], ®, &mut sat_resolver); + assert!(resolve_and_validated(vec![dep("nA")], ®, &mut sat_resolver).is_err()); } #[test] @@ -1310,7 +1314,99 @@ fn bad_lockfile_from_8249() { ]; let reg = registry(input); let mut sat_resolver = SatResolver::new(®); - let _ = resolve_and_validated(vec![dep("foo")], ®, &mut sat_resolver); + assert!(resolve_and_validated(vec![dep("foo")], ®, &mut sat_resolver).is_err()); +} + +#[test] +fn registry_with_features() { + let reg = registry(vec![ + pkg!("a"), + pkg!("b"), + pkg_dep_with( + "image", + vec!["a".to_opt_dep(), "b".to_opt_dep(), "jpg".to_dep()], + &[("default", &["a"]), ("b", &["dep:b"])], + ), + pkg!("jpg"), + pkg!("log"), + pkg!("man"), + pkg_dep_with("rgb", vec!["man".to_opt_dep()], &[("man", &["dep:man"])]), + pkg_dep_with( + "dep", + vec![ + "image".to_dep_with(&["b"]), + "log".to_opt_dep(), + "rgb".to_opt_dep(), + ], + &[ + ("default", &["log", "image/default"]), + ("man", &["rgb?/man"]), + ], + ), + ]); + + for deps in [ + vec!["dep".to_dep_with(&["default", "man", "log", "rgb"])], + vec!["dep".to_dep_with(&["default"])], + vec!["dep".to_dep_with(&[])], + vec!["dep".to_dep_with(&["man"])], + vec!["dep".to_dep_with(&["man", "rgb"])], + ] { + let mut sat_resolver = SatResolver::new(®); + assert!(resolve_and_validated(deps, ®, &mut sat_resolver).is_ok()); + } +} + +#[test] +fn missing_feature() { + let reg = registry(vec![pkg!("a")]); + let mut sat_resolver = SatResolver::new(®); + assert!(resolve_and_validated(vec!["a".to_dep_with(&["f"])], ®, &mut sat_resolver).is_err()); +} + +#[test] +fn conflict_feature_and_sys() { + let reg = registry(vec![ + pkg(("a-sys", "1.0.0")), + pkg(("a-sys", "2.0.0")), + pkg_dep_with( + ("a", "1.0.0"), + vec![dep_req("a-sys", "1.0.0")], + &[("f", &[])], + ), + pkg_dep_with( + ("a", "2.0.0"), + vec![dep_req("a-sys", "2.0.0")], + &[("g", &[])], + ), + pkg_dep("dep", vec![dep_req("a", "2.0.0")]), + ]); + + let deps = vec![dep_req("a", "*").to_dep_with(&["f"]), dep("dep")]; + let mut sat_resolver = SatResolver::new(®); + assert!(resolve_and_validated(deps, ®, &mut sat_resolver).is_err()); +} + +#[test] +fn conflict_weak_features() { + let reg = registry(vec![ + pkg(("a-sys", "1.0.0")), + pkg(("a-sys", "2.0.0")), + pkg_dep("a1", vec![dep_req("a-sys", "1.0.0").to_opt_dep()]), + pkg_dep("a2", vec![dep_req("a-sys", "2.0.0").to_opt_dep()]), + pkg_dep_with( + "dep", + vec!["a1".to_opt_dep(), "a2".to_opt_dep()], + &[("a1", &["a1?/a-sys"]), ("a2", &["a2?/a-sys"])], + ), + ]); + + let deps = vec![dep("dep").to_dep_with(&["a1", "a2"])]; + + // The following asserts should be updated when support for weak dependencies + // is added to the dependency resolver. + assert!(resolve(deps.clone(), ®).is_err()); + assert!(SatResolver::new(®).sat_resolve(&deps)); } #[test]