Skip to content

Commit

Permalink
feat!: replace list_available_versions with make_decision
Browse files Browse the repository at this point in the history
  • Loading branch information
Eh2406 committed Oct 24, 2020
1 parent 82dad87 commit daca2b6
Show file tree
Hide file tree
Showing 7 changed files with 119 additions and 131 deletions.
7 changes: 5 additions & 2 deletions examples/caching_dependency_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,11 @@ impl<P: Package, V: Version + Hash, DP: DependencyProvider<P, V>> DependencyProv
for CachingDependencyProvider<P, V, DP>
{
// Lists only from remote for simplicity
fn list_available_versions(&self, package: &P) -> Result<Vec<V>, Box<dyn Error>> {
self.remote_dependencies.list_available_versions(package)
fn make_decision<T: std::borrow::Borrow<P>>(
&self,
packages: impl Iterator<Item = (T, impl pubgrub::solver::Constraints<V>)>,
) -> Result<(T, Option<V>), Box<dyn Error>> {
self.remote_dependencies.make_decision(packages)
}

// Caches dependencies if they were already queried
Expand Down
19 changes: 3 additions & 16 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,6 @@ pub enum PubGrubError<P: Package, V: Version> {
#[error("No solution")]
NoSolution(DerivationTree<P, V>),

/// Error arising when the implementer of
/// [DependencyProvider](crate::solver::DependencyProvider)
/// returned an error in the method
/// [list_available_versions](crate::solver::DependencyProvider::list_available_versions).
#[error("Retrieving available versions of package {package} failed")]
ErrorRetrievingVersions {
/// Package for which we want the list of versions.
package: P,
/// Error raised by the implementer of
/// [DependencyProvider](crate::solver::DependencyProvider).
source: Box<dyn std::error::Error>,
},

/// Error arising when the implementer of
/// [DependencyProvider](crate::solver::DependencyProvider)
/// returned an error in the method
Expand All @@ -44,9 +31,9 @@ pub enum PubGrubError<P: Package, V: Version> {
},

/// Error arising when the implementer of [DependencyProvider](crate::solver::DependencyProvider)
/// returned an error in the method [pick_package](crate::solver::DependencyProvider::pick_package).
#[error("Pick package failed")]
ErrorPickPackage(Box<dyn std::error::Error>),
/// returned an error in the method [make_decision](crate::solver::DependencyProvider::make_decision).
#[error("Make decision failed")]
ErrorMakeDecision(Box<dyn std::error::Error>),

/// Error arising when the implementer of [DependencyProvider](crate::solver::DependencyProvider)
/// returned an error in the method [should_cancel](crate::solver::DependencyProvider::should_cancel).
Expand Down
13 changes: 0 additions & 13 deletions src/internal/partial_solution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,19 +100,6 @@ impl<P: Package, V: Version> PartialSolution<P, V> {
}
}

/// Pub chooses the latest matching version of the package
/// that match the outstanding constraint.
///
/// Here we just pick the first one that satisfies the terms.
/// It is the responsibility of the provider of `available_versions`
/// to list them with preferred versions first.
pub fn pick_version(available_versions: &[V], partial_solution_term: &Term<V>) -> Option<V> {
available_versions
.iter()
.find(|v| partial_solution_term.contains(v))
.cloned()
}

/// We can add the version to the partial solution as a decision
/// if it doesn't produce any conflict with the new incompatibilities.
/// In practice I think it can only produce a conflict if one of the dependencies
Expand Down
8 changes: 3 additions & 5 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,17 @@
//! and [SemanticVersion](version::SemanticVersion) for versions.
//! This may be done quite easily by implementing the two following functions.
//! ```
//! # use pubgrub::solver::DependencyProvider;
//! # use pubgrub::solver::{DependencyProvider, Constraints};
//! # use pubgrub::version::SemanticVersion;
//! # use std::error::Error;
//! # use pubgrub::range::Range;
//! # use pubgrub::type_aliases::Map;
//! # use std::borrow::Borrow;
//! #
//! # struct MyDependencyProvider;
//! #
//! impl DependencyProvider<String, SemanticVersion> for MyDependencyProvider {
//! fn list_available_versions(
//! &self,
//! package: &String
//! ) -> Result<Vec<SemanticVersion>, Box<dyn Error>> {
//! fn make_decision<T: Borrow<String>>(&self,packages: impl Iterator<Item=(T, impl Constraints<SemanticVersion>)>) -> Result<(T, Option<SemanticVersion>), Box<dyn Error>> {
//! unimplemented!()
//! }
//!
Expand Down
141 changes: 64 additions & 77 deletions src/solver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ use std::hash::Hash;
use crate::error::PubGrubError;
use crate::internal::core::State;
use crate::internal::incompatibility::Incompatibility;
use crate::internal::partial_solution::PartialSolution;
use crate::package::Package;
use crate::range::Range;
use crate::term::Term;
Expand All @@ -97,42 +96,41 @@ pub fn resolve<P: Package, V: Version>(

state.unit_propagation(next.clone())?;

let term = {
let iter = state.partial_solution.potential_packages();
if iter.is_none() {
drop(iter);
return state
.partial_solution
.extract_solution()
.ok_or(PubGrubError::Failure(
"How did we end up with no package to choose but no solution?".into(),
));
}
next = dependency_provider
.pick_package(iter.unwrap())
.map_err(PubGrubError::ErrorPickPackage)?
.clone();
state.partial_solution.term_intersection_for_package(&next)
};

let available_versions =
dependency_provider
.list_available_versions(&next)
.map_err(|err| PubGrubError::ErrorRetrievingVersions {
package: next.clone(),
source: err,
})?;
let iter = state.partial_solution.potential_packages();
if iter.is_none() {
drop(iter);
return state
.partial_solution
.extract_solution()
.ok_or(PubGrubError::Failure(
"How did we end up with no package to choose but no solution?".into(),
));
}
let decision = dependency_provider
.make_decision(iter.unwrap())
.map_err(PubGrubError::ErrorMakeDecision)?;
next = decision.0.clone();

// Pick the next compatible version.
let v = match PartialSolution::<P, V>::pick_version(&available_versions[..], &term) {
let v = match decision.1 {
None => {
let term = state.partial_solution.term_intersection_for_package(&next);
state.add_incompatibility(|id| {
Incompatibility::no_version(id, next.clone(), term.clone())
});
continue;
}
Some(x) => x,
};
if !state
.partial_solution
.term_intersection_for_package(&next)
.contains(&v)
{
return Err(PubGrubError::Failure(
"make_decision rendered a version that did not match its Constraints".into(),
));
}

// Retrieve that package dependencies.
let dependencies = match dependency_provider
Expand Down Expand Up @@ -195,10 +193,20 @@ impl<V: Version> Constraints<V> for &Term<V> {
/// Trait that allows the algorithm to retrieve available packages and their dependencies.
/// An implementor needs to be supplied to the [resolve] function.
pub trait DependencyProvider<P: Package, V: Version> {
/// Lists available versions for a given package.
/// The strategy of which version should be preferably picked in the list of available versions
/// is implied by the order of the list: first version in the list will be tried first.
fn list_available_versions(&self, package: &P) -> Result<Vec<V>, Box<dyn Error>>;
/// Chooses which package the [resolve] should decide on next.
/// If there are available versions for that package returns `Some(V)` to try.
/// If there are not available versions for that package returns `None`.
///
/// This method holds a lot of control over the performance and behaviour of the algorithm.
/// It is normal to pick the package that has the fewest versions contained in the constraints,
/// as they are likely to find problems faster. Also normal is to pick the most recent matching
/// version.
///
/// Note: the type `T` ensures that this returns an item from the `packages` argument.
fn make_decision<T: Borrow<P>>(
&self,
packages: impl Iterator<Item = (T, impl Constraints<V>)>,
) -> Result<(T, Option<V>), Box<dyn Error>>;

/// Retrieves the package dependencies.
/// Return None if its dependencies are unknown.
Expand All @@ -208,30 +216,6 @@ pub trait DependencyProvider<P: Package, V: Version> {
version: &V,
) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>>;

/// Chooses which package the [resolve] should decide on next.
/// The default implementation picks the package that has the fewest versions contained in the constraints.
/// Override to avoid the calls to [list_available_versions], or to use a different heuristic.
/// Note: the type `T` ensures that this returns an item from the `packages` argument.
fn pick_package<T: Borrow<P>>(
&self,
packages: impl Iterator<Item = (T, impl Constraints<V>)>,
) -> Result<T, Box<dyn Error>> {
let mut out: Option<T> = None;
let mut min_key = usize::MAX;
for (p, term) in packages {
let key = self
.list_available_versions(p.borrow())?
.iter()
.filter(|&v| term.contains(v))
.count();
if key < min_key {
min_key = key;
out = Some(p);
}
}
Ok(out.expect("get_dependencies gave us an empty iterator"))
}

/// This is called fairly regularly during the resolution,
/// if it returns an Err then resolution will be terminated.
/// This is helpful if you want to add some form of early termination like a timeout,
Expand Down Expand Up @@ -291,7 +275,7 @@ impl<P: Package, V: Version + Hash> OfflineDependencyProvider<P, V> {

/// Lists versions of saved packages.
/// Returns [None] if no information is available regarding that package.
fn versions(&self, package: &P) -> Option<Set<V>> {
pub fn versions(&self, package: &P) -> Option<Set<V>> {
self.dependencies
.get(package)
.map(|k| k.keys().cloned().collect())
Expand All @@ -309,28 +293,14 @@ impl<P: Package, V: Version + Hash> OfflineDependencyProvider<P, V> {

/// An implementation of [DependencyProvider] that
/// contains all dependency information available in memory.
/// Versions are listed with the newest versions first.
/// Packages are picked with the fewest versions contained in the constraints first.
/// Versions are picked with the newest versions first.
impl<P: Package, V: Version + Hash> DependencyProvider<P, V> for OfflineDependencyProvider<P, V> {
fn list_available_versions(&self, package: &P) -> Result<Vec<V>, Box<dyn Error>> {
Ok(self
.versions(package)
.map(|v| v.into_iter().rev().collect())
.unwrap_or(Vec::new()))
}

fn get_dependencies(
&self,
package: &P,
version: &V,
) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>> {
Ok(self.dependencies(package, version))
}

fn pick_package<T: Borrow<P>>(
fn make_decision<T: Borrow<P>>(
&self,
packages: impl Iterator<Item = (T, impl Constraints<V>)>,
) -> Result<T, Box<dyn Error>> {
let mut out: Option<T> = None;
) -> Result<(T, Option<V>), Box<dyn Error>> {
let mut package: Option<(T, _)> = None;
let mut min_key = usize::MAX;
for (p, term) in packages {
let key = self
Expand All @@ -343,9 +313,26 @@ impl<P: Package, V: Version + Hash> DependencyProvider<P, V> for OfflineDependen

if key < min_key {
min_key = key;
out = Some(p);
package = Some((p, term));
}
}
Ok(out.expect("get_dependencies gave us an empty iterator"))
let (package, term) = package.expect("potential_packages gave us an empty iterator");
let ver = self
.dependencies
.get(package.borrow())
.iter()
.flat_map(|k| k.keys())
.filter(|&v| term.contains(v))
.max()
.cloned();
Ok((package, ver))
}

fn get_dependencies(
&self,
package: &P,
version: &V,
) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>> {
Ok(self.dependencies(package, version))
}
}
60 changes: 43 additions & 17 deletions tests/proptest.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: MPL-2.0

use std::{collections::BTreeSet as Set, error::Error};
use std::{collections::BTreeSet as Set, error::Error, hash::Hash};

use pubgrub::range::Range;
use pubgrub::solver::{resolve, DependencyProvider, OfflineDependencyProvider};
Expand All @@ -18,19 +18,42 @@ use proptest::string::string_regex;
mod sat_dependency_provider;
use crate::sat_dependency_provider::SatResolve;

/// The same as DP but takes versions from the opposite end:
/// if DP returns versions from newest to oldest, this returns them from oldest to newest.
/// The same as [OfflineDependencyProvider] but takes versions from the opposite end:
/// if [OfflineDependencyProvider] returns versions from newest to oldest, this returns them from oldest to newest.
#[derive(Clone)]
struct ReverseDependencyProvider<DP>(DP);
struct OldestDependencyProvider<P: Package, V: Version + Hash>(OfflineDependencyProvider<P, V>);

impl<P: Package, V: Version + Hash> DependencyProvider<P, V> for OldestDependencyProvider<P, V> {
fn make_decision<T: std::borrow::Borrow<P>>(
&self,
packages: impl Iterator<Item = (T, impl pubgrub::solver::Constraints<V>)>,
) -> Result<(T, Option<V>), Box<dyn Error>> {
let mut package: Option<(T, _)> = None;
let mut min_key = usize::MAX;
for (p, term) in packages {
let key = self
.0
.versions(p.borrow())
.iter()
.flat_map(|k| k.iter())
.filter(|&v| term.contains(v))
.count();

impl<P: Package, V: Version, DP: DependencyProvider<P, V>> DependencyProvider<P, V>
for ReverseDependencyProvider<DP>
{
fn list_available_versions(&self, p: &P) -> Result<Vec<V>, Box<dyn Error>> {
self.0.list_available_versions(p).map(|mut v| {
v.reverse();
v
})
if key < min_key {
min_key = key;
package = Some((p, term));
}
}
let (package, term) = package.expect("potential_packages gave us an empty iterator");
let ver = self
.0
.versions(package.borrow())
.iter()
.flat_map(|k| k.iter())
.filter(|&v| term.contains(v))
.min()
.cloned();
Ok((package, ver))
}

fn get_dependencies(&self, p: &P, v: &V) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>> {
Expand Down Expand Up @@ -61,8 +84,11 @@ impl<DP> TimeoutDependencyProvider<DP> {
impl<P: Package, V: Version, DP: DependencyProvider<P, V>> DependencyProvider<P, V>
for TimeoutDependencyProvider<DP>
{
fn list_available_versions(&self, p: &P) -> Result<Vec<V>, Box<dyn Error>> {
self.dp.list_available_versions(p)
fn make_decision<T: std::borrow::Borrow<P>>(
&self,
packages: impl Iterator<Item = (T, impl pubgrub::solver::Constraints<V>)>,
) -> Result<(T, Option<V>), Box<dyn Error>> {
self.dp.make_decision(packages)
}

fn get_dependencies(&self, p: &P, v: &V) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>> {
Expand Down Expand Up @@ -341,7 +367,7 @@ proptest! {
fn prop_reversed_version_errors_the_same(
(dependency_provider, cases) in registry_strategy(0u16..665, 666)
) {
let reverse_provider = ReverseDependencyProvider(dependency_provider.clone());
let reverse_provider = OldestDependencyProvider(dependency_provider.clone());
for (name, ver) in cases {
let l = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver);
let r = resolve(&TimeoutDependencyProvider::new(reverse_provider.clone(), 50_000), name, ver);
Expand All @@ -362,7 +388,7 @@ proptest! {
let mut removed_provider = dependency_provider.clone();
for (package_idx, version_idx, dep_idx) in indexes_to_remove {
let package = package_idx.get(&packages);
let versions = dependency_provider.list_available_versions(package).unwrap();
let versions: Vec<_> = dependency_provider.versions(package).unwrap().into_iter().collect();
let version = version_idx.get(&versions);
let dependencys: Vec<_> = dependency_provider.get_dependencies(package, version).unwrap().unwrap().into_iter().collect();
if !dependencys.is_empty() {
Expand Down Expand Up @@ -395,7 +421,7 @@ proptest! {
.packages()
.flat_map(|&p| {
dependency_provider
.list_available_versions(&p)
.versions(&p)
.unwrap()
.into_iter()
.map(move |v| (p, v))
Expand Down
Loading

0 comments on commit daca2b6

Please sign in to comment.