Skip to content

Commit

Permalink
feat: allow returning unknown dependencies in get_dependencies
Browse files Browse the repository at this point in the history
This makes it possible for a DependencyProvider to signal that it failed
to retrieve a solvable's depenencies (instead of crashing). The
consequence is that the original solvable is marked as excluded.
  • Loading branch information
aochagavia committed Jan 24, 2024
1 parent 22e5357 commit b344fba
Show file tree
Hide file tree
Showing 4 changed files with 145 additions and 35 deletions.
14 changes: 11 additions & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ pub struct Candidates {
/// no solution could be found otherwise.
///
/// The same behavior can be achieved by sorting this candidate to the top using the
/// [`DependencyProvider::sort_candidates`] function but using this method providers better
/// [`DependencyProvider::sort_candidates`] function but using this method provides better
/// error messages to the user.
pub favored: Option<SolvableId>,

Expand All @@ -115,14 +115,22 @@ pub struct Candidates {
}

/// Holds information about the dependencies of a package.
pub enum Dependencies {
/// The dependencies are known.
Known(KnownDependencies),
/// The dependencies are unknown, so the parent solvable should be excluded from the solution.
Unknown,
}

/// Holds information about the dependencies of a package when they are known.
#[derive(Default, Clone, Debug)]
pub struct Dependencies {
pub struct KnownDependencies {
/// Defines which packages should be installed alongside the depending package and the
/// constraints applied to the package.
pub requirements: Vec<VersionSetId>,

/// Defines additional constraints on packages that may or may not be part of the solution.
/// Different from `requirements` packages in this set are not necessarily included in the
/// Different from `requirements`, packages in this set are not necessarily included in the
/// solution. Only when one or more packages list the package in their `requirements` is the
/// package also added to the solution.
///
Expand Down
9 changes: 7 additions & 2 deletions src/solver/clause.rs
Original file line number Diff line number Diff line change
Expand Up @@ -574,8 +574,13 @@ impl<VS: VersionSet, N: PackageName + Display> Debug for ClauseDebug<'_, VS, N>
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
match self.kind {
Clause::InstallRoot => write!(f, "install root"),
Clause::Excluded(_, reason) => {
write!(f, "excluded because {}", self.pool.resolve_string(reason))
Clause::Excluded(solvable_id, reason) => {
write!(
f,
"{} excluded because: {}",
solvable_id.display(self.pool),
self.pool.resolve_string(reason)
)
}
Clause::Learnt(learnt_id) => write!(f, "learnt clause {learnt_id:?}"),
Clause::Requires(solvable_id, match_spec_id) => {
Expand Down
32 changes: 30 additions & 2 deletions src/solver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
pool::Pool,
problem::Problem,
solvable::SolvableInner,
DependencyProvider, PackageName, VersionSet, VersionSetId,
Dependencies, DependencyProvider, PackageName, VersionSet, VersionSetId,
};
use std::collections::HashSet;
use std::fmt::Display;
Expand Down Expand Up @@ -170,7 +170,35 @@ impl<VS: VersionSet, N: PackageName + Display, D: DependencyProvider<VS, N>> Sol
SolvableInner::Root => (self.root_requirements.clone(), Vec::new()),
SolvableInner::Package(_) => {
let deps = self.cache.get_or_cache_dependencies(solvable_id);
(deps.requirements.clone(), deps.constrains.clone())
match deps {
Dependencies::Known(deps) => {
(deps.requirements.clone(), deps.constrains.clone())
}
Dependencies::Unknown => {
// There is no information about the solvable's dependencies, so we add
// an exclusion clause for it
let reason = self
.cache
.pool()
.intern_string("unable to retrieve the package's dependencies");
let clause_id = self
.clauses
.alloc(ClauseState::exclude(solvable_id, reason));

// Exclusions are negative assertions, tracked outside of the watcher system
self.negative_assertions.push((solvable_id, clause_id));

// There should always be a conflict here
debug_assert!(
self.decision_tracker.assigned_value(solvable_id) == Some(true)
);

// The new assertion should be kept (it is returned in the lhs of the
// tuple), but it should also be marked as the source of a conflict (rhs
// of the tuple)
return (vec![clause_id], vec![clause_id]);
}
}
}
};

Expand Down
125 changes: 97 additions & 28 deletions tests/solver.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use indexmap::IndexMap;
use itertools::Itertools;
use resolvo::{
range::Range, Candidates, DefaultSolvableDisplay, Dependencies, DependencyProvider, NameId,
Pool, SolvableId, Solver, SolverCache, VersionSet, VersionSetId,
range::Range, Candidates, DefaultSolvableDisplay, Dependencies, DependencyProvider,
KnownDependencies, NameId, Pool, SolvableId, Solver, SolverCache, VersionSet, VersionSetId,
};
use std::{
collections::HashMap,
Expand All @@ -29,32 +29,56 @@ use tracing_test::traced_test;

/// This is `Pack` which is a unique version and name in our bespoke packaging system
#[derive(Debug, Ord, PartialOrd, Eq, PartialEq, Copy, Clone, Hash)]
#[repr(transparent)]
struct Pack(u32);
struct Pack {
version: u32,
unknown_deps: bool,
}

impl Pack {
fn with_version(version: u32) -> Pack {
Pack {
version,
unknown_deps: false,
}
}

fn offset(&self, version_offset: i32) -> Pack {
Pack {
version: self.version.wrapping_add_signed(version_offset),
unknown_deps: self.unknown_deps,
}
}
}

impl From<u32> for Pack {
fn from(value: u32) -> Self {
Pack(value)
Pack {
version: value,
unknown_deps: false,
}
}
}

impl From<i32> for Pack {
fn from(value: i32) -> Self {
Pack(value as u32)
Pack {
version: value as u32,
unknown_deps: false,
}
}
}

impl Display for Pack {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.0)
write!(f, "{}", self.version)
}
}

impl FromStr for Pack {
type Err = ParseIntError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
u32::from_str(s).map(Self)
u32::from_str(s).map(Pack::with_version)
}
}

Expand Down Expand Up @@ -91,7 +115,7 @@ impl FromStr for Spec {
.map(FromStr::from_str)
.transpose()
.unwrap()
.unwrap_or(Pack(start.0 + 1));
.unwrap_or(start.offset(1));
Range::between(start, end)
} else {
Range::full()
Expand Down Expand Up @@ -139,24 +163,26 @@ impl BundleBoxProvider {
pub fn from_packages(packages: &[(&str, u32, Vec<&str>)]) -> Self {
let mut result = Self::new();
for (name, version, deps) in packages {
result.add_package(name, Pack(*version), deps, &[]);
result.add_package(name, Pack::with_version(*version), deps, &[]);
}
result
}

pub fn set_favored(&mut self, package_name: &str, version: u32) {
self.favored.insert(package_name.to_owned(), Pack(version));
self.favored
.insert(package_name.to_owned(), Pack::with_version(version));
}

pub fn exclude(&mut self, package_name: &str, version: u32, reason: impl Into<String>) {
self.excluded
.entry(package_name.to_owned())
.or_default()
.insert(Pack(version), reason.into());
.insert(Pack::with_version(version), reason.into());
}

pub fn set_locked(&mut self, package_name: &str, version: u32) {
self.locked.insert(package_name.to_owned(), Pack(version));
self.locked
.insert(package_name.to_owned(), Pack::with_version(version));
}

pub fn add_package(
Expand Down Expand Up @@ -205,7 +231,7 @@ impl DependencyProvider<Range<Pack>> for BundleBoxProvider {
let a = self.pool.resolve_solvable(*a).inner();
let b = self.pool.resolve_solvable(*b).inner();
// We want to sort with highest version on top
b.0.cmp(&a.0)
b.version.cmp(&a.version)
});
}

Expand Down Expand Up @@ -243,11 +269,16 @@ impl DependencyProvider<Range<Pack>> for BundleBoxProvider {
let candidate = self.pool.resolve_solvable(solvable);
let package_name = self.pool.resolve_package_name(candidate.name_id());
let pack = candidate.inner();

if pack.unknown_deps {
return Dependencies::Unknown;
}

let Some(deps) = self.packages.get(package_name).and_then(|v| v.get(pack)) else {
return Default::default();
return Dependencies::Known(Default::default());
};

let mut result = Dependencies {
let mut result = KnownDependencies {
requirements: Vec::with_capacity(deps.dependencies.len()),
constrains: Vec::with_capacity(deps.constrains.len()),
};
Expand All @@ -263,7 +294,7 @@ impl DependencyProvider<Range<Pack>> for BundleBoxProvider {
result.constrains.push(dep_spec);
}

result
Dependencies::Known(result)
}
}

Expand Down Expand Up @@ -342,7 +373,7 @@ fn test_unit_propagation_1() {
solver.pool().resolve_package_name(solvable.name_id()),
"asdf"
);
assert_eq!(solvable.inner().0, 1);
assert_eq!(solvable.inner().version, 1);
}

/// Test if we can also select a nested version
Expand All @@ -365,15 +396,15 @@ fn test_unit_propagation_nested() {
solver.pool().resolve_package_name(solvable.name_id()),
"asdf"
);
assert_eq!(solvable.inner().0, 1);
assert_eq!(solvable.inner().version, 1);

let solvable = solver.pool().resolve_solvable(solved[1]);

assert_eq!(
solver.pool().resolve_package_name(solvable.name_id()),
"efgh"
);
assert_eq!(solvable.inner().0, 4);
assert_eq!(solvable.inner().version, 4);
}

/// Test if we can resolve multiple versions at once
Expand All @@ -397,15 +428,15 @@ fn test_resolve_multiple() {
solver.pool().resolve_package_name(solvable.name_id()),
"asdf"
);
assert_eq!(solvable.inner().0, 2);
assert_eq!(solvable.inner().version, 2);

let solvable = solver.pool().resolve_solvable(solved[1]);

assert_eq!(
solver.pool().resolve_package_name(solvable.name_id()),
"efgh"
);
assert_eq!(solvable.inner().0, 5);
assert_eq!(solvable.inner().version, 5);
}

