From 1da794ac823c181f8801b75991a19b79abd9d949 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Sun, 16 Oct 2022 12:08:38 +0200 Subject: [PATCH] feat: add the ability to define constrains --- examples/caching_dependency_provider.rs | 2 +- src/internal/core.rs | 24 ++++-- src/internal/incompatibility.rs | 15 +++- src/solver.rs | 102 +++++++++++++++++++++--- tests/proptest.rs | 73 ++++++++++------- tests/sat_dependency_provider.rs | 44 +++++++--- tests/tests.rs | 71 +++++++++++++++++ 7 files changed, 270 insertions(+), 61 deletions(-) diff --git a/examples/caching_dependency_provider.rs b/examples/caching_dependency_provider.rs index cb278942..15156ef6 100644 --- a/examples/caching_dependency_provider.rs +++ b/examples/caching_dependency_provider.rs @@ -51,7 +51,7 @@ impl> DependencyProvid let dependencies = self.remote_dependencies.get_dependencies(package, version); match dependencies { Ok(Dependencies::Known(dependencies)) => { - cache.add_dependencies( + cache.add_requirements( package.clone(), version.clone(), dependencies.clone(), diff --git a/src/internal/core.rs b/src/internal/core.rs index f54ae860..e2b17b59 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -15,6 +15,7 @@ use crate::internal::partial_solution::{DecisionLevel, PartialSolution}; use crate::internal::small_vec::SmallVec; use crate::package::Package; use crate::report::DerivationTree; +use crate::solver::Requirement; use crate::type_aliases::{DependencyConstraints, Map}; use crate::version_set::VersionSet; @@ -75,14 +76,25 @@ impl State { &mut self, package: P, version: VS::V, - deps: &DependencyConstraints, + deps: &DependencyConstraints>, ) -> std::ops::Range> { // Create incompatibilities and allocate them in the store. - let new_incompats_id_range = self - .incompatibility_store - .alloc_iter(deps.iter().map(|dep| { - Incompatibility::from_dependency(package.clone(), version.clone(), dep) - })); + let new_incompats_id_range = + self.incompatibility_store + .alloc_iter(deps.iter().map(|(dep, requirement)| match requirement { + Requirement::Required(range) => Incompatibility::from_required_dependency( + package.clone(), + version.clone(), + (dep, range), + ), + Requirement::Constrained(range) => { + Incompatibility::from_constrained_dependency( + package.clone(), + version.clone(), + (dep, range), + ) + } + })); // Merge the newly created incompatibilities with the older ones. for id in IncompId::range_to_iter(new_incompats_id_range.clone()) { self.merge_incompatibility(id); diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 93861621..c4bf5d37 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -105,7 +105,7 @@ impl Incompatibility { } /// Build an incompatibility from a given dependency. - pub fn from_dependency(package: P, version: VS::V, dep: (&P, &VS)) -> Self { + pub fn from_required_dependency(package: P, version: VS::V, dep: (&P, &VS)) -> Self { let set1 = VS::singleton(version); let (p2, set2) = dep; Self { @@ -117,6 +117,19 @@ impl Incompatibility { } } + /// Build an incompatibility from a given constraint. + pub fn from_constrained_dependency(package: P, version: VS::V, dep: (&P, &VS)) -> Self { + let set1 = VS::singleton(version); + let (p2, set2) = dep; + Self { + package_terms: SmallMap::Two([ + (package.clone(), Term::Positive(set1.clone())), + (p2.clone(), Term::Positive(set2.complement())), + ]), + kind: Kind::FromDependencyOf(package, set1, p2.clone(), set2.clone()), + } + } + /// Prior cause of two incompatibilities using the rule of resolution. pub fn prior_cause( incompat: Id, diff --git a/src/solver.rs b/src/solver.rs index 846f220c..3974ef3f 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -166,7 +166,10 @@ pub fn resolve( version: v.clone(), }); } - if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&VS::empty()) { + if let Some((dependent, _)) = x + .iter() + .find(|(_, r)| matches!(r, Requirement::Required(r) if r == &VS::empty())) + { return Err(PubGrubError::DependencyOnTheEmptySet { package: p.clone(), version: v.clone(), @@ -215,7 +218,17 @@ pub enum Dependencies { /// Package dependencies are unavailable. Unknown, /// Container for all available package versions. - Known(DependencyConstraints), + Known(DependencyConstraints>), +} + +/// An enum that defines the type of dependency. +#[derive(Clone, Debug)] +pub enum Requirement { + /// A dependency that is resolved as part of the solution. + Required(VS), + + /// Constrains the version of package but does not require inclusion of the package. + Constrained(VS), } /// Trait that allows the algorithm to retrieve available packages and their dependencies. @@ -311,7 +324,7 @@ where )] #[cfg_attr(feature = "serde", serde(transparent))] pub struct OfflineDependencyProvider { - dependencies: Map>>, + dependencies: Map>>>, } impl OfflineDependencyProvider { @@ -324,10 +337,11 @@ impl OfflineDependencyProvider { /// Registers the dependencies of a package and version pair. /// Dependencies must be added with a single call to - /// [add_dependencies](OfflineDependencyProvider::add_dependencies). - /// All subsequent calls to - /// [add_dependencies](OfflineDependencyProvider::add_dependencies) for a given - /// package version pair will replace the dependencies by the new ones. + /// [add_dependencies](OfflineDependencyProvider::add_dependencies), + /// [add_constraints](OfflineDependencyProvider::add_constraints), or + /// [add_requirements](OfflineDependencyProvider::add_requirements). + /// All subsequent calls to any of those functions for a given package version pair will replace the + /// dependencies by the new ones. /// /// The API does not allow to add dependencies one at a time to uphold an assumption that /// [OfflineDependencyProvider.get_dependencies(p, v)](OfflineDependencyProvider::get_dependencies) @@ -338,14 +352,76 @@ impl OfflineDependencyProvider { version: impl Into, dependencies: I, ) { - let package_deps = dependencies.into_iter().collect(); let v = version.into(); - *self + let entries = self + .dependencies + .entry(package) + .or_default() + .entry(v) + .or_default(); + for (dep, range) in dependencies.into_iter() { + entries.insert(dep, Requirement::Required(range)); + } + } + + /// Registers the dependencies of a package and version pair that may *not* be included in the final + /// solution but if it is included in the solution (through another package for example) it must meet + /// the requirements defined in this function call.. + /// Dependencies must be added with a single call to + /// [add_dependencies](OfflineDependencyProvider::add_dependencies), + /// [add_constraints](OfflineDependencyProvider::add_constraints), or + /// [add_requirements](OfflineDependencyProvider::add_requirements). + /// All subsequent calls to any of those functions for a given package version pair will replace the + /// dependencies by the new ones. + /// + /// The API does not allow to add dependencies one at a time to uphold an assumption that + /// [OfflineDependencyProvider.get_dependencies(p, v)](OfflineDependencyProvider::get_dependencies) + /// provides all dependencies of a given package (p) and version (v) pair. + pub fn add_constraints>( + &mut self, + package: P, + version: impl Into, + dependencies: I, + ) { + let v = version.into(); + let entries = self + .dependencies + .entry(package) + .or_default() + .entry(v) + .or_default(); + for (package, constraint) in dependencies.into_iter() { + entries.insert(package, Requirement::Constrained(constraint)); + } + } + + /// Registers the dependencies of a package and version pair. + /// Dependencies must be added with a single call to + /// [add_dependencies](OfflineDependencyProvider::add_dependencies), + /// [add_constraints](OfflineDependencyProvider::add_constraints), or + /// [add_requirements](OfflineDependencyProvider::add_requirements). + /// All subsequent calls to any of those functions for a given package version pair will replace the + /// dependencies by the new ones. + /// + /// The API does not allow to add dependencies one at a time to uphold an assumption that + /// [OfflineDependencyProvider.get_dependencies(p, v)](OfflineDependencyProvider::get_dependencies) + /// provides all dependencies of a given package (p) and version (v) pair. + pub fn add_requirements)>>( + &mut self, + package: P, + version: impl Into, + dependencies: I, + ) { + let v = version.into(); + let entries = self .dependencies .entry(package) .or_default() .entry(v) - .or_default() = package_deps; + .or_default(); + for (package, req) in dependencies.into_iter() { + entries.insert(package, req); + } } /// Lists packages that have been saved. @@ -361,7 +437,11 @@ impl OfflineDependencyProvider { /// Lists dependencies of a given package and version. /// Returns [None] if no information is available regarding that package and version pair. - fn dependencies(&self, package: &P, version: &VS::V) -> Option> { + fn dependencies( + &self, + package: &P, + version: &VS::V, + ) -> Option>> { self.dependencies.get(package)?.get(version).cloned() } } diff --git a/tests/proptest.rs b/tests/proptest.rs index 8ec22e80..d45ca552 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -1,5 +1,6 @@ // SPDX-License-Identifier: MPL-2.0 +use proptest::bool::weighted; use std::{collections::BTreeSet as Set, error::Error}; use pubgrub::error::PubGrubError; @@ -8,7 +9,7 @@ use pubgrub::range::Range; use pubgrub::report::{DefaultStringReporter, Reporter}; use pubgrub::solver::{ choose_package_with_fewest_versions, resolve, Dependencies, DependencyProvider, - OfflineDependencyProvider, + OfflineDependencyProvider, Requirement, }; use pubgrub::version::{NumberVersion, SemanticVersion}; use pubgrub::version_set::VersionSet; @@ -146,7 +147,12 @@ pub fn registry_strategy( let max_deps = max_versions * (max_crates * (max_crates - 1)) / shrinkage; let raw_version_range = (any::(), any::()); - let raw_dependency = (any::(), any::(), raw_version_range); + let raw_dependency = ( + any::(), + any::(), + weighted(0.9), + raw_version_range, + ); fn order_index(a: Index, b: Index, size: usize) -> (usize, usize) { use std::cmp::{max, min}; @@ -169,20 +175,22 @@ pub fn registry_strategy( ) .prop_map( move |(crate_vers_by_name, raw_dependencies, reverse_alphabetical, complicated_len)| { - let mut list_of_pkgid: Vec<((N, NumberVersion), Option>)> = - crate_vers_by_name - .iter() - .flat_map(|(name, vers)| { - vers.iter().map(move |x| { - ( - (name.clone(), NumberVersion::from(x.0)), - if x.1 { Some(vec![]) } else { None }, - ) - }) + let mut list_of_pkgid: Vec<( + (N, NumberVersion), + Option)>>, + )> = crate_vers_by_name + .iter() + .flat_map(|(name, vers)| { + vers.iter().map(move |x| { + ( + (name.clone(), NumberVersion::from(x.0)), + if x.1 { Some(vec![]) } else { None }, + ) }) - .collect(); + }) + .collect(); let len_all_pkgid = list_of_pkgid.len(); - for (a, b, (c, d)) in raw_dependencies { + for (a, b, required, (c, d)) in raw_dependencies { let (a, b) = order_index(a, b, len_all_pkgid); let (a, b) = if reverse_alphabetical { (b, a) } else { (a, b) }; let ((dep_name, _), _) = list_of_pkgid[a].to_owned(); @@ -194,18 +202,23 @@ pub fn registry_strategy( let (c, d) = order_index(c, d, s.len()); if let (_, Some(deps)) = &mut list_of_pkgid[b] { + let range = if c == 0 && d == s_last_index { + Range::full() + } else if c == 0 { + Range::strictly_lower_than(s[d].0 + 1) + } else if d == s_last_index { + Range::higher_than(s[c].0) + } else if c == d { + Range::singleton(s[c].0) + } else { + Range::between(s[c].0, s[d].0 + 1) + }; deps.push(( dep_name, - if c == 0 && d == s_last_index { - Range::full() - } else if c == 0 { - Range::strictly_lower_than(s[d].0 + 1) - } else if d == s_last_index { - Range::higher_than(s[c].0) - } else if c == d { - Range::singleton(s[c].0) + if required { + Requirement::Required(range) } else { - Range::between(s[c].0, s[d].0 + 1) + Requirement::Constrained(range) }, )) } @@ -224,10 +237,12 @@ pub fn registry_strategy( .collect(); for ((name, ver), deps) in list_of_pkgid { - dependency_provider.add_dependencies( + dependency_provider.add_requirements( name, ver, - deps.unwrap_or_else(|| vec![(bad_name.clone(), Range::full())]), + deps.unwrap_or_else(|| { + vec![(bad_name.clone(), Requirement::Required(Range::full()))] + }), ); } @@ -374,7 +389,7 @@ proptest! { .versions(package) .unwrap().collect(); let version = version_idx.get(&versions); - let dependencies: Vec<(u16, NumVS)> = match dependency_provider + let dependencies: Vec<_> = match dependency_provider .get_dependencies(package, version) .unwrap() { @@ -383,7 +398,7 @@ proptest! { }; if !dependencies.is_empty() { let dependency = dep_idx.get(&dependencies).0; - removed_provider.add_dependencies( + removed_provider.add_requirements( **package, **version, dependencies.into_iter().filter(|x| x.0 != dependency), @@ -442,7 +457,7 @@ proptest! { Dependencies::Unknown => panic!(), Dependencies::Known(deps) => deps, }; - smaller_dependency_provider.add_dependencies(n, v, deps) + smaller_dependency_provider.add_requirements(n, v, deps) } } prop_assert!( @@ -464,7 +479,7 @@ proptest! { Dependencies::Unknown => panic!(), Dependencies::Known(deps) => deps, }; - smaller_dependency_provider.add_dependencies(n, v, deps) + smaller_dependency_provider.add_requirements(n, v, deps) } } prop_assert!( diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs index 97ecab3e..e2268b1e 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MPL-2.0 use pubgrub::package::Package; -use pubgrub::solver::{Dependencies, DependencyProvider, OfflineDependencyProvider}; +use pubgrub::solver::{Dependencies, DependencyProvider, OfflineDependencyProvider, Requirement}; use pubgrub::type_aliases::{Map, SelectedDependencies}; use pubgrub::version_set::VersionSet; use varisat::ExtendFormula; @@ -79,19 +79,37 @@ impl SatResolve { Dependencies::Unknown => panic!(), Dependencies::Known(d) => d, }; - for (p1, range) in &deps { + for (p1, requirement) in &deps { let empty_vec = vec![]; - let mut matches: Vec = all_versions_by_p - .get(p1) - .unwrap_or(&empty_vec) - .iter() - .filter(|(v1, _)| range.contains(v1)) - .map(|(_, var1)| var1.positive()) - .collect(); - // ^ the `dep` is satisfied or - matches.push(var.negative()); - // ^ `p` is not active - cnf.add_clause(&matches); + + match requirement { + Requirement::Required(range) => { + let mut matches = Vec::new(); + for (p1_version, p1_version_var) in + all_versions_by_p.get(p1).unwrap_or(&empty_vec).iter() + { + if range.contains(p1_version) { + matches.push(p1_version_var.positive()); + } + } + + // ^ the `dep` is satisfied or + matches.push(var.negative()); + + // ^ `p` is not active + cnf.add_clause(&matches); + } + Requirement::Constrained(range) => { + for (p1_version, p1_version_var) in + all_versions_by_p.get(p1).unwrap_or(&empty_vec).iter() + { + if !range.contains(p1_version) { + // Either this specific variant is not selected or non-matching dependency versions are not selected. + cnf.add_clause(&[var.negative(), p1_version_var.negative()]); + } + } + } + } } } diff --git a/tests/tests.rs b/tests/tests.rs index b1d05a4a..d8bed7ae 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -3,10 +3,81 @@ use pubgrub::error::PubGrubError; use pubgrub::range::Range; use pubgrub::solver::{resolve, OfflineDependencyProvider}; +use pubgrub::type_aliases::Map; use pubgrub::version::NumberVersion; type NumVS = Range; +#[test] +fn constrains_are_not_in_solution() { + let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); + + dependency_provider.add_dependencies("a", 0, []); + dependency_provider.add_constraints("a", 0, [("c", Range::singleton(1))]); + + // Run the algorithm + let computed_solution = + resolve(&dependency_provider, "a", NumberVersion(0)).expect("a solution was not found"); + + // Solution. + let mut expected_solution = Map::default(); + expected_solution.insert("a", NumberVersion(0)); + + assert_eq!(computed_solution, expected_solution); +} + +#[test] +fn constrains_affect_the_solution() { + let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); + + dependency_provider.add_dependencies("a", 0, [("b", Range::full())]); + dependency_provider.add_constraints("a", 0, [("c", Range::singleton(0))]); + dependency_provider.add_dependencies("b", 0, [("c", Range::full())]); + dependency_provider.add_dependencies("c", 0, []); + dependency_provider.add_dependencies("c", 1, []); + + // Run the algorithm + let computed_solution = + resolve(&dependency_provider, "a", NumberVersion(0)).expect("a solution was not found"); + + // Solution. + let mut expected_solution = Map::default(); + expected_solution.insert("a", NumberVersion(0)); + expected_solution.insert("b", NumberVersion(0)); + expected_solution.insert("c", NumberVersion(0)); + + assert_eq!(computed_solution, expected_solution); +} + +#[test] +fn constrains_can_exclude_dependency_from_the_solution() { + let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new(); + + // "a" depends on "b" but requires that "c" is not included through an empty range constraint. + // version 0 of "b" depends on nothing + // version 1 of "b" depends on "c" + dependency_provider.add_dependencies("a", 0, [("b", Range::full())]); + dependency_provider.add_constraints("a", 0, [("c", Range::empty())]); + dependency_provider.add_dependencies("b", 0, []); + dependency_provider.add_dependencies("b", 1, [("c", Range::full())]); + dependency_provider.add_dependencies("c", 0, []); + + // Run the algorithm + let computed_solution = + resolve(&dependency_provider, "a", NumberVersion(0)).expect("a solution was not found"); + + // Solution. + // + // - The expected result is that "b" version 0 is selected over version 1 because that latest + // version (1) depends on the 'illegal' package "c". + // - Package "c" is not included. + let mut expected_solution = Map::default(); + expected_solution.insert("a", NumberVersion(0)); + expected_solution.insert("b", NumberVersion(0)); + + assert_eq!(computed_solution, expected_solution); +} + #[test] fn same_result_on_repeated_runs() { let mut dependency_provider = OfflineDependencyProvider::<_, NumVS>::new();