diff --git a/src/internal/id.rs b/src/internal/id.rs index bfeef22..1eabbf6 100644 --- a/src/internal/id.rs +++ b/src/internal/id.rs @@ -18,6 +18,21 @@ impl ArenaId for NameId { } } +/// The id associated with a generic string +#[repr(transparent)] +#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug, Ord, PartialOrd)] +pub struct StringId(u32); + +impl ArenaId for StringId { + fn from_usize(x: usize) -> Self { + Self(x as u32) + } + + fn to_usize(self) -> usize { + self.0 as usize + } +} + /// The id associated with a VersionSet. #[repr(transparent)] #[derive(Clone, Default, Copy, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)] diff --git a/src/lib.rs b/src/lib.rs index 6d1131a..a2f8211 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9,6 +9,7 @@ //! comments. #![deny(missing_docs)] + pub(crate) mod internal; mod pool; pub mod problem; @@ -19,7 +20,7 @@ mod solver; use itertools::Itertools; pub use internal::{ - id::{NameId, SolvableId, VersionSetId}, + id::{NameId, SolvableId, StringId, VersionSetId}, mapping::Mapping, }; pub use pool::Pool; @@ -39,6 +40,7 @@ use std::{ /// /// A blanket trait implementation is provided for any type that implements [`Eq`] and [`Hash`]. pub trait PackageName: Eq + Hash {} + impl PackageName for N {} /// A [`VersionSet`] is describes a set of "versions". The trait defines whether a given version @@ -104,6 +106,12 @@ pub struct Candidates { /// solver doesnt actually need this information to form a solution. In general though, if the /// dependencies can easily be provided one should provide them up-front. pub hint_dependencies_available: Vec, + + /// A list of solvables that are available but have been excluded from the solver. For example, + /// a package might be excluded from the solver because it is not compatible with the + /// runtime. The solver will not consider these solvables when forming a solution but will use + /// them in the error message if no solution could be found. + pub excluded: Vec<(SolvableId, StringId)>, } /// Holds information about the dependencies of a package. diff --git a/src/pool.rs b/src/pool.rs index 2bd00b6..24a5ab8 100644 --- a/src/pool.rs +++ b/src/pool.rs @@ -1,5 +1,6 @@ use std::fmt::{Display, Formatter}; +use crate::internal::id::StringId; use crate::{ internal::{ arena::Arena, @@ -26,6 +27,12 @@ pub struct Pool { /// Map from package names to the id of their interned counterpart pub(crate) names_to_ids: FrozenCopyMap, + /// Interned strings + strings: Arena, + + /// Map from package names to the id of their interned counterpart + pub(crate) string_to_ids: FrozenCopyMap, + /// Interned match specs pub(crate) version_sets: Arena, @@ -40,9 +47,10 @@ impl Default for Pool { Self { solvables, - names_to_ids: Default::default(), package_names: Arena::new(), + strings: Arena::new(), + string_to_ids: Default::default(), version_set_to_id: Default::default(), version_sets: Arena::new(), } @@ -55,6 +63,26 @@ impl Pool { Self::default() } + /// Interns a generic string into the `Pool` and returns its `StringId`. Strings are + /// deduplicated. + pub fn intern_string(&self, name: impl Into + AsRef) -> StringId { + if let Some(id) = self.string_to_ids.get_copy(name.as_ref()) { + return id; + } + + let string = name.into(); + let id = self.strings.alloc(string.clone()); + self.string_to_ids.insert_copy(string, id); + id + } + + /// Returns the string associated with the provided [`StringId`]. + /// + /// Panics if the string is not found in the pool. + pub fn resolve_string(&self, string_id: StringId) -> &str { + &self.strings[string_id] + } + /// Interns a package name into the `Pool`, returning its `NameId`. Names are deduplicated. If /// the same name is inserted twice the same `NameId` will be returned. /// diff --git a/src/problem.rs b/src/problem.rs index f7c3acc..e15a455 100644 --- a/src/problem.rs +++ b/src/problem.rs @@ -12,6 +12,7 @@ use petgraph::graph::{DiGraph, EdgeIndex, EdgeReference, NodeIndex}; use petgraph::visit::{Bfs, DfsPostOrder, EdgeRef}; use petgraph::Direction; +use crate::internal::id::StringId; use crate::{ internal::id::{ClauseId, SolvableId, VersionSetId}, pool::Pool, @@ -54,6 +55,12 @@ impl Problem { let clause = &solver.clauses[*clause_id].kind; match clause { Clause::InstallRoot => (), + Clause::Excluded(solvable, reason) => { + tracing::info!("{solvable:?} is excluded"); + let package_node = Self::add_node(&mut graph, &mut nodes, *solvable); + let conflict = ConflictCause::Excluded(*solvable, *reason); + graph.add_edge(root_node, package_node, ProblemEdge::Conflict(conflict)); + } Clause::Learnt(..) => unreachable!(), &Clause::Requires(package_id, version_set_id) => { let package_node = Self::add_node(&mut graph, &mut nodes, package_id); @@ -214,6 +221,8 @@ pub enum ConflictCause { Constrains(VersionSetId), /// It is forbidden to install multiple instances of the same dependency ForbidMultipleInstances, + /// The node was excluded + Excluded(SolvableId, StringId), } /// Represents a node that has been merged with others @@ -291,6 +300,9 @@ impl ProblemGraph { | ProblemEdge::Conflict(ConflictCause::Locked(_)) => { "already installed".to_string() } + ProblemEdge::Conflict(ConflictCause::Excluded(_, reason)) => { + format!("excluded because {}", pool.resolve_string(*reason)) + } }; let target = match target { @@ -399,6 +411,19 @@ impl ProblemGraph { continue; } + // Determine any incoming "exclude" edges to the node. This would indicate that the + // node is disabled for external reasons. + let excluding_edges = self.graph.edges_directed(nx, Direction::Incoming).any(|e| { + matches!( + e.weight(), + ProblemEdge::Conflict(ConflictCause::Excluded(_, _)) + ) + }); + if excluding_edges { + // Nodes with incoming disabling edges aren't installable + continue; + } + let outgoing_conflicts = self .graph .edges_directed(nx, Direction::Outgoing) @@ -645,6 +670,14 @@ impl<'pool, VS: VersionSet, N: PackageName + Display, M: SolvableDisplay> .display_candidates(self.pool, &[solvable_id]) }; + let excluded = graph + .edges_directed(candidate, Direction::Incoming) + .find_map(|e| match e.weight() { + ProblemEdge::Conflict(ConflictCause::Excluded(_, reason)) => { + Some(reason) + } + _ => None, + }); let already_installed = graph.edges(candidate).any(|e| { e.weight() == &ProblemEdge::Conflict(ConflictCause::ForbidMultipleInstances) }); @@ -656,7 +689,13 @@ impl<'pool, VS: VersionSet, N: PackageName + Display, M: SolvableDisplay> }); let is_leaf = graph.edges(candidate).next().is_none(); - if is_leaf { + if let Some(excluded_reason) = excluded { + writeln!( + f, + "{indent}{name} {version} is excluded because {reason}", + reason = self.pool.resolve_string(*excluded_reason) + )?; + } else if is_leaf { writeln!(f, "{indent}{name} {version}")?; } else if already_installed { writeln!(f, "{indent}{name} {version}, which conflicts with the versions reported above.")?; @@ -740,21 +779,22 @@ impl> fmt::D }; // The only possible conflict at the root level is a Locked conflict - let locked_id = match conflict { + match conflict { ConflictCause::Constrains(_) | ConflictCause::ForbidMultipleInstances => { unreachable!() } - &ConflictCause::Locked(solvable_id) => solvable_id, + &ConflictCause::Locked(solvable_id) => { + let locked = self.pool.resolve_solvable(solvable_id); + writeln!( + f, + "{indent}{} {} is locked, but another version is required as reported above", + locked.name.display(self.pool), + self.merged_solvable_display + .display_candidates(self.pool, &[solvable_id]) + )?; + } + ConflictCause::Excluded(_, _) => continue, }; - - let locked = self.pool.resolve_solvable(locked_id); - writeln!( - f, - "{indent}{} {} is locked, but another version is required as reported above", - locked.name.display(self.pool), - self.merged_solvable_display - .display_candidates(self.pool, &[locked_id]) - )?; } } diff --git a/src/solver/clause.rs b/src/solver/clause.rs index f39a232..7226068 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -8,6 +8,7 @@ use crate::{ PackageName, VersionSet, }; +use crate::internal::id::StringId; use elsa::FrozenMap; use std::fmt::{Debug, Display, Formatter}; use std::hash::Hash; @@ -76,6 +77,9 @@ pub(crate) enum Clause { /// The learnt clause id can be used to retrieve the clause's literals, which are stored /// elsewhere to prevent the size of [`Clause`] from blowing up Learnt(LearntClauseId), + + /// A clause that forbids a package from being installed for an external reason. + Excluded(SolvableId, StringId), } impl Clause { @@ -122,6 +126,10 @@ impl Clause { (Clause::InstallRoot, None) } + fn exclude(candidate: SolvableId, reason: StringId) -> (Self, Option<[SolvableId; 2]>) { + (Clause::Excluded(candidate, reason), None) + } + fn lock( locked_candidate: SolvableId, other_candidate: SolvableId, @@ -160,6 +168,12 @@ impl Clause { ) { match *self { Clause::InstallRoot => unreachable!(), + Clause::Excluded(solvable, _) => { + visit(Literal { + solvable_id: solvable, + negate: true, + }); + } Clause::Learnt(learnt_id) => { for &literal in &learnt_clauses[learnt_id] { visit(literal); @@ -265,6 +279,11 @@ impl ClauseState { Self::from_kind_and_initial_watches(kind, watched_literals) } + pub fn exclude(candidate: SolvableId, reason: StringId) -> Self { + let (kind, watched_literals) = Clause::exclude(candidate, reason); + Self::from_kind_and_initial_watches(kind, watched_literals) + } + fn from_kind_and_initial_watches( kind: Clause, watched_literals: Option<[SolvableId; 2]>, @@ -368,6 +387,7 @@ impl ClauseState { match self.kind { Clause::InstallRoot => unreachable!(), + Clause::Excluded(_, _) => unreachable!(), Clause::Learnt(learnt_id) => { // TODO: we might want to do something else for performance, like keeping the whole // literal in `self.watched_literals`, to avoid lookups... But first we should @@ -413,6 +433,7 @@ impl ClauseState { match self.kind { Clause::InstallRoot => unreachable!(), + Clause::Excluded(_, _) => unreachable!(), Clause::Learnt(learnt_id) => learnt_clauses[learnt_id] .iter() .cloned() @@ -486,6 +507,9 @@ impl 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::Learnt(learnt_id) => write!(f, "learnt clause {learnt_id:?}"), Clause::Requires(solvable_id, match_spec_id) => { let match_spec = self.pool.resolve_version_set(match_spec_id).to_string(); diff --git a/src/solver/mod.rs b/src/solver/mod.rs index e49fbd8..8cfc83f 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -49,7 +49,7 @@ pub struct Solver> root_requirements: Vec, } -impl> Solver { +impl> Solver { /// Create a solver, using the provided pool pub fn new(provider: D) -> Self { Self { @@ -71,7 +71,9 @@ impl> Sol pub fn pool(&self) -> &Pool { self.cache.pool() } +} +impl> Solver { /// Solves the provided `jobs` and returns a transaction from the found solution /// /// Returns a [`Problem`] if no solution was found, which provides ways to inspect the causes @@ -283,6 +285,21 @@ impl> Sol } } + // Add a clause for solvables that are externally excluded. + for (solvable, reason) in package_candidates.excluded.iter().copied() { + let clause_id = self.clauses.alloc(ClauseState::exclude(solvable, reason)); + + // Immediately add the decision to unselect this solvable. This should be fine because + // no other decision should have been made about this solvable in the first place. + let exclude_descision = Decision::new(solvable, false, clause_id); + self.decision_tracker + .try_add_fixed_assignment(exclude_descision) + .expect("already decided about a solvable that wasn't properly added yet."); + + // No need to add watches for this clause because the clause is an assertion on which + // we already decided. + } + self.clauses_added_for_package.insert(package_name); } @@ -460,27 +477,31 @@ impl> Sol let mut made_a_decision = false; for &clause_id in clauses.iter() { let clause = &self.clauses[clause_id]; - if let Clause::Requires(solvable_id, _) = clause.kind { - if !clause.has_watches() { - // A requires clause without watches means it has a single literal (i.e. - // there are no candidates) - let decided = match self - .decision_tracker - .try_add_fixed_assignment(Decision::new(solvable_id, false, clause_id)) - { - Ok(decided) => decided, - Err(_) => { - conflicting_clause = Some(clause_id); - true - } - }; + let decision = match clause.kind { + // A requires clause without watches means it has a single literal (i.e. + // there are no candidates) + Clause::Requires(solvable_id, _) if !clause.has_watches() => self + .decision_tracker + .try_add_fixed_assignment(Decision::new(solvable_id, false, clause_id)) + .map(|b| b.then_some(solvable_id)), + _ => continue, + }; - if decided { - tracing::debug!("Set {} = false", solvable_id.display(self.pool())); - made_a_decision = true; - } + match decision { + Ok(Some(solvable_id)) => { + // We made a decision without conflict. + tracing::debug!("Set {} = false", solvable_id.display(self.pool())); + made_a_decision = true; } - } + Err(_) => { + // Decision immediately caused a conflict! + conflicting_clause = Some(clause_id); + made_a_decision = true; + } + Ok(None) => { + // No new decision + } + }; } match conflicting_clause { @@ -555,15 +576,24 @@ impl> Sol None => (count, possible_decision), Some((best_count, _)) if count < best_count => { (count, possible_decision) - }, + } Some(best_decision) => best_decision, }) - }, + } ControlFlow::Break(_) => continue, ControlFlow::Continue((None, _)) => unreachable!("when we get here it means that all candidates have been assigned false. This should not be able to happen at this point because during propagation the solvable should have been assigned false as well."), } } + if let Some((count, (candidate, solvable_id, _clause_id))) = best_decision { + tracing::info!( + "deciding to assign {} with {} candidates (required by {})", + candidate.display(self.pool()), + count, + solvable_id.display(self.pool()) + ); + } + // Could not find a requirement that needs satisfying. best_decision.map(|d| d.1) } diff --git a/tests/snapshots/solver__excluded.snap b/tests/snapshots/solver__excluded.snap new file mode 100644 index 0000000..9bae558 --- /dev/null +++ b/tests/snapshots/solver__excluded.snap @@ -0,0 +1,13 @@ +--- +source: tests/solver.rs +expression: "solve_snapshot(provider, &[\"a\"])" +--- +The following packages are incompatible +|-- a * cannot be installed because there are no viable options: + |-- a 2 would require + |-- b *, which cannot be installed because there are no viable options: + |-- b 1 is excluded because it is externally excluded + |-- a 1 would require + |-- c *, which cannot be installed because there are no viable options: + |-- c 1 is excluded because it is externally excluded + diff --git a/tests/snapshots/solver__merge_excluded.snap b/tests/snapshots/solver__merge_excluded.snap new file mode 100644 index 0000000..3370895 --- /dev/null +++ b/tests/snapshots/solver__merge_excluded.snap @@ -0,0 +1,9 @@ +--- +source: tests/solver.rs +expression: "solve_snapshot(provider, &[\"a\"])" +--- +The following packages are incompatible +|-- a * cannot be installed because there are no viable options: + |-- a 2 is excluded because it is externally excluded + |-- a 1 is excluded because it is externally excluded + diff --git a/tests/snapshots/solver__root_excluded.snap b/tests/snapshots/solver__root_excluded.snap new file mode 100644 index 0000000..d9a1002 --- /dev/null +++ b/tests/snapshots/solver__root_excluded.snap @@ -0,0 +1,8 @@ +--- +source: tests/solver.rs +expression: "solve_snapshot(provider, &[\"a\"])" +--- +The following packages are incompatible +|-- a * cannot be installed because there are no viable options: + |-- a 1 is excluded because it is externally excluded + diff --git a/tests/solver.rs b/tests/solver.rs index b5f8ef6..01a0384 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -7,6 +7,8 @@ use resolvo::{ use std::{ collections::HashMap, fmt::{Debug, Display, Formatter}, + io::stderr, + io::Write, num::ParseIntError, str::FromStr, }; @@ -109,6 +111,7 @@ struct BundleBoxProvider { packages: IndexMap>, favored: HashMap, locked: HashMap, + excluded: HashMap>, } struct BundleBoxPackageDependencies { @@ -145,6 +148,13 @@ impl BundleBoxProvider { self.favored.insert(package_name.to_owned(), Pack(version)); } + pub fn exclude(&mut self, package_name: &str, version: u32, reason: impl Into) { + self.excluded + .entry(package_name.to_owned()) + .or_default() + .insert(Pack(version), reason.into()); + } + pub fn set_locked(&mut self, package_name: &str, version: u32) { self.locked.insert(package_name.to_owned(), Pack(version)); } @@ -209,6 +219,7 @@ impl DependencyProvider> for BundleBoxProvider { }; let favor = self.favored.get(package_name); let locked = self.locked.get(package_name); + let excluded = self.excluded.get(package_name); for pack in package.keys() { let solvable = self.pool.intern_solvable(name, *pack); candidates.candidates.push(solvable); @@ -218,6 +229,11 @@ impl DependencyProvider> for BundleBoxProvider { if Some(pack) == locked { candidates.locked = Some(solvable); } + if let Some(excluded) = excluded.and_then(|d| d.get(pack)) { + candidates + .excluded + .push((solvable, self.pool.intern_string(excluded))); + } } Some(candidates) @@ -273,9 +289,19 @@ fn solve_unsat(provider: BundleBoxProvider, specs: &[&str]) -> String { let mut solver = Solver::new(provider); match solver.solve(requirements) { Ok(_) => panic!("expected unsat, but a solution was found"), - Err(problem) => problem - .display_user_friendly(&solver, &DefaultSolvableDisplay) - .to_string(), + Err(problem) => { + // Write the problem graphviz to stderr + let graph = problem.graph(&solver); + let mut output = stderr(); + writeln!(output, "UNSOLVABLE:").unwrap(); + graph.graphviz(&mut output, solver.pool(), true).unwrap(); + writeln!(output, "\n").unwrap(); + + // Format a user friendly error message + problem + .display_user_friendly(&solver, &DefaultSolvableDisplay) + .to_string() + } } } @@ -285,9 +311,19 @@ fn solve_snapshot(provider: BundleBoxProvider, specs: &[&str]) -> String { let mut solver = Solver::new(provider); match solver.solve(requirements) { Ok(solvables) => transaction_to_string(solver.pool(), &solvables), - Err(problem) => problem - .display_user_friendly(&solver, &DefaultSolvableDisplay) - .to_string(), + Err(problem) => { + // Write the problem graphviz to stderr + let graph = problem.graph(&solver); + let mut output = stderr(); + writeln!(output, "UNSOLVABLE:").unwrap(); + graph.graphviz(&mut output, solver.pool(), true).unwrap(); + writeln!(output, "\n").unwrap(); + + // Format a user friendly error message + problem + .display_user_friendly(&solver, &DefaultSolvableDisplay) + .to_string() + } } } @@ -553,8 +589,7 @@ fn test_unsat_locked_and_excluded() { ("c", 1, vec![]), ]); provider.set_locked("c", 1); - let error = solve_unsat(provider, &["asdf"]); - insta::assert_snapshot!(error); + insta::assert_snapshot!(solve_snapshot(provider, &["asdf"])); } #[test] @@ -748,3 +783,31 @@ fn test_incremental_crash() { ]); insta::assert_snapshot!(solve_snapshot(provider, &["a"])); } + +#[test] +fn test_excluded() { + let mut provider = BundleBoxProvider::from_packages(&[ + ("a", 2, vec!["b"]), + ("a", 1, vec!["c"]), + ("b", 1, vec![]), + ("c", 1, vec![]), + ]); + provider.exclude("b", 1, "it is externally excluded"); + provider.exclude("c", 1, "it is externally excluded"); + insta::assert_snapshot!(solve_snapshot(provider, &["a"])); +} + +#[test] +fn test_merge_excluded() { + let mut provider = BundleBoxProvider::from_packages(&[("a", 1, vec![]), ("a", 2, vec![])]); + provider.exclude("a", 1, "it is externally excluded"); + provider.exclude("a", 2, "it is externally excluded"); + insta::assert_snapshot!(solve_snapshot(provider, &["a"])); +} + +#[test] +fn test_root_excluded() { + let mut provider = BundleBoxProvider::from_packages(&[("a", 1, vec![])]); + provider.exclude("a", 1, "it is externally excluded"); + insta::assert_snapshot!(solve_snapshot(provider, &["a"])); +}