/// In case of a conflict the version should not be selected with the conflict
Expand Down Expand Up @@ -453,7 +484,7 @@ fn test_resolve_with_nonexisting() {
solver.pool().resolve_package_name(solvable.name_id()),
"asdf"
);
assert_eq!(solvable.inner().0, 3);
assert_eq!(solvable.inner().version, 3);
}

#[test]
Expand Down Expand Up @@ -489,7 +520,44 @@ fn test_resolve_with_nested_deps() {
solver.pool().resolve_package_name(solvable.name_id()),
"apache-airflow"
);
assert_eq!(solvable.inner().0, 1);
assert_eq!(solvable.inner().version, 1);
}

#[test]
#[traced_test]
fn test_resolve_with_unknown_deps() {
let mut provider = BundleBoxProvider::new();
provider.add_package(
"opentelemetry-api",
Pack {
version: 3,
unknown_deps: true,
},
&[],
&[],
);
provider.add_package(
"opentelemetry-api",
Pack {
version: 2,
unknown_deps: false,
},
&[],
&[],
);
let requirements = provider.requirements(&["opentelemetry-api"]);
let mut solver = Solver::new(provider);
let solved = solver.solve(requirements).unwrap();

assert_eq!(solved.len(), 1);

let solvable = solver.pool().resolve_solvable(solved[0]);

assert_eq!(
solver.pool().resolve_package_name(solvable.name_id()),
"opentelemetry-api"
);
assert_eq!(solvable.inner().version, 2);
}

/// Locking a specific package version in this case a lower version namely `3` should result
Expand All @@ -507,7 +575,10 @@ fn test_resolve_locked_top_level() {

assert_eq!(solved.len(), 1);
let solvable_id = solved[0];
assert_eq!(solver.pool().resolve_solvable(solvable_id).inner().0, 3);
assert_eq!(
solver.pool().resolve_solvable(solvable_id).inner().version,
3
);
}

/// Should ignore lock when it is not a top level package and a newer version exists without it
Expand All @@ -532,7 +603,7 @@ fn test_resolve_ignored_locked_top_level() {
solver.pool().resolve_package_name(solvable.name_id()),
"asdf"
);
assert_eq!(solvable.inner().0, 4);
assert_eq!(solvable.inner().version, 4);
}

/// Test checks if favoring without a conflict results in a package upgrade
Expand Down Expand Up @@ -567,7 +638,6 @@ fn test_resolve_favor_without_conflict() {
"###);
}

//
#[test]
fn test_resolve_favor_with_conflict() {
let mut provider = BundleBoxProvider::from_packages(&[
Expand Down Expand Up @@ -742,7 +812,6 @@ fn test_unsat_applies_graph_compression() {
insta::assert_snapshot!(error);
}

//
#[test]
fn test_unsat_constrains() {
let mut provider = BundleBoxProvider::from_packages(&[
Expand Down

0 comments on commit b344fba

Please sign in to comment.