From 0586e742e6b94e76af1a7415054591a6050a15e4 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 17 Jan 2025 15:12:31 -0500 Subject: [PATCH 01/41] initial commit for conditional dependencies support --- cpp/include/resolvo.h | 9 ++++ cpp/src/lib.rs | 20 +++++++++ src/conflict.rs | 89 ++++++++++++++++++++++++++++++++++--- src/requirement.rs | 30 ++++++++++--- src/snapshot.rs | 8 ++++ src/solver/cache.rs | 5 +++ src/solver/clause.rs | 100 +++++++++++++++++++++++++++++++++++++++--- src/solver/mod.rs | 2 +- 8 files changed, 244 insertions(+), 19 deletions(-) diff --git a/cpp/include/resolvo.h b/cpp/include/resolvo.h index 97d00f5..c824df4 100644 --- a/cpp/include/resolvo.h +++ b/cpp/include/resolvo.h @@ -24,6 +24,15 @@ inline Requirement requirement_union(VersionSetUnionId id) { return cbindgen_private::resolvo_requirement_union(id); } +/** + * Specifies a conditional requirement, where the requirement is only active when the condition is met. + * @param condition The version set that must be satisfied for the requirement to be active. + * @param requirement The version set that must be satisfied when the condition is met. + */ +inline Requirement requirement_conditional(VersionSetId condition, VersionSetId requirement) { + return cbindgen_private::resolvo_requirement_conditional(condition, requirement); +} + /** * Called to solve a package problem. * diff --git a/cpp/src/lib.rs b/cpp/src/lib.rs index 781e365..8267d9e 100644 --- a/cpp/src/lib.rs +++ b/cpp/src/lib.rs @@ -48,6 +48,11 @@ pub enum Requirement { /// cbindgen:derive-eq /// cbindgen:derive-neq Union(VersionSetUnionId), + /// Specifies a conditional requirement, where the requirement is only active when the condition is met. + /// First VersionSetId is the condition, second is the requirement. + /// cbindgen:derive-eq + /// cbindgen:derive-neq + ConditionalRequires(VersionSetId, VersionSetId), } impl From for crate::Requirement { @@ -55,6 +60,9 @@ impl From for crate::Requirement { match value { resolvo::Requirement::Single(id) => Requirement::Single(id.into()), resolvo::Requirement::Union(id) => Requirement::Union(id.into()), + resolvo::Requirement::ConditionalRequires(condition, requirement) => { + Requirement::ConditionalRequires(condition.into(), requirement.into()) + } } } } @@ -64,6 +72,9 @@ impl From for resolvo::Requirement { match value { Requirement::Single(id) => resolvo::Requirement::Single(id.into()), Requirement::Union(id) => resolvo::Requirement::Union(id.into()), + Requirement::ConditionalRequires(condition, requirement) => { + resolvo::Requirement::ConditionalRequires(condition.into(), requirement.into()) + } } } } @@ -539,6 +550,15 @@ pub extern "C" fn resolvo_requirement_union( Requirement::Union(version_set_union_id) } +#[no_mangle] +#[allow(unused)] +pub extern "C" fn resolvo_requirement_conditional( + condition: VersionSetId, + requirement: VersionSetId, +) -> Requirement { + Requirement::ConditionalRequires(condition, requirement) +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/conflict.rs b/src/conflict.rs index 3d121b6..48bc77a 100644 --- a/src/conflict.rs +++ b/src/conflict.rs @@ -11,14 +11,17 @@ use petgraph::{ Direction, }; -use crate::solver::variable_map::VariableOrigin; use crate::{ internal::{ arena::ArenaId, - id::{ClauseId, SolvableId, SolvableOrRootId, StringId, VersionSetId}, + id::{ClauseId, SolvableId, SolvableOrRootId, StringId, VariableId, VersionSetId}, }, runtime::AsyncRuntime, - solver::{clause::Clause, Solver}, + solver::{ + clause::Clause, + variable_map::{VariableMap, VariableOrigin}, + Solver, + }, DependencyProvider, Interner, Requirement, }; @@ -160,6 +163,49 @@ impl Conflict { ConflictEdge::Conflict(ConflictCause::Constrains(version_set_id)), ); } + &Clause::Conditional(package_id, condition, then_version_set_id) => { + let solvable = package_id + .as_solvable_or_root(&solver.variable_map) + .expect("only solvables can be excluded"); + let package_node = Self::add_node(&mut graph, &mut nodes, solvable); + + let candidates = solver.async_runtime.block_on(solver.cache.get_or_cache_sorted_candidates(then_version_set_id)).unwrap_or_else(|_| { + unreachable!("The version set was used in the solver, so it must have been cached. Therefore cancellation is impossible here and we cannot get an `Err(...)`") + }); + + if candidates.is_empty() { + tracing::trace!( + "{package_id:?} conditionally requires {then_version_set_id:?}, which has no candidates" + ); + graph.add_edge( + package_node, + unresolved_node, + ConflictEdge::ConditionalRequires(then_version_set_id, condition), + ); + } else { + for &candidate_id in candidates { + tracing::trace!("{package_id:?} conditionally requires {candidate_id:?}"); + + let candidate_node = + Self::add_node(&mut graph, &mut nodes, candidate_id.into()); + graph.add_edge( + package_node, + candidate_node, + ConflictEdge::ConditionalRequires(then_version_set_id, condition), + ); + } + } + + // Add an edge for the unsatisfied condition if it exists + if let Some(condition_solvable) = condition.as_solvable(&solver.variable_map) { + let condition_node = Self::add_node(&mut graph, &mut nodes, condition_solvable.into()); + graph.add_edge( + package_node, + condition_node, + ConflictEdge::Conflict(ConflictCause::UnsatisfiedCondition(condition)), + ); + } + } } } @@ -205,7 +251,7 @@ impl Conflict { solver: &'a Solver, ) -> DisplayUnsat<'a, D> { let graph = self.graph(solver); - DisplayUnsat::new(graph, solver.provider()) + DisplayUnsat::new(graph, solver.provider(), &solver.variable_map) } } @@ -239,13 +285,15 @@ impl ConflictNode { } /// An edge in the graph representation of a [`Conflict`] -#[derive(Copy, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)] +#[derive(Clone, Copy, Hash, Eq, PartialEq, Ord, PartialOrd)] pub(crate) enum ConflictEdge { /// The target node is a candidate for the dependency specified by the /// [`Requirement`] Requires(Requirement), /// The target node is involved in a conflict, caused by `ConflictCause` Conflict(ConflictCause), + /// The target node is a candidate for a conditional dependency + ConditionalRequires(Requirement, VariableId), } impl ConflictEdge { @@ -253,12 +301,14 @@ impl ConflictEdge { match self { ConflictEdge::Requires(match_spec_id) => Some(match_spec_id), ConflictEdge::Conflict(_) => None, + ConflictEdge::ConditionalRequires(match_spec_id, _) => Some(match_spec_id), } } fn requires(self) -> Requirement { match self { ConflictEdge::Requires(match_spec_id) => match_spec_id, + ConflictEdge::ConditionalRequires(match_spec_id, _) => match_spec_id, ConflictEdge::Conflict(_) => panic!("expected requires edge, found conflict"), } } @@ -275,6 +325,8 @@ pub(crate) enum ConflictCause { ForbidMultipleInstances, /// The node was excluded Excluded, + /// The condition for a conditional dependency was not satisfied + UnsatisfiedCondition(VariableId), } /// Represents a node that has been merged with others @@ -307,6 +359,7 @@ impl ConflictGraph { &self, f: &mut impl std::io::Write, interner: &impl Interner, + variable_map: &VariableMap, simplify: bool, ) -> Result<(), std::io::Error> { let graph = &self.graph; @@ -356,6 +409,16 @@ impl ConflictGraph { "already installed".to_string() } ConflictEdge::Conflict(ConflictCause::Excluded) => "excluded".to_string(), + ConflictEdge::Conflict(ConflictCause::UnsatisfiedCondition(condition)) => { + let condition_solvable = condition.as_solvable(variable_map) + .expect("condition must be a solvable"); + format!("unsatisfied condition: {}", condition_solvable.display(interner)) + } + ConflictEdge::ConditionalRequires(requirement, condition) => { + let condition_solvable = condition.as_solvable(variable_map) + .expect("condition must be a solvable"); + format!("if {} then {}", condition_solvable.display(interner), requirement.display(interner)) + } }; let target = match target { @@ -494,6 +557,7 @@ impl ConflictGraph { .edges_directed(nx, Direction::Outgoing) .map(|e| match e.weight() { ConflictEdge::Requires(version_set_id) => (version_set_id, e.target()), + ConflictEdge::ConditionalRequires(version_set_id, _) => (version_set_id, e.target()), ConflictEdge::Conflict(_) => unreachable!(), }) .chunk_by(|(&version_set_id, _)| version_set_id); @@ -540,6 +604,7 @@ impl ConflictGraph { .edges_directed(nx, Direction::Outgoing) .map(|e| match e.weight() { ConflictEdge::Requires(version_set_id) => (version_set_id, e.target()), + ConflictEdge::ConditionalRequires(version_set_id, _) => (version_set_id, e.target()), ConflictEdge::Conflict(_) => unreachable!(), }) .chunk_by(|(&version_set_id, _)| version_set_id); @@ -673,10 +738,11 @@ pub struct DisplayUnsat<'i, I: Interner> { installable_set: HashSet, missing_set: HashSet, interner: &'i I, + variable_map: &'i VariableMap, } impl<'i, I: Interner> DisplayUnsat<'i, I> { - pub(crate) fn new(graph: ConflictGraph, interner: &'i I) -> Self { + pub(crate) fn new(graph: ConflictGraph, interner: &'i I, variable_map: &'i VariableMap) -> Self { let merged_candidates = graph.simplify(interner); let installable_set = graph.get_installable_set(); let missing_set = graph.get_missing_set(); @@ -687,6 +753,7 @@ impl<'i, I: Interner> DisplayUnsat<'i, I> { installable_set, missing_set, interner, + variable_map, } } @@ -1020,6 +1087,7 @@ impl<'i, I: Interner> fmt::Display for DisplayUnsat<'i, I> { let conflict = match e.weight() { ConflictEdge::Requires(_) => continue, ConflictEdge::Conflict(conflict) => conflict, + ConflictEdge::ConditionalRequires(_, _) => continue, }; // The only possible conflict at the root level is a Locked conflict @@ -1045,6 +1113,15 @@ impl<'i, I: Interner> fmt::Display for DisplayUnsat<'i, I> { )?; } ConflictCause::Excluded => continue, + &ConflictCause::UnsatisfiedCondition(condition) => { + let condition_solvable = condition.as_solvable(self.variable_map) + .expect("condition must be a solvable"); + writeln!( + f, + "{indent}condition {} is not satisfied", + condition_solvable.display(self.interner), + )?; + } }; } } diff --git a/src/requirement.rs b/src/requirement.rs index 244ec48..c8e876f 100644 --- a/src/requirement.rs +++ b/src/requirement.rs @@ -13,6 +13,9 @@ pub enum Requirement { /// This variant is typically used for requirements that can be satisfied by two or more /// version sets belonging to _different_ packages. Union(VersionSetUnionId), + /// Specifies a conditional requirement, where the requirement is only active when the condition is met. + /// First VersionSetId is the condition, second is the requirement. + ConditionalRequires(VersionSetId, VersionSetId), } impl Default for Requirement { @@ -46,12 +49,15 @@ impl Requirement { &'i self, interner: &'i impl Interner, ) -> impl Iterator + 'i { - match *self { + match self { Requirement::Single(version_set) => { - itertools::Either::Left(std::iter::once(version_set)) + itertools::Either::Left(itertools::Either::Left(std::iter::once(*version_set))) } Requirement::Union(version_set_union) => { - itertools::Either::Right(interner.version_sets_in_union(version_set_union)) + itertools::Either::Left(itertools::Either::Right(interner.version_sets_in_union(*version_set_union))) + } + Requirement::ConditionalRequires(condition, requirement) => { + itertools::Either::Right(std::iter::once(*condition).chain(std::iter::once(*requirement))) } } } @@ -64,18 +70,18 @@ pub(crate) struct DisplayRequirement<'i, I: Interner> { impl<'i, I: Interner> Display for DisplayRequirement<'i, I> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match *self.requirement { + match self.requirement { Requirement::Single(version_set) => write!( f, "{} {}", self.interner - .display_name(self.interner.version_set_name(version_set)), - self.interner.display_version_set(version_set) + .display_name(self.interner.version_set_name(*version_set)), + self.interner.display_version_set(*version_set) ), Requirement::Union(version_set_union) => { let formatted_version_sets = self .interner - .version_sets_in_union(version_set_union) + .version_sets_in_union(*version_set_union) .format_with(" | ", |version_set, f| { f(&format_args!( "{} {}", @@ -87,6 +93,16 @@ impl<'i, I: Interner> Display for DisplayRequirement<'i, I> { write!(f, "{}", formatted_version_sets) } + Requirement::ConditionalRequires(condition, requirement) => { + write!( + f, + "if {} then {} {}", + self.interner.display_version_set(*condition), + self.interner + .display_name(self.interner.version_set_name(*requirement)), + self.interner.display_version_set(*requirement) + ) + } } } } diff --git a/src/snapshot.rs b/src/snapshot.rs index 0b8b6d2..0c4a986 100644 --- a/src/snapshot.rs +++ b/src/snapshot.rs @@ -243,6 +243,14 @@ impl DependencySnapshot { .version_set_unions .insert(version_set_union_id, version_sets); } + Requirement::ConditionalRequires(condition, requirement) => { + if seen.insert(Element::VersionSet(condition)) { + queue.push_back(Element::VersionSet(condition)); + } + if seen.insert(Element::VersionSet(requirement)) { + queue.push_back(Element::VersionSet(requirement)); + } + } } } } diff --git a/src/solver/cache.rs b/src/solver/cache.rs index cf6c6cc..00c8c92 100644 --- a/src/solver/cache.rs +++ b/src/solver/cache.rs @@ -280,6 +280,11 @@ impl SolverCache { } } } + Requirement::ConditionalRequires(condition, requirement) => { + let candidates = self.get_or_cache_sorted_candidates_for_version_set(condition).await?; + let sorted_candidates = self.get_or_cache_sorted_candidates_for_version_set(requirement).await?; + Ok(sorted_candidates) + } } } diff --git a/src/solver/clause.rs b/src/solver/clause.rs index f034130..4e41c0e 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -11,12 +11,10 @@ use crate::{ internal::{ arena::{Arena, ArenaId}, id::{ClauseId, LearntClauseId, StringId, VersionSetId}, - }, - solver::{ + }, solver::{ decision_map::DecisionMap, decision_tracker::DecisionTracker, variable_map::VariableMap, VariableId, - }, - Interner, NameId, Requirement, + }, DependencyProvider, Interner, NameId, Requirement }; /// Represents a single clause in the SAT problem @@ -46,7 +44,7 @@ use crate::{ /// limited set of clauses. There are thousands of clauses for a particular /// dependency resolution problem, and we try to keep the [`Clause`] enum small. /// A naive implementation would store a `Vec`. -#[derive(Copy, Clone, Debug)] +#[derive(Clone, Copy, Debug)] pub(crate) enum Clause { /// An assertion that the root solvable must be installed /// @@ -77,6 +75,10 @@ pub(crate) enum Clause { /// /// In SAT terms: (¬A ∨ ¬B) Constrains(VariableId, VariableId, VersionSetId), + /// In SAT terms: (¬A ∨ ¬C ∨ B1 ∨ B2 ∨ ... ∨ B99), where A is the solvable, + /// C is the condition, and B1 to B99 represent the possible candidates for + /// the provided [`Requirement`]. + Conditional(VariableId, VariableId, Requirement), /// Forbids the package on the right-hand side /// /// Note that the package on the left-hand side is not part of the clause, @@ -230,6 +232,45 @@ impl Clause { ) } + fn conditional_impl( + package_id: VariableId, + condition: VariableId, + then: Requirement, + candidates: impl IntoIterator, + decision_tracker: &DecisionTracker, + ) -> (Self, Option<[Literal; 2]>, bool) { + // It only makes sense to introduce a conditional clause when the package is undecided or going to be installed + assert_ne!(decision_tracker.assigned_value(package_id), Some(false)); + assert_ne!(decision_tracker.assigned_value(condition), Some(false)); + + let kind = Clause::Conditional(package_id, condition, then); + let mut candidates = candidates.into_iter().peekable(); + let first_candidate = candidates.peek().copied(); + if let Some(first_candidate) = first_candidate { + match candidates.find(|&c| decision_tracker.assigned_value(c) != Some(false)) { + // Watch any candidate that is not assigned to false + Some(watched_candidate) => ( + kind, + Some([package_id.negative(), watched_candidate.positive()]), + false, + ), + + // All candidates are assigned to false! Therefore, the clause conflicts with the + // current decisions. There are no valid watches for it at the moment, but we will + // assign default ones nevertheless, because they will become valid after the solver + // restarts. + None => ( + kind, + Some([package_id.negative(), first_candidate.positive()]), + true, + ), + } + } else { + // If there are no candidates there is no need to watch anything. + (kind, None, false) + } + } + /// Tries to fold over all the literals in the clause. /// /// This function is useful to iterate, find, or filter the literals in a @@ -272,6 +313,17 @@ impl Clause { Clause::Lock(_, s) => [s.negative(), VariableId::root().negative()] .into_iter() .try_fold(init, visit), + Clause::Conditional(package_id, condition, then) => { + [package_id.negative(), condition.negative()] + .into_iter() + .chain( + requirements_to_sorted_candidates[&then] + .iter() + .flatten() + .map(|&s| s.positive()), + ) + .try_fold(init, visit) + } } } @@ -419,6 +471,33 @@ impl WatchedLiterals { (Self::from_kind_and_initial_watches(watched_literals), kind) } + /// Shorthand method to construct a [Clause::Conditional] without requiring + /// complicated arguments. + /// + /// The returned boolean value is true when adding the clause resulted in a + /// conflict. + pub fn conditional( + package_id: VariableId, + condition: VariableId, + then: Requirement, + candidates: impl IntoIterator, + decision_tracker: &DecisionTracker, + ) -> (Option, bool, Clause) { + let (kind, watched_literals, conflict) = Clause::conditional_impl( + package_id, + condition, + then, + candidates, + decision_tracker, + ); + + ( + WatchedLiterals::from_kind_and_initial_watches(watched_literals), + conflict, + kind, + ) + } + fn from_kind_and_initial_watches(watched_literals: Option<[Literal; 2]>) -> Option { let watched_literals = watched_literals?; debug_assert!(watched_literals[0] != watched_literals[1]); @@ -611,6 +690,17 @@ impl<'i, I: Interner> Display for ClauseDisplay<'i, I> { other, ) } + Clause::Conditional(package_id, condition, then) => { + write!( + f, + "Conditional({}({:?}), {}({:?}), {})", + package_id.display(self.variable_map, self.interner), + package_id, + condition.display(self.variable_map, self.interner), + condition, + then.display(self.interner) + ) + } } } } diff --git a/src/solver/mod.rs b/src/solver/mod.rs index 8c0e026..cf3adf0 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -695,7 +695,7 @@ impl Solver { fn resolve_dependencies(&mut self, mut level: u32) -> Result { loop { // Make a decision. If no decision could be made it means the problem is - // satisfyable. + // satisfiable. let Some((candidate, required_by, clause_id)) = self.decide() else { break; }; From 0383bfc5a4efa7b1ff4bca49fe6e536d18edcaad Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 21 Jan 2025 11:19:10 -0500 Subject: [PATCH 02/41] Add the watched literals for conditional requirements --- src/conflict.rs | 75 ++++++++++--------- src/requirement.rs | 35 +++++---- src/solver/cache.rs | 8 +- src/solver/clause.rs | 114 +++++++++++++++-------------- src/solver/mod.rs | 171 +++++++++++++++++++++++++++++++++++++------ 5 files changed, 277 insertions(+), 126 deletions(-) diff --git a/src/conflict.rs b/src/conflict.rs index 48bc77a..e17ec73 100644 --- a/src/conflict.rs +++ b/src/conflict.rs @@ -163,7 +163,7 @@ impl Conflict { ConflictEdge::Conflict(ConflictCause::Constrains(version_set_id)), ); } - &Clause::Conditional(package_id, condition, then_version_set_id) => { + &Clause::Conditional(package_id, condition) => { let solvable = package_id .as_solvable_or_root(&solver.variable_map) .expect("only solvables can be excluded"); @@ -172,33 +172,36 @@ impl Conflict { let candidates = solver.async_runtime.block_on(solver.cache.get_or_cache_sorted_candidates(then_version_set_id)).unwrap_or_else(|_| { unreachable!("The version set was used in the solver, so it must have been cached. Therefore cancellation is impossible here and we cannot get an `Err(...)`") }); - + if candidates.is_empty() { tracing::trace!( - "{package_id:?} conditionally requires {then_version_set_id:?}, which has no candidates" + "{package_id:?} conditionally requires {condition:?}, which has no candidates" ); graph.add_edge( package_node, unresolved_node, - ConflictEdge::ConditionalRequires(then_version_set_id, condition), + ConflictEdge::ConditionalRequires(condition), ); } else { for &candidate_id in candidates { - tracing::trace!("{package_id:?} conditionally requires {candidate_id:?}"); + tracing::trace!( + "{package_id:?} conditionally requires {candidate_id:?}" + ); let candidate_node = Self::add_node(&mut graph, &mut nodes, candidate_id.into()); graph.add_edge( package_node, candidate_node, - ConflictEdge::ConditionalRequires(then_version_set_id, condition), + ConflictEdge::ConditionalRequires(condition), ); } } - + // Add an edge for the unsatisfied condition if it exists if let Some(condition_solvable) = condition.as_solvable(&solver.variable_map) { - let condition_node = Self::add_node(&mut graph, &mut nodes, condition_solvable.into()); + let condition_node = + Self::add_node(&mut graph, &mut nodes, condition_solvable.into()); graph.add_edge( package_node, condition_node, @@ -251,7 +254,7 @@ impl Conflict { solver: &'a Solver, ) -> DisplayUnsat<'a, D> { let graph = self.graph(solver); - DisplayUnsat::new(graph, solver.provider(), &solver.variable_map) + DisplayUnsat::new(graph, solver.provider()) } } @@ -293,22 +296,22 @@ pub(crate) enum ConflictEdge { /// The target node is involved in a conflict, caused by `ConflictCause` Conflict(ConflictCause), /// The target node is a candidate for a conditional dependency - ConditionalRequires(Requirement, VariableId), + ConditionalRequires(Requirement), } impl ConflictEdge { fn try_requires(self) -> Option { match self { - ConflictEdge::Requires(match_spec_id) => Some(match_spec_id), + ConflictEdge::Requires(match_spec_id) + | ConflictEdge::ConditionalRequires(match_spec_id) => Some(match_spec_id), ConflictEdge::Conflict(_) => None, - ConflictEdge::ConditionalRequires(match_spec_id, _) => Some(match_spec_id), } } fn requires(self) -> Requirement { match self { - ConflictEdge::Requires(match_spec_id) => match_spec_id, - ConflictEdge::ConditionalRequires(match_spec_id, _) => match_spec_id, + ConflictEdge::Requires(match_spec_id) + | ConflictEdge::ConditionalRequires(match_spec_id) => match_spec_id, ConflictEdge::Conflict(_) => panic!("expected requires edge, found conflict"), } } @@ -326,7 +329,7 @@ pub(crate) enum ConflictCause { /// The node was excluded Excluded, /// The condition for a conditional dependency was not satisfied - UnsatisfiedCondition(VariableId), + UnsatisfiedCondition(Requirement), } /// Represents a node that has been merged with others @@ -359,7 +362,6 @@ impl ConflictGraph { &self, f: &mut impl std::io::Write, interner: &impl Interner, - variable_map: &VariableMap, simplify: bool, ) -> Result<(), std::io::Error> { let graph = &self.graph; @@ -410,14 +412,16 @@ impl ConflictGraph { } ConflictEdge::Conflict(ConflictCause::Excluded) => "excluded".to_string(), ConflictEdge::Conflict(ConflictCause::UnsatisfiedCondition(condition)) => { - let condition_solvable = condition.as_solvable(variable_map) - .expect("condition must be a solvable"); - format!("unsatisfied condition: {}", condition_solvable.display(interner)) + // let condition_solvable = condition.as_solvable(&solver.variable_map) + // .expect("condition must be a solvable"); + // format!("unsatisfied condition: {}", condition_solvable.display(interner)) + todo!() } ConflictEdge::ConditionalRequires(requirement, condition) => { - let condition_solvable = condition.as_solvable(variable_map) - .expect("condition must be a solvable"); - format!("if {} then {}", condition_solvable.display(interner), requirement.display(interner)) + // let condition_solvable = condition.as_solvable(&solver.variable_map) + // .expect("condition must be a solvable"); + // format!("if {} then {}", condition_solvable.display(interner), requirement.display(interner)) + todo!() } }; @@ -557,7 +561,9 @@ impl ConflictGraph { .edges_directed(nx, Direction::Outgoing) .map(|e| match e.weight() { ConflictEdge::Requires(version_set_id) => (version_set_id, e.target()), - ConflictEdge::ConditionalRequires(version_set_id, _) => (version_set_id, e.target()), + ConflictEdge::ConditionalRequires(version_set_id, _) => { + (version_set_id, e.target()) + } ConflictEdge::Conflict(_) => unreachable!(), }) .chunk_by(|(&version_set_id, _)| version_set_id); @@ -604,7 +610,9 @@ impl ConflictGraph { .edges_directed(nx, Direction::Outgoing) .map(|e| match e.weight() { ConflictEdge::Requires(version_set_id) => (version_set_id, e.target()), - ConflictEdge::ConditionalRequires(version_set_id, _) => (version_set_id, e.target()), + ConflictEdge::ConditionalRequires(version_set_id, _) => { + (version_set_id, e.target()) + } ConflictEdge::Conflict(_) => unreachable!(), }) .chunk_by(|(&version_set_id, _)| version_set_id); @@ -738,11 +746,10 @@ pub struct DisplayUnsat<'i, I: Interner> { installable_set: HashSet, missing_set: HashSet, interner: &'i I, - variable_map: &'i VariableMap, } impl<'i, I: Interner> DisplayUnsat<'i, I> { - pub(crate) fn new(graph: ConflictGraph, interner: &'i I, variable_map: &'i VariableMap) -> Self { + pub(crate) fn new(graph: ConflictGraph, interner: &'i I) -> Self { let merged_candidates = graph.simplify(interner); let installable_set = graph.get_installable_set(); let missing_set = graph.get_missing_set(); @@ -753,7 +760,6 @@ impl<'i, I: Interner> DisplayUnsat<'i, I> { installable_set, missing_set, interner, - variable_map, } } @@ -1114,13 +1120,14 @@ impl<'i, I: Interner> fmt::Display for DisplayUnsat<'i, I> { } ConflictCause::Excluded => continue, &ConflictCause::UnsatisfiedCondition(condition) => { - let condition_solvable = condition.as_solvable(self.variable_map) - .expect("condition must be a solvable"); - writeln!( - f, - "{indent}condition {} is not satisfied", - condition_solvable.display(self.interner), - )?; + // let condition_solvable = condition.as_solvable(self.variable_map) + // .expect("condition must be a solvable"); + // writeln!( + // f, + // "{indent}condition {} is not satisfied", + // condition_solvable.display(self.interner), + // )?; + todo!() } }; } diff --git a/src/requirement.rs b/src/requirement.rs index c8e876f..62589f3 100644 --- a/src/requirement.rs +++ b/src/requirement.rs @@ -48,17 +48,22 @@ impl Requirement { pub(crate) fn version_sets<'i>( &'i self, interner: &'i impl Interner, - ) -> impl Iterator + 'i { + ) -> ( + impl Iterator + 'i, + Option, + ) { match self { Requirement::Single(version_set) => { - itertools::Either::Left(itertools::Either::Left(std::iter::once(*version_set))) - } - Requirement::Union(version_set_union) => { - itertools::Either::Left(itertools::Either::Right(interner.version_sets_in_union(*version_set_union))) - } - Requirement::ConditionalRequires(condition, requirement) => { - itertools::Either::Right(std::iter::once(*condition).chain(std::iter::once(*requirement))) + (itertools::Either::Left(std::iter::once(*version_set)), None) } + Requirement::Union(version_set_union) => ( + itertools::Either::Right(interner.version_sets_in_union(*version_set_union)), + None, + ), + Requirement::ConditionalRequires(condition, requirement) => ( + itertools::Either::Left(std::iter::once(*requirement)), + Some(*condition), + ), } } } @@ -70,18 +75,18 @@ pub(crate) struct DisplayRequirement<'i, I: Interner> { impl<'i, I: Interner> Display for DisplayRequirement<'i, I> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self.requirement { + match *self.requirement { Requirement::Single(version_set) => write!( f, "{} {}", self.interner - .display_name(self.interner.version_set_name(*version_set)), - self.interner.display_version_set(*version_set) + .display_name(self.interner.version_set_name(version_set)), + self.interner.display_version_set(version_set) ), Requirement::Union(version_set_union) => { let formatted_version_sets = self .interner - .version_sets_in_union(*version_set_union) + .version_sets_in_union(version_set_union) .format_with(" | ", |version_set, f| { f(&format_args!( "{} {}", @@ -97,10 +102,10 @@ impl<'i, I: Interner> Display for DisplayRequirement<'i, I> { write!( f, "if {} then {} {}", - self.interner.display_version_set(*condition), + self.interner.display_version_set(condition), self.interner - .display_name(self.interner.version_set_name(*requirement)), - self.interner.display_version_set(*requirement) + .display_name(self.interner.version_set_name(requirement)), + self.interner.display_version_set(requirement) ) } } diff --git a/src/solver/cache.rs b/src/solver/cache.rs index 00c8c92..952c8cb 100644 --- a/src/solver/cache.rs +++ b/src/solver/cache.rs @@ -281,8 +281,12 @@ impl SolverCache { } } Requirement::ConditionalRequires(condition, requirement) => { - let candidates = self.get_or_cache_sorted_candidates_for_version_set(condition).await?; - let sorted_candidates = self.get_or_cache_sorted_candidates_for_version_set(requirement).await?; + let candidates = self + .get_or_cache_sorted_candidates_for_version_set(condition) + .await?; + let sorted_candidates = self + .get_or_cache_sorted_candidates_for_version_set(requirement) + .await?; Ok(sorted_candidates) } } diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 4e41c0e..15f9d44 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -11,10 +11,12 @@ use crate::{ internal::{ arena::{Arena, ArenaId}, id::{ClauseId, LearntClauseId, StringId, VersionSetId}, - }, solver::{ + }, + solver::{ decision_map::DecisionMap, decision_tracker::DecisionTracker, variable_map::VariableMap, VariableId, - }, DependencyProvider, Interner, NameId, Requirement + }, + Interner, NameId, Requirement, }; /// Represents a single clause in the SAT problem @@ -78,7 +80,7 @@ pub(crate) enum Clause { /// In SAT terms: (¬A ∨ ¬C ∨ B1 ∨ B2 ∨ ... ∨ B99), where A is the solvable, /// C is the condition, and B1 to B99 represent the possible candidates for /// the provided [`Requirement`]. - Conditional(VariableId, VariableId, Requirement), + Conditional(VariableId, Requirement), /// Forbids the package on the right-hand side /// /// Note that the package on the left-hand side is not part of the clause, @@ -232,42 +234,53 @@ impl Clause { ) } - fn conditional_impl( - package_id: VariableId, - condition: VariableId, - then: Requirement, - candidates: impl IntoIterator, + fn conditional( + parent_id: VariableId, + requirement: Requirement, decision_tracker: &DecisionTracker, + requirement_candidates: impl IntoIterator, + condition_candidates: impl IntoIterator, ) -> (Self, Option<[Literal; 2]>, bool) { - // It only makes sense to introduce a conditional clause when the package is undecided or going to be installed - assert_ne!(decision_tracker.assigned_value(package_id), Some(false)); - assert_ne!(decision_tracker.assigned_value(condition), Some(false)); + assert_ne!(decision_tracker.assigned_value(parent_id), Some(false)); - let kind = Clause::Conditional(package_id, condition, then); - let mut candidates = candidates.into_iter().peekable(); + let mut candidates = condition_candidates.into_iter().peekable(); let first_candidate = candidates.peek().copied(); if let Some(first_candidate) = first_candidate { - match candidates.find(|&c| decision_tracker.assigned_value(c) != Some(false)) { - // Watch any candidate that is not assigned to false - Some(watched_candidate) => ( - kind, - Some([package_id.negative(), watched_candidate.positive()]), - false, - ), - - // All candidates are assigned to false! Therefore, the clause conflicts with the - // current decisions. There are no valid watches for it at the moment, but we will - // assign default ones nevertheless, because they will become valid after the solver - // restarts. - None => ( - kind, - Some([package_id.negative(), first_candidate.positive()]), - true, + match condition_candidates + .into_iter() + .find(|&condition_id| decision_tracker.assigned_value(condition_id) == Some(true)) + { + Some(_) => Clause::requires( + parent_id, + requirement, + requirement_candidates, + decision_tracker, ), + None => { + if let Some(first_unset_candidate) = candidates.find(|&condition_id| { + decision_tracker.assigned_value(condition_id) != Some(false) + }) { + ( + Clause::Conditional(parent_id, requirement), + Some([parent_id.negative(), first_unset_candidate.negative()]), + false, + ) + } else { + // All condition candidates are assigned to false! Therefore, the clause conflicts with the + // current decisions. There are no valid watches for it at the moment, but we will + // assign default ones nevertheless, because they will become valid after the solver + // restarts. + ( + Clause::Conditional(parent_id, requirement), + Some([parent_id.negative(), first_candidate.negative()]), + true, + ) + } + } } } else { - // If there are no candidates there is no need to watch anything. - (kind, None, false) + // No condition candidates, so no need to watch anything + (Clause::Conditional(parent_id, requirement), None, false) } } @@ -313,17 +326,14 @@ impl Clause { Clause::Lock(_, s) => [s.negative(), VariableId::root().negative()] .into_iter() .try_fold(init, visit), - Clause::Conditional(package_id, condition, then) => { - [package_id.negative(), condition.negative()] - .into_iter() - .chain( - requirements_to_sorted_candidates[&then] - .iter() - .flatten() - .map(|&s| s.positive()), - ) - .try_fold(init, visit) - } + Clause::Conditional(package_id, condition) => iter::once(package_id.negative()) + .chain( + requirements_to_sorted_candidates[&condition] + .iter() + .flatten() + .map(|&s| s.positive()), + ) + .try_fold(init, visit), } } @@ -478,17 +488,17 @@ impl WatchedLiterals { /// conflict. pub fn conditional( package_id: VariableId, - condition: VariableId, - then: Requirement, - candidates: impl IntoIterator, + condition: Requirement, decision_tracker: &DecisionTracker, + requirement_candidates: impl IntoIterator, + condition_candidates: impl IntoIterator, ) -> (Option, bool, Clause) { - let (kind, watched_literals, conflict) = Clause::conditional_impl( + let (kind, watched_literals, conflict) = Clause::conditional( package_id, condition, - then, - candidates, decision_tracker, + requirement_candidates, + condition_candidates, ); ( @@ -690,15 +700,13 @@ impl<'i, I: Interner> Display for ClauseDisplay<'i, I> { other, ) } - Clause::Conditional(package_id, condition, then) => { + Clause::Conditional(package_id, condition) => { write!( f, - "Conditional({}({:?}), {}({:?}), {})", + "Conditional({}({:?}), {})", package_id.display(self.variable_map, self.interner), package_id, - condition.display(self.variable_map, self.interner), - condition, - then.display(self.interner) + condition.display(self.interner), ) } } diff --git a/src/solver/mod.rs b/src/solver/mod.rs index cf3adf0..5dfea63 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -788,6 +788,8 @@ impl Solver { // Get the candidates for the individual version sets. let version_set_candidates = &self.requirement_to_sorted_candidates[deps]; + let (version_sets, condition) = deps.version_sets(self.provider()); + // Iterate over all version sets in the requirement and find the first version // set that we can act on, or if a single candidate (from any version set) makes // the clause true. @@ -795,10 +797,7 @@ impl Solver { // NOTE: We zip the version sets from the requirements and the variables that we // previously cached. This assumes that the order of the version sets is the // same in both collections. - for (version_set, candidates) in deps - .version_sets(self.provider()) - .zip(version_set_candidates) - { + for (version_set, candidates) in version_sets.zip(version_set_candidates) { // Find the first candidate that is not yet assigned a value or find the first // value that makes this clause true. candidate = candidates.iter().try_fold( @@ -1536,6 +1535,12 @@ async fn add_clauses_for_solvables( requirement: Requirement, candidates: Vec<&'i [SolvableId]>, }, + ConditionalSortedCandidates { + solvable_id: SolvableOrRootId, + requirement: Requirement, + requirement_candidates: Vec<&'i [SolvableId]>, + condition_candidates: &'i [SolvableId], + }, NonMatchingCandidates { solvable_id: SolvableOrRootId, version_set_id: VersionSetId, @@ -1637,10 +1642,13 @@ async fn add_clauses_for_solvables( } }; - for version_set_id in requirements + for (version_set_id, condition) in requirements .iter() - .flat_map(|requirement| requirement.version_sets(cache.provider())) - .chain(constrains.iter().copied()) + .flat_map(|requirement| { + let (version_sets, condition) = requirement.version_sets(cache.provider()); + version_sets.map(move |vs| (vs, condition)) + }) + .chain(constrains.iter().map(|&vs| (vs, None))) { let dependency_name = cache.provider().version_set_name(version_set_id); if clauses_added_for_package.insert(dependency_name) { @@ -1661,28 +1669,56 @@ async fn add_clauses_for_solvables( .boxed_local(), ); } + + if let Some(condition) = condition { + let condition_name = cache.provider().version_set_name(condition); + if clauses_added_for_package.insert(condition_name) { + pending_futures.push( + async move { + let condition_candidates = + cache.get_or_cache_candidates(condition_name).await?; + Ok(TaskResult::Candidates { + name_id: condition_name, + package_candidates: &condition_candidates, + }) + } + .boxed_local(), + ); + } + } } for requirement in requirements { // Find all the solvable that match for the given version set pending_futures.push( async move { - let candidates = futures::future::try_join_all( - requirement - .version_sets(cache.provider()) - .map(|version_set| { - cache.get_or_cache_sorted_candidates_for_version_set( - version_set, - ) - }), - ) - .await?; + let (version_sets, condition) = + requirement.version_sets(cache.provider()); + let candidates = + futures::future::try_join_all(version_sets.map(|version_set| { + cache + .get_or_cache_sorted_candidates_for_version_set(version_set) + })) + .await?; - Ok(TaskResult::SortedCandidates { - solvable_id, - requirement, - candidates, - }) + if let Some(condition) = condition { + let condition_candidates = cache + .get_or_cache_sorted_candidates_for_version_set(condition) + .await?; + + Ok(TaskResult::ConditionalSortedCandidates { + solvable_id, + requirement, + requirement_candidates: candidates, + condition_candidates, + }) + } else { + Ok(TaskResult::SortedCandidates { + solvable_id, + requirement, + candidates, + }) + } } .boxed_local(), ); @@ -1846,6 +1882,97 @@ async fn add_clauses_for_solvables( output.negative_assertions.push((variable, clause_id)); } } + TaskResult::ConditionalSortedCandidates { + solvable_id, + requirement, + requirement_candidates, + condition_candidates, + } => { + tracing::trace!( + "conditional candidates available for {}", + solvable_id.display(cache.provider()), + ); + + // Allocate a variable for the solvable + let variable = match solvable_id.solvable() { + Some(solvable_id) => variable_map.intern_solvable(solvable_id), + None => variable_map.root(), + }; + + // Cache the candidates for this requirement + let version_set_variables: Vec<_> = requirement_candidates + .iter() + .map(|candidates| { + candidates + .iter() + .map(|&candidate| variable_map.intern_solvable(candidate)) + .collect::>() + }) + .collect(); + + requirement_to_sorted_candidates + .insert(requirement.clone(), version_set_variables.clone()); + + // Add forbidden clauses for the candidates + for candidates in requirement_candidates.iter() { + for &candidate in *candidates { + let candidate_var = variable_map.intern_solvable(candidate); + let name_id = cache.provider().solvable_name(candidate); + let other_solvables = forbidden_clauses_added.entry(name_id).or_default(); + other_solvables.add( + candidate_var, + |a, b, positive| { + let (watched_literals, kind) = WatchedLiterals::forbid_multiple( + a, + if positive { b.positive() } else { b.negative() }, + name_id, + ); + let clause_id = clauses.alloc(watched_literals, kind); + debug_assert!( + clauses.watched_literals[clause_id.to_usize()].is_some() + ); + output.clauses_to_watch.push(clause_id); + }, + || variable_map.alloc_forbid_multiple_variable(name_id), + ); + } + } + + // Add the requirements clause + let no_candidates = requirement_candidates + .iter() + .all(|candidates| candidates.is_empty()); + let condition_variables = condition_candidates + .iter() + .map(|&candidate| variable_map.intern_solvable(candidate)) + .collect::>(); + + let (watched_literals, conflict, kind) = WatchedLiterals::conditional( + variable, + requirement, + decision_tracker, + version_set_variables.iter().flatten().copied(), + condition_variables.iter().copied(), + ); + + let has_watches = watched_literals.is_some(); + let clause_id = clauses.alloc(watched_literals, kind); + + if has_watches { + output.clauses_to_watch.push(clause_id); + } + + output + .new_requires_clauses + .push((variable, requirement, clause_id)); + + if conflict { + output.conflicting_clauses.push(clause_id); + } else if no_candidates { + // Add assertions for unit clauses (i.e. those with no matching candidates) + output.negative_assertions.push((variable, clause_id)); + } + } TaskResult::NonMatchingCandidates { solvable_id, version_set_id, From 2e6d57b9823ec2d974105eb8f8ce989bc3a37441 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 21 Jan 2025 16:24:02 -0500 Subject: [PATCH 03/41] move conditional requirements to a separate struct --- cpp/src/lib.rs | 57 ++++---- src/conflict.rs | 63 ++++----- src/lib.rs | 4 +- src/requirement.rs | 84 ++++++++---- src/snapshot.rs | 12 +- src/solver/cache.rs | 9 -- src/solver/clause.rs | 120 ++++++++++------- src/solver/mod.rs | 219 +++++++++++-------------------- tools/solve-snapshot/src/main.rs | 3 +- 9 files changed, 279 insertions(+), 292 deletions(-) diff --git a/cpp/src/lib.rs b/cpp/src/lib.rs index 8267d9e..04d05c0 100644 --- a/cpp/src/lib.rs +++ b/cpp/src/lib.rs @@ -31,6 +31,17 @@ impl From for resolvo::SolvableId { } } +/// Specifies a conditional requirement, where the requirement is only active when the condition is met. +/// First VersionSetId is the condition, second is the requirement. +/// cbindgen:derive-eq +/// cbindgen:derive-neq +#[repr(C)] +#[derive(Copy, Clone)] +pub struct ConditionalRequirement { + pub condition: Option, + pub requirement: Requirement, +} + /// Specifies the dependency of a solvable on a set of version sets. /// cbindgen:derive-eq /// cbindgen:derive-neq @@ -48,11 +59,6 @@ pub enum Requirement { /// cbindgen:derive-eq /// cbindgen:derive-neq Union(VersionSetUnionId), - /// Specifies a conditional requirement, where the requirement is only active when the condition is met. - /// First VersionSetId is the condition, second is the requirement. - /// cbindgen:derive-eq - /// cbindgen:derive-neq - ConditionalRequires(VersionSetId, VersionSetId), } impl From for crate::Requirement { @@ -60,9 +66,6 @@ impl From for crate::Requirement { match value { resolvo::Requirement::Single(id) => Requirement::Single(id.into()), resolvo::Requirement::Union(id) => Requirement::Union(id.into()), - resolvo::Requirement::ConditionalRequires(condition, requirement) => { - Requirement::ConditionalRequires(condition.into(), requirement.into()) - } } } } @@ -72,9 +75,24 @@ impl From for resolvo::Requirement { match value { Requirement::Single(id) => resolvo::Requirement::Single(id.into()), Requirement::Union(id) => resolvo::Requirement::Union(id.into()), - Requirement::ConditionalRequires(condition, requirement) => { - resolvo::Requirement::ConditionalRequires(condition.into(), requirement.into()) - } + } + } +} + +impl From for ConditionalRequirement { + fn from(value: resolvo::ConditionalRequirement) -> Self { + Self { + condition: value.condition.map(|id| id.into()), + requirement: value.requirement.into(), + } + } +} + +impl From for resolvo::ConditionalRequirement { + fn from(value: ConditionalRequirement) -> Self { + Self { + condition: value.condition.map(|id| id.into()), + requirement: value.requirement.into(), } } } @@ -173,7 +191,7 @@ pub struct Dependencies { /// A pointer to the first element of a list of requirements. Requirements /// defines which packages should be installed alongside the depending /// package and the constraints applied to the package. - pub requirements: Vector, + pub conditional_requirements: Vector, /// Defines additional constraints on packages that may or may not be part /// of the solution. Different from `requirements`, packages in this set @@ -470,8 +488,8 @@ impl<'d> resolvo::DependencyProvider for &'d DependencyProvider { }; resolvo::Dependencies::Known(KnownDependencies { - requirements: dependencies - .requirements + conditional_requirements: dependencies + .conditional_requirements .into_iter() .map(Into::into) .collect(), @@ -486,7 +504,7 @@ impl<'d> resolvo::DependencyProvider for &'d DependencyProvider { #[repr(C)] pub struct Problem<'a> { - pub requirements: Slice<'a, Requirement>, + pub requirements: Slice<'a, ConditionalRequirement>, pub constraints: Slice<'a, VersionSetId>, pub soft_requirements: Slice<'a, SolvableId>, } @@ -550,15 +568,6 @@ pub extern "C" fn resolvo_requirement_union( Requirement::Union(version_set_union_id) } -#[no_mangle] -#[allow(unused)] -pub extern "C" fn resolvo_requirement_conditional( - condition: VersionSetId, - requirement: VersionSetId, -) -> Requirement { - Requirement::ConditionalRequires(condition, requirement) -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/conflict.rs b/src/conflict.rs index e17ec73..6c1ace8 100644 --- a/src/conflict.rs +++ b/src/conflict.rs @@ -14,14 +14,10 @@ use petgraph::{ use crate::{ internal::{ arena::ArenaId, - id::{ClauseId, SolvableId, SolvableOrRootId, StringId, VariableId, VersionSetId}, + id::{ClauseId, SolvableId, SolvableOrRootId, StringId, VersionSetId}, }, runtime::AsyncRuntime, - solver::{ - clause::Clause, - variable_map::{VariableMap, VariableOrigin}, - Solver, - }, + solver::{clause::Clause, variable_map::VariableOrigin, Solver}, DependencyProvider, Interner, Requirement, }; @@ -163,24 +159,24 @@ impl Conflict { ConflictEdge::Conflict(ConflictCause::Constrains(version_set_id)), ); } - &Clause::Conditional(package_id, condition) => { + &Clause::Conditional(package_id, condition, version_set_id) => { let solvable = package_id .as_solvable_or_root(&solver.variable_map) .expect("only solvables can be excluded"); let package_node = Self::add_node(&mut graph, &mut nodes, solvable); - let candidates = solver.async_runtime.block_on(solver.cache.get_or_cache_sorted_candidates(then_version_set_id)).unwrap_or_else(|_| { + let candidates = solver.async_runtime.block_on(solver.cache.get_or_cache_sorted_candidates(version_set_id)).unwrap_or_else(|_| { unreachable!("The version set was used in the solver, so it must have been cached. Therefore cancellation is impossible here and we cannot get an `Err(...)`") }); if candidates.is_empty() { tracing::trace!( - "{package_id:?} conditionally requires {condition:?}, which has no candidates" + "{package_id:?} conditionally requires {version_set_id:?}, which has no candidates" ); graph.add_edge( package_node, unresolved_node, - ConflictEdge::ConditionalRequires(condition), + ConflictEdge::ConditionalRequires(condition, version_set_id), ); } else { for &candidate_id in candidates { @@ -193,21 +189,22 @@ impl Conflict { graph.add_edge( package_node, candidate_node, - ConflictEdge::ConditionalRequires(condition), + ConflictEdge::ConditionalRequires(condition, version_set_id), ); } } - // Add an edge for the unsatisfied condition if it exists - if let Some(condition_solvable) = condition.as_solvable(&solver.variable_map) { - let condition_node = - Self::add_node(&mut graph, &mut nodes, condition_solvable.into()); - graph.add_edge( - package_node, - condition_node, - ConflictEdge::Conflict(ConflictCause::UnsatisfiedCondition(condition)), - ); - } + // TODO: Add an edge for the unsatisfied condition if it exists + // // Add an edge for the unsatisfied condition if it exists + // if let Some(condition_solvable) = condition.as_solvable(&solver.variable_map) { + // let condition_node = + // Self::add_node(&mut graph, &mut nodes, condition_solvable.into()); + // graph.add_edge( + // package_node, + // condition_node, + // ConflictEdge::Conflict(ConflictCause::UnsatisfiedCondition(condition.into())), + // ); + // } } } } @@ -296,22 +293,24 @@ pub(crate) enum ConflictEdge { /// The target node is involved in a conflict, caused by `ConflictCause` Conflict(ConflictCause), /// The target node is a candidate for a conditional dependency - ConditionalRequires(Requirement), + ConditionalRequires(VersionSetId, Requirement), } impl ConflictEdge { fn try_requires(self) -> Option { match self { - ConflictEdge::Requires(match_spec_id) - | ConflictEdge::ConditionalRequires(match_spec_id) => Some(match_spec_id), + ConflictEdge::Requires(match_spec_id) => Some(match_spec_id), + ConflictEdge::ConditionalRequires(_, _) => None, ConflictEdge::Conflict(_) => None, } } fn requires(self) -> Requirement { match self { - ConflictEdge::Requires(match_spec_id) - | ConflictEdge::ConditionalRequires(match_spec_id) => match_spec_id, + ConflictEdge::Requires(match_spec_id) => match_spec_id, + ConflictEdge::ConditionalRequires(_, _) => { + panic!("expected requires edge, found conditional requires") + } ConflictEdge::Conflict(_) => panic!("expected requires edge, found conflict"), } } @@ -560,12 +559,12 @@ impl ConflictGraph { .graph .edges_directed(nx, Direction::Outgoing) .map(|e| match e.weight() { - ConflictEdge::Requires(version_set_id) => (version_set_id, e.target()), - ConflictEdge::ConditionalRequires(version_set_id, _) => { - (version_set_id, e.target()) - } + ConflictEdge::Requires(req) => (req, e.target()), + ConflictEdge::ConditionalRequires(_, req) => (req, e.target()), ConflictEdge::Conflict(_) => unreachable!(), }) + .collect::>() + .into_iter() .chunk_by(|(&version_set_id, _)| version_set_id); for (_, mut deps) in &dependencies { @@ -610,11 +609,13 @@ impl ConflictGraph { .edges_directed(nx, Direction::Outgoing) .map(|e| match e.weight() { ConflictEdge::Requires(version_set_id) => (version_set_id, e.target()), - ConflictEdge::ConditionalRequires(version_set_id, _) => { + ConflictEdge::ConditionalRequires(_, version_set_id) => { (version_set_id, e.target()) } ConflictEdge::Conflict(_) => unreachable!(), }) + .collect::>() + .into_iter() .chunk_by(|(&version_set_id, _)| version_set_id); // Missing if at least one dependency is missing diff --git a/src/lib.rs b/src/lib.rs index 575c678..cdaec9e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -28,7 +28,7 @@ pub use internal::{ mapping::Mapping, }; use itertools::Itertools; -pub use requirement::Requirement; +pub use requirement::{ConditionalRequirement, Requirement}; pub use solver::{Problem, Solver, SolverCache, UnsolvableOrCancelled}; /// An object that is used by the solver to query certain properties of @@ -206,7 +206,7 @@ pub struct KnownDependencies { feature = "serde", serde(default, skip_serializing_if = "Vec::is_empty") )] - pub requirements: Vec, + pub conditional_requirements: Vec, /// Defines additional constraints on packages that may or may not be part /// of the solution. Different from `requirements`, packages in this set diff --git a/src/requirement.rs b/src/requirement.rs index 62589f3..18b6ce8 100644 --- a/src/requirement.rs +++ b/src/requirement.rs @@ -2,6 +2,57 @@ use crate::{Interner, VersionSetId, VersionSetUnionId}; use itertools::Itertools; use std::fmt::Display; +/// Specifies a conditional requirement, where the requirement is only active when the condition is met. +#[derive(Clone, Copy, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct ConditionalRequirement { + /// The condition that must be met for the requirement to be active. + pub condition: Option, + /// The requirement that is only active when the condition is met. + pub requirement: Requirement, +} + +impl ConditionalRequirement { + /// Creates a new conditional requirement. + pub fn new(condition: VersionSetId, requirement: Requirement) -> Self { + Self { + condition: Some(condition), + requirement, + } + } + /// Returns the version sets that satisfy the requirement. + pub fn requirement_version_sets<'i>( + &'i self, + interner: &'i impl Interner, + ) -> impl Iterator + 'i { + self.requirement.version_sets(interner) + } + + /// Returns the version sets that satisfy the requirement, along with the condition that must be met. + pub fn version_sets_with_condition<'i>( + &'i self, + interner: &'i impl Interner, + ) -> impl Iterator)> + 'i { + self.requirement + .version_sets(interner) + .map(move |vs| (vs, self.condition)) + } + + /// Returns the condition and requirement. + pub fn into_condition_and_requirement(self) -> (Option, Requirement) { + (self.condition, self.requirement) + } +} + +impl From for ConditionalRequirement { + fn from(value: Requirement) -> Self { + Self { + condition: None, + requirement: value, + } + } +} + /// Specifies the dependency of a solvable on a set of version sets. #[derive(Clone, Copy, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] @@ -13,9 +64,6 @@ pub enum Requirement { /// This variant is typically used for requirements that can be satisfied by two or more /// version sets belonging to _different_ packages. Union(VersionSetUnionId), - /// Specifies a conditional requirement, where the requirement is only active when the condition is met. - /// First VersionSetId is the condition, second is the requirement. - ConditionalRequires(VersionSetId, VersionSetId), } impl Default for Requirement { @@ -48,22 +96,14 @@ impl Requirement { pub(crate) fn version_sets<'i>( &'i self, interner: &'i impl Interner, - ) -> ( - impl Iterator + 'i, - Option, - ) { - match self { + ) -> impl Iterator + 'i { + match *self { Requirement::Single(version_set) => { - (itertools::Either::Left(std::iter::once(*version_set)), None) + itertools::Either::Left(std::iter::once(version_set)) + } + Requirement::Union(version_set_union) => { + itertools::Either::Right(interner.version_sets_in_union(version_set_union)) } - Requirement::Union(version_set_union) => ( - itertools::Either::Right(interner.version_sets_in_union(*version_set_union)), - None, - ), - Requirement::ConditionalRequires(condition, requirement) => ( - itertools::Either::Left(std::iter::once(*requirement)), - Some(*condition), - ), } } } @@ -98,16 +138,6 @@ impl<'i, I: Interner> Display for DisplayRequirement<'i, I> { write!(f, "{}", formatted_version_sets) } - Requirement::ConditionalRequires(condition, requirement) => { - write!( - f, - "if {} then {} {}", - self.interner.display_version_set(condition), - self.interner - .display_name(self.interner.version_set_name(requirement)), - self.interner.display_version_set(requirement) - ) - } } } } diff --git a/src/snapshot.rs b/src/snapshot.rs index 0c4a986..1d2e458 100644 --- a/src/snapshot.rs +++ b/src/snapshot.rs @@ -220,7 +220,9 @@ impl DependencySnapshot { } } - for &requirement in deps.requirements.iter() { + for &req in deps.conditional_requirements.iter() { + let (_, requirement) = req.into_condition_and_requirement(); // TODO: condition + match requirement { Requirement::Single(version_set) => { if seen.insert(Element::VersionSet(version_set)) { @@ -243,14 +245,6 @@ impl DependencySnapshot { .version_set_unions .insert(version_set_union_id, version_sets); } - Requirement::ConditionalRequires(condition, requirement) => { - if seen.insert(Element::VersionSet(condition)) { - queue.push_back(Element::VersionSet(condition)); - } - if seen.insert(Element::VersionSet(requirement)) { - queue.push_back(Element::VersionSet(requirement)); - } - } } } } diff --git a/src/solver/cache.rs b/src/solver/cache.rs index 952c8cb..cf6c6cc 100644 --- a/src/solver/cache.rs +++ b/src/solver/cache.rs @@ -280,15 +280,6 @@ impl SolverCache { } } } - Requirement::ConditionalRequires(condition, requirement) => { - let candidates = self - .get_or_cache_sorted_candidates_for_version_set(condition) - .await?; - let sorted_candidates = self - .get_or_cache_sorted_candidates_for_version_set(requirement) - .await?; - Ok(sorted_candidates) - } } } diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 15f9d44..2369ebf 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -80,7 +80,7 @@ pub(crate) enum Clause { /// In SAT terms: (¬A ∨ ¬C ∨ B1 ∨ B2 ∨ ... ∨ B99), where A is the solvable, /// C is the condition, and B1 to B99 represent the possible candidates for /// the provided [`Requirement`]. - Conditional(VariableId, Requirement), + Conditional(VariableId, VersionSetId, Requirement), /// Forbids the package on the right-hand side /// /// Note that the package on the left-hand side is not part of the clause, @@ -237,50 +237,61 @@ impl Clause { fn conditional( parent_id: VariableId, requirement: Requirement, + condition: VersionSetId, decision_tracker: &DecisionTracker, requirement_candidates: impl IntoIterator, condition_candidates: impl IntoIterator, ) -> (Self, Option<[Literal; 2]>, bool) { assert_ne!(decision_tracker.assigned_value(parent_id), Some(false)); - let mut candidates = condition_candidates.into_iter().peekable(); - let first_candidate = candidates.peek().copied(); - if let Some(first_candidate) = first_candidate { - match condition_candidates - .into_iter() - .find(|&condition_id| decision_tracker.assigned_value(condition_id) == Some(true)) - { - Some(_) => Clause::requires( - parent_id, - requirement, - requirement_candidates, - decision_tracker, - ), - None => { - if let Some(first_unset_candidate) = candidates.find(|&condition_id| { - decision_tracker.assigned_value(condition_id) != Some(false) - }) { - ( - Clause::Conditional(parent_id, requirement), - Some([parent_id.negative(), first_unset_candidate.negative()]), - false, - ) - } else { - // All condition candidates are assigned to false! Therefore, the clause conflicts with the - // current decisions. There are no valid watches for it at the moment, but we will - // assign default ones nevertheless, because they will become valid after the solver - // restarts. - ( - Clause::Conditional(parent_id, requirement), - Some([parent_id.negative(), first_candidate.negative()]), - true, - ) - } + let mut condition_candidates = condition_candidates.into_iter().peekable(); + let condition_first_candidate = condition_candidates + .peek() + .copied() + .expect("no condition candidates"); + let mut requirement_candidates = requirement_candidates.into_iter().peekable(); + + match condition_candidates + .find(|&condition_id| decision_tracker.assigned_value(condition_id) == Some(true)) + { + Some(_) => { + // Condition is true, find first requirement candidate not set to false + if let Some(req_candidate) = requirement_candidates + .find(|&req_id| decision_tracker.assigned_value(req_id) != Some(false)) + { + ( + Clause::Conditional(parent_id, condition, requirement), + Some([parent_id.negative(), req_candidate.positive()]), + false, + ) + } else { + // No valid requirement candidate, use first condition candidate and mark conflict + ( + Clause::Conditional(parent_id, condition, requirement), + Some([parent_id.negative(), condition_first_candidate.positive()]), + true, + ) + } + } + None => { + // No true condition, look for unset condition + if let Some(unset_condition) = condition_candidates.find(|&condition_id| { + decision_tracker.assigned_value(condition_id) != Some(false) + }) { + ( + Clause::Conditional(parent_id, condition, requirement), + Some([parent_id.negative(), unset_condition.negative()]), + false, + ) + } else { + // All conditions false + ( + Clause::Conditional(parent_id, condition, requirement), + None, + false, + ) } } - } else { - // No condition candidates, so no need to watch anything - (Clause::Conditional(parent_id, requirement), None, false) } } @@ -326,14 +337,22 @@ impl Clause { Clause::Lock(_, s) => [s.negative(), VariableId::root().negative()] .into_iter() .try_fold(init, visit), - Clause::Conditional(package_id, condition) => iter::once(package_id.negative()) - .chain( - requirements_to_sorted_candidates[&condition] - .iter() - .flatten() - .map(|&s| s.positive()), - ) - .try_fold(init, visit), + Clause::Conditional(package_id, condition, requirement) => { + iter::once(package_id.negative()) + .chain( + requirements_to_sorted_candidates[&condition.into()] + .iter() + .flatten() + .map(|&s| s.negative()), + ) + .chain( + requirements_to_sorted_candidates[&requirement] + .iter() + .flatten() + .map(|&s| s.positive()), + ) + .try_fold(init, visit) + } } } @@ -488,13 +507,15 @@ impl WatchedLiterals { /// conflict. pub fn conditional( package_id: VariableId, - condition: Requirement, + requirement: Requirement, + condition: VersionSetId, decision_tracker: &DecisionTracker, requirement_candidates: impl IntoIterator, condition_candidates: impl IntoIterator, ) -> (Option, bool, Clause) { let (kind, watched_literals, conflict) = Clause::conditional( package_id, + requirement, condition, decision_tracker, requirement_candidates, @@ -700,13 +721,14 @@ impl<'i, I: Interner> Display for ClauseDisplay<'i, I> { other, ) } - Clause::Conditional(package_id, condition) => { + Clause::Conditional(package_id, condition, requirement) => { write!( f, - "Conditional({}({:?}), {})", + "Conditional({}({:?}), {}, {})", package_id.display(self.variable_map, self.interner), package_id, - condition.display(self.interner), + self.interner.display_version_set(condition), + requirement.display(self.interner), ) } } diff --git a/src/solver/mod.rs b/src/solver/mod.rs index 5dfea63..a64628c 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -19,6 +19,7 @@ use crate::{ id::{ClauseId, LearntClauseId, NameId, SolvableId, SolvableOrRootId, VariableId}, mapping::Mapping, }, + requirement::ConditionalRequirement, runtime::{AsyncRuntime, NowOrNeverRuntime}, solver::binary_encoding::AtMostOnceTracker, Candidates, Dependencies, DependencyProvider, KnownDependencies, Requirement, VersionSetId, @@ -51,7 +52,7 @@ struct AddClauseOutput { /// This struct follows the builder pattern and can have its fields set by one /// of the available setter methods. pub struct Problem { - requirements: Vec, + requirements: Vec, constraints: Vec, soft_requirements: S, } @@ -80,7 +81,7 @@ impl> Problem { /// /// Returns the [`Problem`] for further mutation or to pass to /// [`Solver::solve`]. - pub fn requirements(self, requirements: Vec) -> Self { + pub fn requirements(self, requirements: Vec) -> Self { Self { requirements, ..self @@ -172,7 +173,7 @@ pub struct Solver { decision_tracker: DecisionTracker, /// The [`Requirement`]s that must be installed as part of the solution. - root_requirements: Vec, + root_requirements: Vec, /// Additional constraints imposed by the root. root_constraints: Vec, @@ -788,7 +789,7 @@ impl Solver { // Get the candidates for the individual version sets. let version_set_candidates = &self.requirement_to_sorted_candidates[deps]; - let (version_sets, condition) = deps.version_sets(self.provider()); + let version_sets = deps.version_sets(self.provider()); // Iterate over all version sets in the requirement and find the first version // set that we can act on, or if a single candidate (from any version set) makes @@ -1518,7 +1519,7 @@ async fn add_clauses_for_solvables( RequirementCandidateVariables, ahash::RandomState, >, - root_requirements: &[Requirement], + root_requirements: &[ConditionalRequirement], root_constraints: &[VersionSetId], ) -> Result> { let mut output = AddClauseOutput::default(); @@ -1533,14 +1534,9 @@ async fn add_clauses_for_solvables( SortedCandidates { solvable_id: SolvableOrRootId, requirement: Requirement, + condition: Option<(VersionSetId, Vec<&'i [SolvableId]>)>, candidates: Vec<&'i [SolvableId]>, }, - ConditionalSortedCandidates { - solvable_id: SolvableOrRootId, - requirement: Requirement, - requirement_candidates: Vec<&'i [SolvableId]>, - condition_candidates: &'i [SolvableId], - }, NonMatchingCandidates { solvable_id: SolvableOrRootId, version_set_id: VersionSetId, @@ -1588,7 +1584,7 @@ async fn add_clauses_for_solvables( ready(Ok(TaskResult::Dependencies { solvable_id: solvable_or_root, dependencies: Dependencies::Known(KnownDependencies { - requirements: root_requirements.to_vec(), + conditional_requirements: root_requirements.to_vec(), constrains: root_constraints.to_vec(), }), })) @@ -1620,8 +1616,8 @@ async fn add_clauses_for_solvables( None => variable_map.root(), }; - let (requirements, constrains) = match dependencies { - Dependencies::Known(deps) => (deps.requirements, deps.constrains), + let (conditional_requirements, constrains) = match dependencies { + Dependencies::Known(deps) => (deps.conditional_requirements, deps.constrains), Dependencies::Unknown(reason) => { // There is no information about the solvable's dependencies, so we add // an exclusion clause for it @@ -1642,11 +1638,10 @@ async fn add_clauses_for_solvables( } }; - for (version_set_id, condition) in requirements + for (version_set_id, condition) in conditional_requirements .iter() - .flat_map(|requirement| { - let (version_sets, condition) = requirement.version_sets(cache.provider()); - version_sets.map(move |vs| (vs, condition)) + .flat_map(|conditional_requirement| { + conditional_requirement.version_sets_with_condition(cache.provider()) }) .chain(constrains.iter().map(|&vs| (vs, None))) { @@ -1668,32 +1663,32 @@ async fn add_clauses_for_solvables( } .boxed_local(), ); - } - if let Some(condition) = condition { - let condition_name = cache.provider().version_set_name(condition); - if clauses_added_for_package.insert(condition_name) { - pending_futures.push( - async move { - let condition_candidates = - cache.get_or_cache_candidates(condition_name).await?; - Ok(TaskResult::Candidates { - name_id: condition_name, - package_candidates: &condition_candidates, - }) - } - .boxed_local(), - ); + if let Some(condition) = condition { + let condition_name = cache.provider().version_set_name(condition); + if clauses_added_for_package.insert(condition_name) { + pending_futures.push( + async move { + let condition_candidates = + cache.get_or_cache_candidates(condition_name).await?; + Ok(TaskResult::Candidates { + name_id: condition_name, + package_candidates: &condition_candidates, + }) + } + .boxed_local(), + ); + } } } } - for requirement in requirements { + for conditional_requirement in conditional_requirements { // Find all the solvable that match for the given version set pending_futures.push( async move { - let (version_sets, condition) = - requirement.version_sets(cache.provider()); + let version_sets = + conditional_requirement.requirement_version_sets(cache.provider()); let candidates = futures::future::try_join_all(version_sets.map(|version_set| { cache @@ -1701,21 +1696,26 @@ async fn add_clauses_for_solvables( })) .await?; - if let Some(condition) = condition { - let condition_candidates = cache - .get_or_cache_sorted_candidates_for_version_set(condition) - .await?; + // condition is `VersionSetId` right now but it will become a `Requirement` + // in the future + if let Some(condition) = conditional_requirement.condition { + let condition_candidates = vec![ + cache + .get_or_cache_sorted_candidates_for_version_set(condition) + .await?, + ]; - Ok(TaskResult::ConditionalSortedCandidates { + Ok(TaskResult::SortedCandidates { solvable_id, - requirement, - requirement_candidates: candidates, - condition_candidates, + requirement: conditional_requirement.requirement, + condition: Some((condition, condition_candidates)), + candidates, }) } else { Ok(TaskResult::SortedCandidates { solvable_id, - requirement, + requirement: conditional_requirement.requirement, + condition: None, candidates, }) } @@ -1787,6 +1787,7 @@ async fn add_clauses_for_solvables( TaskResult::SortedCandidates { solvable_id, requirement, + condition, candidates, } => { tracing::trace!( @@ -1856,104 +1857,42 @@ async fn add_clauses_for_solvables( ); } - // Add the requirements clause - let no_candidates = candidates.iter().all(|candidates| candidates.is_empty()); - let (watched_literals, conflict, kind) = WatchedLiterals::requires( - variable, - requirement, - version_set_variables.iter().flatten().copied(), - decision_tracker, - ); - let has_watches = watched_literals.is_some(); - let clause_id = clauses.alloc(watched_literals, kind); - - if has_watches { - output.clauses_to_watch.push(clause_id); - } - - output - .new_requires_clauses - .push((variable, requirement, clause_id)); - - if conflict { - output.conflicting_clauses.push(clause_id); - } else if no_candidates { - // Add assertions for unit clauses (i.e. those with no matching candidates) - output.negative_assertions.push((variable, clause_id)); - } - } - TaskResult::ConditionalSortedCandidates { - solvable_id, - requirement, - requirement_candidates, - condition_candidates, - } => { - tracing::trace!( - "conditional candidates available for {}", - solvable_id.display(cache.provider()), - ); - - // Allocate a variable for the solvable - let variable = match solvable_id.solvable() { - Some(solvable_id) => variable_map.intern_solvable(solvable_id), - None => variable_map.root(), - }; - - // Cache the candidates for this requirement - let version_set_variables: Vec<_> = requirement_candidates - .iter() - .map(|candidates| { - candidates - .iter() - .map(|&candidate| variable_map.intern_solvable(candidate)) - .collect::>() - }) - .collect(); - - requirement_to_sorted_candidates - .insert(requirement.clone(), version_set_variables.clone()); + let (watched_literals, conflict, kind) = + if let Some((condition, condition_candidates)) = condition { + let condition_version_set_variables = requirement_to_sorted_candidates + .insert( + condition.into(), + condition_candidates + .iter() + .map(|&candidates| { + candidates + .iter() + .map(|&var| variable_map.intern_solvable(var)) + .collect() + }) + .collect(), + ); - // Add forbidden clauses for the candidates - for candidates in requirement_candidates.iter() { - for &candidate in *candidates { - let candidate_var = variable_map.intern_solvable(candidate); - let name_id = cache.provider().solvable_name(candidate); - let other_solvables = forbidden_clauses_added.entry(name_id).or_default(); - other_solvables.add( - candidate_var, - |a, b, positive| { - let (watched_literals, kind) = WatchedLiterals::forbid_multiple( - a, - if positive { b.positive() } else { b.negative() }, - name_id, - ); - let clause_id = clauses.alloc(watched_literals, kind); - debug_assert!( - clauses.watched_literals[clause_id.to_usize()].is_some() - ); - output.clauses_to_watch.push(clause_id); - }, - || variable_map.alloc_forbid_multiple_variable(name_id), - ); - } - } + // Add a condition clause + WatchedLiterals::conditional( + variable, + requirement, + condition, + decision_tracker, + version_set_variables.iter().flatten().copied(), + condition_version_set_variables.iter().flatten().copied(), + ) + } else { + WatchedLiterals::requires( + variable, + requirement, + version_set_variables.iter().flatten().copied(), + decision_tracker, + ) + }; // Add the requirements clause - let no_candidates = requirement_candidates - .iter() - .all(|candidates| candidates.is_empty()); - let condition_variables = condition_candidates - .iter() - .map(|&candidate| variable_map.intern_solvable(candidate)) - .collect::>(); - - let (watched_literals, conflict, kind) = WatchedLiterals::conditional( - variable, - requirement, - decision_tracker, - version_set_variables.iter().flatten().copied(), - condition_variables.iter().copied(), - ); + let no_candidates = candidates.iter().all(|candidates| candidates.is_empty()); let has_watches = watched_literals.is_some(); let clause_id = clauses.alloc(watched_literals, kind); diff --git a/tools/solve-snapshot/src/main.rs b/tools/solve-snapshot/src/main.rs index 901996c..3629eaf 100644 --- a/tools/solve-snapshot/src/main.rs +++ b/tools/solve-snapshot/src/main.rs @@ -128,7 +128,8 @@ fn main() { let start = Instant::now(); - let problem = Problem::default().requirements(requirements); + let problem = + Problem::default().requirements(requirements.into_iter().map(Into::into).collect()); let mut solver = Solver::new(provider); let mut records = None; let mut error = None; From 995a2c398d43ecc9f9440ab368e355b1a40b30aa Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 21 Jan 2025 16:24:24 -0500 Subject: [PATCH 04/41] minor fix --- src/solver/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/solver/mod.rs b/src/solver/mod.rs index a64628c..d1d2ba1 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -1673,7 +1673,7 @@ async fn add_clauses_for_solvables( cache.get_or_cache_candidates(condition_name).await?; Ok(TaskResult::Candidates { name_id: condition_name, - package_candidates: &condition_candidates, + package_candidates: condition_candidates, }) } .boxed_local(), From dd636d293f033f40e1ed65a062ac5b045c634753 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 21 Jan 2025 16:26:22 -0500 Subject: [PATCH 05/41] remove deprecated conditional requirement function from resolvo.h --- cpp/include/resolvo.h | 9 --------- 1 file changed, 9 deletions(-) diff --git a/cpp/include/resolvo.h b/cpp/include/resolvo.h index c824df4..97d00f5 100644 --- a/cpp/include/resolvo.h +++ b/cpp/include/resolvo.h @@ -24,15 +24,6 @@ inline Requirement requirement_union(VersionSetUnionId id) { return cbindgen_private::resolvo_requirement_union(id); } -/** - * Specifies a conditional requirement, where the requirement is only active when the condition is met. - * @param condition The version set that must be satisfied for the requirement to be active. - * @param requirement The version set that must be satisfied when the condition is met. - */ -inline Requirement requirement_conditional(VersionSetId condition, VersionSetId requirement) { - return cbindgen_private::resolvo_requirement_conditional(condition, requirement); -} - /** * Called to solve a package problem. * From eb4d253084c4da3aedcafe163a146ba598c9d0ce Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Wed, 22 Jan 2025 00:18:48 -0500 Subject: [PATCH 06/41] add a map of conditional clauses to the DependencyProvider struct --- src/solver/mod.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/solver/mod.rs b/src/solver/mod.rs index d1d2ba1..1814603 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -37,6 +37,7 @@ mod watch_map; #[derive(Default)] struct AddClauseOutput { new_requires_clauses: Vec<(VariableId, Requirement, ClauseId)>, + new_conditional_clauses: Vec<(VariableId, VersionSetId, Requirement, ClauseId)>, conflicting_clauses: Vec, negative_assertions: Vec<(VariableId, ClauseId)>, clauses_to_watch: Vec, @@ -151,6 +152,7 @@ pub struct Solver { pub(crate) clauses: Clauses, requires_clauses: IndexMap, ahash::RandomState>, + conditional_clauses: IndexMap, ahash::RandomState>, watches: WatchMap, /// A mapping from requirements to the variables that represent the @@ -201,6 +203,7 @@ impl Solver { clauses: Clauses::default(), variable_map: VariableMap::default(), requires_clauses: Default::default(), + conditional_clauses: Default::default(), requirement_to_sorted_candidates: FrozenMap::default(), watches: WatchMap::new(), negative_assertions: Default::default(), @@ -281,6 +284,7 @@ impl Solver { clauses: self.clauses, variable_map: self.variable_map, requires_clauses: self.requires_clauses, + conditional_clauses: self.conditional_clauses, requirement_to_sorted_candidates: self.requirement_to_sorted_candidates, watches: self.watches, negative_assertions: self.negative_assertions, @@ -661,6 +665,14 @@ impl Solver { .or_default() .push((requirement, clause_id)); } + + for (solvable_id, condition, requirement, clause_id) in output.new_conditional_clauses { + self.conditional_clauses + .entry(solvable_id) + .or_default() + .push((condition, requirement, clause_id)); + } + self.negative_assertions .append(&mut output.negative_assertions); From 451536e0eacef451cbaa505ed72055d4246a323f Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Wed, 22 Jan 2025 00:18:49 -0500 Subject: [PATCH 07/41] add the conditional clauses to the DependencyProvider --- src/solver/mod.rs | 263 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 216 insertions(+), 47 deletions(-) diff --git a/src/solver/mod.rs b/src/solver/mod.rs index 1814603..ece8f77 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -927,6 +927,154 @@ impl Solver { } } + for (&solvable_id, conditional_clauses) in self.conditional_clauses.iter() { + let is_explicit_requirement = solvable_id == VariableId::root(); + if let Some(best_decision) = &best_decision { + // If we already have an explicit requirement, there is no need to evaluate + // non-explicit requirements. + if best_decision.is_explicit_requirement && !is_explicit_requirement { + continue; + } + } + + // Consider only clauses in which we have decided to install the solvable + if self.decision_tracker.assigned_value(solvable_id) != Some(true) { + continue; + } + + for (condition, requirement, clause_id) in conditional_clauses.iter() { + let mut candidate = ControlFlow::Break(()); + + // Get the candidates for the individual version sets. + let version_set_candidates = &self.requirement_to_sorted_candidates[condition]; + + let version_sets = condition.version_sets(self.provider()); + + // Iterate over all version sets in the requirement and find the first version + // set that we can act on, or if a single candidate (from any version set) makes + // the clause true. + // + // NOTE: We zip the version sets from the requirements and the variables that we + // previously cached. This assumes that the order of the version sets is the + // same in both collections. + for (version_set, candidates) in version_sets.zip(version_set_candidates) { + // Find the first candidate that is not yet assigned a value or find the first + // value that makes this clause true. + candidate = candidates.iter().try_fold( + match candidate { + ControlFlow::Continue(x) => x, + _ => None, + }, + |first_candidate, &candidate| { + let assigned_value = self.decision_tracker.assigned_value(candidate); + ControlFlow::Continue(match assigned_value { + Some(true) => { + // This candidate has already been assigned so the clause is + // already true. Skip it. + return ControlFlow::Break(()); + } + Some(false) => { + // This candidate has already been assigned false, continue the + // search. + first_candidate + } + None => match first_candidate { + Some(( + first_candidate, + candidate_version_set, + mut candidate_count, + package_activity, + )) => { + // We found a candidate that has not been assigned yet, but + // it is not the first candidate. + if candidate_version_set == version_set { + // Increment the candidate count if this is a candidate + // in the same version set. + candidate_count += 1u32; + } + Some(( + first_candidate, + candidate_version_set, + candidate_count, + package_activity, + )) + } + None => { + // We found the first candidate that has not been assigned + // yet. + let package_activity = self.name_activity[self + .provider() + .version_set_name(version_set) + .to_usize()]; + Some((candidate, version_set, 1, package_activity)) + } + }, + }) + }, + ); + + // Stop searching if we found a candidate that makes the clause true. + if candidate.is_break() { + break; + } + } + + match candidate { + ControlFlow::Break(_) => { + // A candidate has been assigned true which means the clause is already + // true, and we can skip it. + 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.") + } + ControlFlow::Continue(Some(( + candidate, + _version_set_id, + candidate_count, + package_activity, + ))) => { + let decision = (candidate, solvable_id, *clause_id); + best_decision = Some(match &best_decision { + None => PossibleDecision { + is_explicit_requirement, + package_activity, + candidate_count, + decision, + }, + Some(best_decision) => { + // Prefer decisions on explicit requirements over non-explicit + // requirements. This optimizes direct dependencies over transitive + // dependencies. + if best_decision.is_explicit_requirement && !is_explicit_requirement + { + continue; + } + + // Prefer decisions with a higher package activity score to root out + // conflicts faster. + if best_decision.package_activity >= package_activity { + continue; + } + + if best_decision.candidate_count <= candidate_count { + continue; + } + + PossibleDecision { + is_explicit_requirement, + package_activity, + candidate_count, + decision, + } + } + }) + } + } + } + + } + if let Some(PossibleDecision { candidate_count, package_activity, @@ -1869,59 +2017,80 @@ async fn add_clauses_for_solvables( ); } - let (watched_literals, conflict, kind) = - if let Some((condition, condition_candidates)) = condition { - let condition_version_set_variables = requirement_to_sorted_candidates - .insert( - condition.into(), - condition_candidates - .iter() - .map(|&candidates| { - candidates - .iter() - .map(|&var| variable_map.intern_solvable(var)) - .collect() - }) - .collect(), - ); + if let Some((condition, condition_candidates)) = condition { + let condition_version_set_variables = requirement_to_sorted_candidates + .insert( + condition.into(), + condition_candidates + .iter() + .map(|&candidates| { + candidates + .iter() + .map(|&var| variable_map.intern_solvable(var)) + .collect() + }) + .collect(), + ); - // Add a condition clause - WatchedLiterals::conditional( - variable, - requirement, - condition, - decision_tracker, - version_set_variables.iter().flatten().copied(), - condition_version_set_variables.iter().flatten().copied(), - ) - } else { - WatchedLiterals::requires( - variable, - requirement, - version_set_variables.iter().flatten().copied(), - decision_tracker, - ) - }; + // Add a condition clause + let (watched_literals, conflict, kind) = WatchedLiterals::conditional( + variable, + requirement, + condition, + decision_tracker, + version_set_variables.iter().flatten().copied(), + condition_version_set_variables.iter().flatten().copied(), + ); + + // Add the conditional clause + let no_candidates = candidates.iter().all(|candidates| candidates.is_empty()); + + let has_watches = watched_literals.is_some(); + let clause_id = clauses.alloc(watched_literals, kind); - // Add the requirements clause - let no_candidates = candidates.iter().all(|candidates| candidates.is_empty()); + if has_watches { + output.clauses_to_watch.push(clause_id); + } - let has_watches = watched_literals.is_some(); - let clause_id = clauses.alloc(watched_literals, kind); + output + .new_conditional_clauses + .push((variable, condition, requirement, clause_id)); - if has_watches { - output.clauses_to_watch.push(clause_id); - } + if conflict { + output.conflicting_clauses.push(clause_id); + } else if no_candidates { + // Add assertions for unit clauses (i.e. those with no matching candidates) + output.negative_assertions.push((variable, clause_id)); + } + + } else { + let (watched_literals, conflict, kind) = WatchedLiterals::requires( + variable, + requirement, + version_set_variables.iter().flatten().copied(), + decision_tracker, + ); + + // Add the requirements clause + let no_candidates = candidates.iter().all(|candidates| candidates.is_empty()); - output - .new_requires_clauses - .push((variable, requirement, clause_id)); + let has_watches = watched_literals.is_some(); + let clause_id = clauses.alloc(watched_literals, kind); + + if has_watches { + output.clauses_to_watch.push(clause_id); + } - if conflict { - output.conflicting_clauses.push(clause_id); - } else if no_candidates { - // Add assertions for unit clauses (i.e. those with no matching candidates) - output.negative_assertions.push((variable, clause_id)); + output + .new_requires_clauses + .push((variable, requirement, clause_id)); + + if conflict { + output.conflicting_clauses.push(clause_id); + } else if no_candidates { + // Add assertions for unit clauses (i.e. those with no matching candidates) + output.negative_assertions.push((variable, clause_id)); + } } } TaskResult::NonMatchingCandidates { From a9fdad27c07b0758e4cf45d001931df4d9be012a Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Wed, 22 Jan 2025 11:00:46 -0500 Subject: [PATCH 08/41] update decide function to iterate over the conditional clauses. --- src/solver/mod.rs | 240 ++++++++++++++-------------------------------- 1 file changed, 70 insertions(+), 170 deletions(-) diff --git a/src/solver/mod.rs b/src/solver/mod.rs index ece8f77..7700f96 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -152,7 +152,8 @@ pub struct Solver { pub(crate) clauses: Clauses, requires_clauses: IndexMap, ahash::RandomState>, - conditional_clauses: IndexMap, ahash::RandomState>, + conditional_clauses: + IndexMap<(VariableId, VersionSetId), Vec<(Requirement, ClauseId)>, ahash::RandomState>, watches: WatchMap, /// A mapping from requirements to the variables that represent the @@ -668,9 +669,9 @@ impl Solver { for (solvable_id, condition, requirement, clause_id) in output.new_conditional_clauses { self.conditional_clauses - .entry(solvable_id) + .entry((solvable_id, condition)) .or_default() - .push((condition, requirement, clause_id)); + .push((requirement, clause_id)); } self.negative_assertions @@ -780,8 +781,39 @@ impl Solver { } let mut best_decision: Option = None; - for (&solvable_id, requirements) in self.requires_clauses.iter() { - let is_explicit_requirement = solvable_id == VariableId::root(); + + // Chain together the requires_clauses and conditional_clauses iterations + let requires_iter = self + .requires_clauses + .iter() + .map(|(&solvable_id, requirements)| { + ( + solvable_id, + None, + requirements + .iter() + .map(|(r, c)| (*r, *c)) + .collect::>(), + ) + }); + + let conditional_iter = + self.conditional_clauses + .iter() + .map(|((solvable_id, condition), clauses)| { + ( + *solvable_id, + Some(*condition), + clauses.iter().map(|(r, c)| (*r, *c)).collect::>(), + ) + }); + + for (solvable_id, condition, requirements) in requires_iter.chain(conditional_iter) { + let is_explicit_requirement = match condition { + None => solvable_id == VariableId::root(), + Some(_) => solvable_id == VariableId::root(), + }; + if let Some(best_decision) = &best_decision { // If we already have an explicit requirement, there is no need to evaluate // non-explicit requirements. @@ -795,160 +827,28 @@ impl Solver { continue; } - for (deps, clause_id) in requirements.iter() { - let mut candidate = ControlFlow::Break(()); - - // Get the candidates for the individual version sets. - let version_set_candidates = &self.requirement_to_sorted_candidates[deps]; + // For conditional clauses, check that at least one conditional variable is true + if let Some(condition) = condition { + let condition_requirement: Requirement = condition.into(); + let conditional_candidates = + &self.requirement_to_sorted_candidates[&condition_requirement]; - let version_sets = deps.version_sets(self.provider()); - - // Iterate over all version sets in the requirement and find the first version - // set that we can act on, or if a single candidate (from any version set) makes - // the clause true. - // - // NOTE: We zip the version sets from the requirements and the variables that we - // previously cached. This assumes that the order of the version sets is the - // same in both collections. - for (version_set, candidates) in version_sets.zip(version_set_candidates) { - // Find the first candidate that is not yet assigned a value or find the first - // value that makes this clause true. - candidate = candidates.iter().try_fold( - match candidate { - ControlFlow::Continue(x) => x, - _ => None, - }, - |first_candidate, &candidate| { - let assigned_value = self.decision_tracker.assigned_value(candidate); - ControlFlow::Continue(match assigned_value { - Some(true) => { - // This candidate has already been assigned so the clause is - // already true. Skip it. - return ControlFlow::Break(()); - } - Some(false) => { - // This candidate has already been assigned false, continue the - // search. - first_candidate - } - None => match first_candidate { - Some(( - first_candidate, - candidate_version_set, - mut candidate_count, - package_activity, - )) => { - // We found a candidate that has not been assigned yet, but - // it is not the first candidate. - if candidate_version_set == version_set { - // Increment the candidate count if this is a candidate - // in the same version set. - candidate_count += 1u32; - } - Some(( - first_candidate, - candidate_version_set, - candidate_count, - package_activity, - )) - } - None => { - // We found the first candidate that has not been assigned - // yet. - let package_activity = self.name_activity[self - .provider() - .version_set_name(version_set) - .to_usize()]; - Some((candidate, version_set, 1, package_activity)) - } - }, - }) - }, - ); - - // Stop searching if we found a candidate that makes the clause true. - if candidate.is_break() { - break; - } - } - - match candidate { - ControlFlow::Break(_) => { - // A candidate has been assigned true which means the clause is already - // true, and we can skip it. - 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.") - } - ControlFlow::Continue(Some(( - candidate, - _version_set_id, - candidate_count, - package_activity, - ))) => { - let decision = (candidate, solvable_id, *clause_id); - best_decision = Some(match &best_decision { - None => PossibleDecision { - is_explicit_requirement, - package_activity, - candidate_count, - decision, - }, - Some(best_decision) => { - // Prefer decisions on explicit requirements over non-explicit - // requirements. This optimizes direct dependencies over transitive - // dependencies. - if best_decision.is_explicit_requirement && !is_explicit_requirement - { - continue; - } - - // Prefer decisions with a higher package activity score to root out - // conflicts faster. - if best_decision.package_activity >= package_activity { - continue; - } - - if best_decision.candidate_count <= candidate_count { - continue; - } - - PossibleDecision { - is_explicit_requirement, - package_activity, - candidate_count, - decision, - } - } - }) - } - } - } - } - - for (&solvable_id, conditional_clauses) in self.conditional_clauses.iter() { - let is_explicit_requirement = solvable_id == VariableId::root(); - if let Some(best_decision) = &best_decision { - // If we already have an explicit requirement, there is no need to evaluate - // non-explicit requirements. - if best_decision.is_explicit_requirement && !is_explicit_requirement { + if !conditional_candidates.iter().any(|candidates| { + candidates.iter().any(|&candidate| { + self.decision_tracker.assigned_value(candidate) == Some(true) + }) + }) { continue; } } - // Consider only clauses in which we have decided to install the solvable - if self.decision_tracker.assigned_value(solvable_id) != Some(true) { - continue; - } - - for (condition, requirement, clause_id) in conditional_clauses.iter() { + for (requirement, clause_id) in requirements { let mut candidate = ControlFlow::Break(()); // Get the candidates for the individual version sets. - let version_set_candidates = &self.requirement_to_sorted_candidates[condition]; + let version_set_candidates = &self.requirement_to_sorted_candidates[&requirement]; - let version_sets = condition.version_sets(self.provider()); + let version_sets = requirement.version_sets(self.provider()); // Iterate over all version sets in the requirement and find the first version // set that we can act on, or if a single candidate (from any version set) makes @@ -1034,7 +934,7 @@ impl Solver { candidate_count, package_activity, ))) => { - let decision = (candidate, solvable_id, *clause_id); + let decision = (candidate, solvable_id, clause_id); best_decision = Some(match &best_decision { None => PossibleDecision { is_explicit_requirement, @@ -1072,7 +972,6 @@ impl Solver { } } } - } if let Some(PossibleDecision { @@ -2018,19 +1917,18 @@ async fn add_clauses_for_solvables( } if let Some((condition, condition_candidates)) = condition { - let condition_version_set_variables = requirement_to_sorted_candidates - .insert( - condition.into(), - condition_candidates - .iter() - .map(|&candidates| { - candidates - .iter() - .map(|&var| variable_map.intern_solvable(var)) - .collect() - }) - .collect(), - ); + let condition_version_set_variables = requirement_to_sorted_candidates.insert( + condition.into(), + condition_candidates + .iter() + .map(|&candidates| { + candidates + .iter() + .map(|&var| variable_map.intern_solvable(var)) + .collect() + }) + .collect(), + ); // Add a condition clause let (watched_literals, conflict, kind) = WatchedLiterals::conditional( @@ -2041,7 +1939,7 @@ async fn add_clauses_for_solvables( version_set_variables.iter().flatten().copied(), condition_version_set_variables.iter().flatten().copied(), ); - + // Add the conditional clause let no_candidates = candidates.iter().all(|candidates| candidates.is_empty()); @@ -2052,9 +1950,12 @@ async fn add_clauses_for_solvables( output.clauses_to_watch.push(clause_id); } - output - .new_conditional_clauses - .push((variable, condition, requirement, clause_id)); + output.new_conditional_clauses.push(( + variable, + condition, + requirement, + clause_id, + )); if conflict { output.conflicting_clauses.push(clause_id); @@ -2062,7 +1963,6 @@ async fn add_clauses_for_solvables( // Add assertions for unit clauses (i.e. those with no matching candidates) output.negative_assertions.push((variable, clause_id)); } - } else { let (watched_literals, conflict, kind) = WatchedLiterals::requires( variable, From e62b91c91dff530072dd3acb4734f0a29bc02a57 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Wed, 22 Jan 2025 11:39:13 -0500 Subject: [PATCH 09/41] add new node types and labels for conditional edges int he conflict graph --- src/conflict.rs | 100 ++++++++++++++++++++++++------------------------ 1 file changed, 49 insertions(+), 51 deletions(-) diff --git a/src/conflict.rs b/src/conflict.rs index 6c1ace8..a597301 100644 --- a/src/conflict.rs +++ b/src/conflict.rs @@ -159,52 +159,62 @@ impl Conflict { ConflictEdge::Conflict(ConflictCause::Constrains(version_set_id)), ); } - &Clause::Conditional(package_id, condition, version_set_id) => { + &Clause::Conditional(package_id, condition, requirement) => { let solvable = package_id .as_solvable_or_root(&solver.variable_map) .expect("only solvables can be excluded"); let package_node = Self::add_node(&mut graph, &mut nodes, solvable); - let candidates = solver.async_runtime.block_on(solver.cache.get_or_cache_sorted_candidates(version_set_id)).unwrap_or_else(|_| { - unreachable!("The version set was used in the solver, so it must have been cached. Therefore cancellation is impossible here and we cannot get an `Err(...)`") + let requirement_candidates = solver + .async_runtime + .block_on(solver.cache.get_or_cache_sorted_candidates( + requirement, + )) + .unwrap_or_else(|_| { + unreachable!( + "The version set was used in the solver, so it must have been cached. Therefore cancellation is impossible here and we cannot get an `Err(...)`" + ) + }); + + let conditional_candidates = solver.async_runtime.block_on(solver.cache.get_or_cache_sorted_candidates(condition.into())).unwrap_or_else(|_| { + unreachable!("The condition's version set was used in the solver, so it must have been cached. Therefore cancellation is impossible here and we cannot get an `Err(...)`") }); - if candidates.is_empty() { + if requirement_candidates.is_empty() { + tracing::trace!( + "{package_id:?} conditionally requires {requirement:?}, which has no candidates" + ); + graph.add_edge( + package_node, + unresolved_node, + ConflictEdge::ConditionalRequires(condition, requirement), + ); + } else if conditional_candidates.is_empty() { tracing::trace!( - "{package_id:?} conditionally requires {version_set_id:?}, which has no candidates" + "{package_id:?} conditionally requires {requirement:?}, but the condition has no candidates" ); graph.add_edge( package_node, unresolved_node, - ConflictEdge::ConditionalRequires(condition, version_set_id), + ConflictEdge::ConditionalRequires(condition, requirement), ); } else { - for &candidate_id in candidates { + for &candidate_id in conditional_candidates { tracing::trace!( - "{package_id:?} conditionally requires {candidate_id:?}" + "{package_id:?} conditionally requires {requirement:?} if {candidate_id:?}" ); - let candidate_node = - Self::add_node(&mut graph, &mut nodes, candidate_id.into()); - graph.add_edge( - package_node, - candidate_node, - ConflictEdge::ConditionalRequires(condition, version_set_id), - ); + for &candidate_id in requirement_candidates { + let candidate_node = + Self::add_node(&mut graph, &mut nodes, candidate_id.into()); + graph.add_edge( + package_node, + candidate_node, + ConflictEdge::ConditionalRequires(condition, requirement), + ); + } } } - - // TODO: Add an edge for the unsatisfied condition if it exists - // // Add an edge for the unsatisfied condition if it exists - // if let Some(condition_solvable) = condition.as_solvable(&solver.variable_map) { - // let condition_node = - // Self::add_node(&mut graph, &mut nodes, condition_solvable.into()); - // graph.add_edge( - // package_node, - // condition_node, - // ConflictEdge::Conflict(ConflictCause::UnsatisfiedCondition(condition.into())), - // ); - // } } } } @@ -327,8 +337,6 @@ pub(crate) enum ConflictCause { ForbidMultipleInstances, /// The node was excluded Excluded, - /// The condition for a conditional dependency was not satisfied - UnsatisfiedCondition(Requirement), } /// Represents a node that has been merged with others @@ -395,6 +403,11 @@ impl ConflictGraph { ConflictEdge::Requires(_) if target != ConflictNode::UnresolvedDependency => { "black" } + ConflictEdge::ConditionalRequires(_, _) + if target != ConflictNode::UnresolvedDependency => + { + "blue" + } _ => "red", }; @@ -402,6 +415,13 @@ impl ConflictGraph { ConflictEdge::Requires(requirement) => { requirement.display(interner).to_string() } + ConflictEdge::ConditionalRequires(version_set_id, requirement) => { + format!( + "if {} then {}", + interner.display_version_set(*version_set_id), + requirement.display(interner) + ) + } ConflictEdge::Conflict(ConflictCause::Constrains(version_set_id)) => { interner.display_version_set(*version_set_id).to_string() } @@ -410,18 +430,6 @@ impl ConflictGraph { "already installed".to_string() } ConflictEdge::Conflict(ConflictCause::Excluded) => "excluded".to_string(), - ConflictEdge::Conflict(ConflictCause::UnsatisfiedCondition(condition)) => { - // let condition_solvable = condition.as_solvable(&solver.variable_map) - // .expect("condition must be a solvable"); - // format!("unsatisfied condition: {}", condition_solvable.display(interner)) - todo!() - } - ConflictEdge::ConditionalRequires(requirement, condition) => { - // let condition_solvable = condition.as_solvable(&solver.variable_map) - // .expect("condition must be a solvable"); - // format!("if {} then {}", condition_solvable.display(interner), requirement.display(interner)) - todo!() - } }; let target = match target { @@ -1120,16 +1128,6 @@ impl<'i, I: Interner> fmt::Display for DisplayUnsat<'i, I> { )?; } ConflictCause::Excluded => continue, - &ConflictCause::UnsatisfiedCondition(condition) => { - // let condition_solvable = condition.as_solvable(self.variable_map) - // .expect("condition must be a solvable"); - // writeln!( - // f, - // "{indent}condition {} is not satisfied", - // condition_solvable.display(self.interner), - // )?; - todo!() - } }; } } From db7ac8bae89dec5720d10f80685edb24933834d4 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Wed, 22 Jan 2025 11:43:00 -0500 Subject: [PATCH 10/41] Fix the issues with current tests --- src/requirement.rs | 9 +++++++++ tests/solver.rs | 25 ++++++++++++++----------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/requirement.rs b/src/requirement.rs index 18b6ce8..b498027 100644 --- a/src/requirement.rs +++ b/src/requirement.rs @@ -53,6 +53,15 @@ impl From for ConditionalRequirement { } } +impl From for ConditionalRequirement { + fn from(value: VersionSetId) -> Self { + Self { + condition: None, + requirement: value.into(), + } + } +} + /// Specifies the dependency of a solvable on a set of version sets. #[derive(Clone, Copy, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] diff --git a/tests/solver.rs b/tests/solver.rs index de15d8a..1376528 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -22,9 +22,9 @@ use itertools::Itertools; use resolvo::{ snapshot::{DependencySnapshot, SnapshotProvider}, utils::Pool, - Candidates, Dependencies, DependencyProvider, Interner, KnownDependencies, NameId, Problem, - Requirement, SolvableId, Solver, SolverCache, StringId, UnsolvableOrCancelled, VersionSetId, - VersionSetUnionId, + Candidates, ConditionalRequirement, Dependencies, DependencyProvider, Interner, + KnownDependencies, NameId, Problem, Requirement, SolvableId, Solver, SolverCache, StringId, + UnsolvableOrCancelled, VersionSetId, VersionSetUnionId, }; use tracing_test::traced_test; use version_ranges::Ranges; @@ -123,9 +123,7 @@ impl Spec { pub fn parse_union( spec: &str, ) -> impl Iterator::Err>> + '_ { - spec.split('|') - .map(str::trim) - .map(|dep| Spec::from_str(dep)) + spec.split('|').map(str::trim).map(Spec::from_str) } } @@ -386,7 +384,7 @@ impl DependencyProvider for BundleBoxProvider { candidates .iter() .copied() - .filter(|s| range.contains(&self.pool.resolve_solvable(*s).record) == !inverse) + .filter(|s| range.contains(&self.pool.resolve_solvable(*s).record) != inverse) .collect() } @@ -486,7 +484,7 @@ impl DependencyProvider for BundleBoxProvider { }; let mut result = KnownDependencies { - requirements: Vec::with_capacity(deps.dependencies.len()), + conditional_requirements: Vec::with_capacity(deps.dependencies.len()), constrains: Vec::with_capacity(deps.constrains.len()), }; for req in &deps.dependencies { @@ -516,7 +514,12 @@ impl DependencyProvider for BundleBoxProvider { .into() }; - result.requirements.push(requirement); + result + .conditional_requirements + .push(ConditionalRequirement { + requirement, + condition: None, + }); } for req in &deps.constrains { @@ -538,7 +541,7 @@ impl DependencyProvider for BundleBoxProvider { } /// Create a string from a [`Transaction`] -fn transaction_to_string(interner: &impl Interner, solvables: &Vec) -> String { +fn transaction_to_string(interner: &impl Interner, solvables: &[SolvableId]) -> String { use std::fmt::Write; let mut buf = String::new(); for solvable in solvables @@ -590,7 +593,7 @@ fn solve_snapshot(mut provider: BundleBoxProvider, specs: &[&str]) -> String { let requirements = provider.parse_requirements(specs); let mut solver = Solver::new(provider).with_runtime(runtime); - let problem = Problem::new().requirements(requirements); + let problem = Problem::new().requirements(requirements.into_iter().map(|r| r.into()).collect()); match solver.solve(problem) { Ok(solvables) => transaction_to_string(solver.provider(), &solvables), Err(UnsolvableOrCancelled::Unsolvable(conflict)) => { From 766a1a611f3d97b417901d5620c3f376cd43a0af Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Wed, 22 Jan 2025 14:32:38 -0500 Subject: [PATCH 11/41] fix cpp bindings and tests --- cpp/_deps/corrosion-src | 1 + cpp/include/resolvo.h | 18 +++++ cpp/src/lib.rs | 120 ++++++++++++++++++++++++++++------ cpp/tests/solve.cpp | 16 +++-- src/solver/clause.rs | 16 ++--- tests/.solver.rs.pending-snap | 10 +++ tests/solver.rs | 71 ++++++++++++++++++++ 7 files changed, 215 insertions(+), 37 deletions(-) create mode 160000 cpp/_deps/corrosion-src create mode 100644 tests/.solver.rs.pending-snap diff --git a/cpp/_deps/corrosion-src b/cpp/_deps/corrosion-src new file mode 160000 index 0000000..ff5a236 --- /dev/null +++ b/cpp/_deps/corrosion-src @@ -0,0 +1 @@ +Subproject commit ff5a236550afc591f034e6cc5134ca9e5371bd98 diff --git a/cpp/include/resolvo.h b/cpp/include/resolvo.h index 97d00f5..5343ac7 100644 --- a/cpp/include/resolvo.h +++ b/cpp/include/resolvo.h @@ -4,6 +4,7 @@ #include "resolvo_internal.h" namespace resolvo { +using cbindgen_private::ConditionalRequirement; using cbindgen_private::Problem; using cbindgen_private::Requirement; @@ -24,6 +25,23 @@ inline Requirement requirement_union(VersionSetUnionId id) { return cbindgen_private::resolvo_requirement_union(id); } +/** + * Specifies a conditional requirement (dependency) of a single version set. + * A solvable belonging to the version set satisfies the requirement if the condition is true. + */ +inline ConditionalRequirement conditional_requirement_single(VersionSetId id) { + return cbindgen_private::resolvo_conditional_requirement_single(id); +} + +/** + * Specifies a conditional requirement (dependency) of the union (logical OR) of multiple version + * sets. A solvable belonging to any of the version sets contained in the union satisfies the + * requirement if the condition is true. + */ +inline ConditionalRequirement conditional_requirement_union(VersionSetUnionId id) { + return cbindgen_private::resolvo_conditional_requirement_union(id); +} + /** * Called to solve a package problem. * diff --git a/cpp/src/lib.rs b/cpp/src/lib.rs index 04d05c0..6a1bda2 100644 --- a/cpp/src/lib.rs +++ b/cpp/src/lib.rs @@ -31,6 +31,66 @@ impl From for resolvo::SolvableId { } } +/// A wrapper around an optional version set id. +/// cbindgen:derive-eq +/// cbindgen:derive-neq +#[repr(C)] +#[derive(Copy, Clone)] +pub struct FfiOptionVersionSetId { + pub is_some: bool, + pub value: VersionSetId, +} + +impl From> for FfiOptionVersionSetId { + fn from(opt: Option) -> Self { + match opt { + Some(v) => Self { + is_some: true, + value: v.into(), + }, + None => Self { + is_some: false, + value: VersionSetId { id: 0 }, + }, + } + } +} + +impl From for Option { + fn from(ffi: FfiOptionVersionSetId) -> Self { + if ffi.is_some { + Some(ffi.value.into()) + } else { + None + } + } +} + +impl From> for FfiOptionVersionSetId { + fn from(opt: Option) -> Self { + match opt { + Some(v) => Self { + is_some: true, + value: v.into(), + }, + None => Self { + is_some: false, + value: VersionSetId { id: 0 }, + }, + } + } +} + +impl From for Option { + fn from(ffi: FfiOptionVersionSetId) -> Self { + if ffi.is_some { + Some(ffi.value.into()) + } else { + None + } + } +} + /// Specifies a conditional requirement, where the requirement is only active when the condition is met. /// First VersionSetId is the condition, second is the requirement. /// cbindgen:derive-eq @@ -38,10 +98,28 @@ impl From for resolvo::SolvableId { #[repr(C)] #[derive(Copy, Clone)] pub struct ConditionalRequirement { - pub condition: Option, + pub condition: FfiOptionVersionSetId, pub requirement: Requirement, } +impl From for ConditionalRequirement { + fn from(value: resolvo::ConditionalRequirement) -> Self { + Self { + condition: value.condition.into(), + requirement: value.requirement.into(), + } + } +} + +impl From for resolvo::ConditionalRequirement { + fn from(value: ConditionalRequirement) -> Self { + Self { + condition: value.condition.into(), + requirement: value.requirement.into(), + } + } +} + /// Specifies the dependency of a solvable on a set of version sets. /// cbindgen:derive-eq /// cbindgen:derive-neq @@ -79,24 +157,6 @@ impl From for resolvo::Requirement { } } -impl From for ConditionalRequirement { - fn from(value: resolvo::ConditionalRequirement) -> Self { - Self { - condition: value.condition.map(|id| id.into()), - requirement: value.requirement.into(), - } - } -} - -impl From for resolvo::ConditionalRequirement { - fn from(value: ConditionalRequirement) -> Self { - Self { - condition: value.condition.map(|id| id.into()), - requirement: value.requirement.into(), - } - } -} - /// A unique identifier for a version set union. A version set union describes /// the union (logical OR) of a non-empty set of version sets belonging to /// more than one package. @@ -554,6 +614,28 @@ pub extern "C" fn resolvo_solve( } } +#[no_mangle] +#[allow(unused)] +pub extern "C" fn resolvo_conditional_requirement_single( + version_set_id: VersionSetId, +) -> ConditionalRequirement { + ConditionalRequirement { + condition: Option::::None.into(), + requirement: Requirement::Single(version_set_id), + } +} + +#[no_mangle] +#[allow(unused)] +pub extern "C" fn resolvo_conditional_requirement_union( + version_set_union_id: VersionSetUnionId, +) -> ConditionalRequirement { + ConditionalRequirement { + condition: Option::::None.into(), + requirement: Requirement::Union(version_set_union_id), + } +} + #[no_mangle] #[allow(unused)] pub extern "C" fn resolvo_requirement_single(version_set_id: VersionSetId) -> Requirement { diff --git a/cpp/tests/solve.cpp b/cpp/tests/solve.cpp index 1bb02b7..952e86e 100644 --- a/cpp/tests/solve.cpp +++ b/cpp/tests/solve.cpp @@ -48,16 +48,17 @@ struct PackageDatabase : public resolvo::DependencyProvider { /** * Allocates a new requirement for a single version set. */ - resolvo::Requirement alloc_requirement(std::string_view package, uint32_t version_start, - uint32_t version_end) { + resolvo::ConditionalRequirement alloc_requirement(std::string_view package, + uint32_t version_start, + uint32_t version_end) { auto id = alloc_version_set(package, version_start, version_end); - return resolvo::requirement_single(id); + return resolvo::conditional_requirement_single(id); } /** * Allocates a new requirement for a version set union. */ - resolvo::Requirement alloc_requirement_union( + resolvo::ConditionalRequirement alloc_requirement_union( std::initializer_list> version_sets) { std::vector version_set_union{version_sets.size()}; @@ -69,7 +70,7 @@ struct PackageDatabase : public resolvo::DependencyProvider { auto id = resolvo::VersionSetUnionId{static_cast(version_set_unions.size())}; version_set_unions.push_back(std::move(version_set_union)); - return resolvo::requirement_union(id); + return resolvo::conditional_requirement_union(id); } /** @@ -219,7 +220,8 @@ SCENARIO("Solve") { const auto d_1 = db.alloc_candidate("d", 1, {}); // Construct a problem to be solved by the solver - resolvo::Vector requirements = {db.alloc_requirement("a", 1, 3)}; + resolvo::Vector requirements = { + db.alloc_requirement("a", 1, 3)}; resolvo::Vector constraints = { db.alloc_version_set("b", 1, 3), db.alloc_version_set("c", 1, 3), @@ -263,7 +265,7 @@ SCENARIO("Solve Union") { "f", 1, {{db.alloc_requirement("b", 1, 10)}, {db.alloc_version_set("a", 10, 20)}}); // Construct a problem to be solved by the solver - resolvo::Vector requirements = { + resolvo::Vector requirements = { db.alloc_requirement_union({{"c", 1, 10}, {"d", 1, 10}}), db.alloc_requirement("e", 1, 10), db.alloc_requirement("f", 1, 10), diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 2369ebf..44b8cdc 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -791,17 +791,11 @@ mod test { clause.as_ref().unwrap().watched_literals[0].variable(), parent ); - assert_eq!( - clause.unwrap().watched_literals[1].variable(), - candidate1.into() - ); + assert_eq!(clause.unwrap().watched_literals[1].variable(), candidate1); // No conflict, still one candidate available decisions - .try_add_decision( - Decision::new(candidate1.into(), false, ClauseId::from_usize(0)), - 1, - ) + .try_add_decision(Decision::new(candidate1, false, ClauseId::from_usize(0)), 1) .unwrap(); let (clause, conflict, _kind) = WatchedLiterals::requires( parent, @@ -816,13 +810,13 @@ mod test { ); assert_eq!( clause.as_ref().unwrap().watched_literals[1].variable(), - candidate2.into() + candidate2 ); // Conflict, no candidates available decisions .try_add_decision( - Decision::new(candidate2.into(), false, ClauseId::install_root()), + Decision::new(candidate2, false, ClauseId::install_root()), 1, ) .unwrap(); @@ -839,7 +833,7 @@ mod test { ); assert_eq!( clause.as_ref().unwrap().watched_literals[1].variable(), - candidate1.into() + candidate1 ); // Panic diff --git a/tests/.solver.rs.pending-snap b/tests/.solver.rs.pending-snap new file mode 100644 index 0000000..237110b --- /dev/null +++ b/tests/.solver.rs.pending-snap @@ -0,0 +1,10 @@ +{"run_id":"1737574135-658533000","line":1475,"new":{"module_name":"solver","snapshot_name":"conditional_requirements-2","metadata":{"source":"tests/solver.rs","assertion_line":1475,"expression":"result"},"snapshot":""},"old":{"module_name":"solver","metadata":{},"snapshot":"b=1\nc=1"}} +{"run_id":"1737574174-973942000","line":1211,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":1428,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":923,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":1254,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":1306,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":885,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":906,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":943,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":1376,"new":null,"old":null} diff --git a/tests/solver.rs b/tests/solver.rs index 1376528..fab2da8 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -1432,6 +1432,77 @@ fn test_explicit_root_requirements() { "###); } +// #[test] +// fn test_conditional_requirements() { +// let provider = BundleBoxProvider::from_packages(&[ +// // Package a has a conditional requirement on c only if b is installed +// ("a", 1, vec!["b", "c"]), // Regular dependency +// ("a", 2, vec!["b"]), // Version 2 only requires b +// ("b", 1, vec![]), // Simple package b +// ("c", 1, vec![]), // Simple package c +// ]); + +// // First test: Basic dependency resolution +// let requirements = provider.requirements(&["a"]); +// let mut solver = Solver::new(provider); +// let problem = Problem::new().requirements(requirements); +// let solved = solver.solve(problem).unwrap(); +// let result = transaction_to_string(solver.provider(), &solved); +// insta::assert_snapshot!(result, @r###" +// a=2 +// b=1 +// "###); + +// // Now test with conditional requirement +// let provider = BundleBoxProvider::from_packages(&[ +// ("b", 1, vec![]), // Simple package b +// ("c", 1, vec![]), // Simple package c +// ]); + +// let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); +// let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + +// let b_version_set = provider.intern_version_set(&b_spec); +// let c_version_set = provider.intern_version_set(&c_spec); + +// let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); +// let requirements = vec![conditional_req]; + +// let mut solver = Solver::new(provider); +// let problem = Problem::new().requirements(requirements); +// let solved = solver.solve(problem).unwrap(); +// let result = transaction_to_string(solver.provider(), &solved); +// insta::assert_snapshot!(result, @r###" +// b=1 +// c=1 +// "###); +// } + +// #[test] +// fn test_conditional_requirements_not_met() { +// let provider = BundleBoxProvider::from_packages(&[ +// ("b", 2, vec![]), // Different version of b +// ("c", 1, vec![]), // Simple package c +// ]); + +// let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); // Condition requires b=1 +// let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + +// let b_version_set = provider.intern_version_set(&b_spec); +// let c_version_set = provider.intern_version_set(&c_spec); + +// let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); +// let requirements = vec![conditional_req]; + +// let mut solver = Solver::new(provider); +// let problem = Problem::new().requirements(requirements); +// let solved = solver.solve(problem).unwrap(); +// let result = transaction_to_string(solver.provider(), &solved); +// // Since b=1 is not available, c should not be installed +// insta::assert_snapshot!(result, @r###" +// "###); +// } + #[cfg(feature = "serde")] fn serialize_snapshot(snapshot: &DependencySnapshot, destination: impl AsRef) { let file = std::io::BufWriter::new(std::fs::File::create(destination.as_ref()).unwrap()); From 69042a1051df4d7b6deaa4f5c7f40d6051940ae5 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Wed, 22 Jan 2025 14:32:51 -0500 Subject: [PATCH 12/41] remove extra file --- cpp/_deps/corrosion-src | 1 - 1 file changed, 1 deletion(-) delete mode 160000 cpp/_deps/corrosion-src diff --git a/cpp/_deps/corrosion-src b/cpp/_deps/corrosion-src deleted file mode 160000 index ff5a236..0000000 --- a/cpp/_deps/corrosion-src +++ /dev/null @@ -1 +0,0 @@ -Subproject commit ff5a236550afc591f034e6cc5134ca9e5371bd98 From 220fed0af8cd0bec95eaa423dfb588e0379c1136 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Wed, 22 Jan 2025 14:34:57 -0500 Subject: [PATCH 13/41] lint fix --- cpp/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/lib.rs b/cpp/src/lib.rs index 6a1bda2..9051206 100644 --- a/cpp/src/lib.rs +++ b/cpp/src/lib.rs @@ -71,7 +71,7 @@ impl From> for FfiOptionVersionSetId { match opt { Some(v) => Self { is_some: true, - value: v.into(), + value: v, }, None => Self { is_some: false, @@ -84,7 +84,7 @@ impl From> for FfiOptionVersionSetId { impl From for Option { fn from(ffi: FfiOptionVersionSetId) -> Self { if ffi.is_some { - Some(ffi.value.into()) + Some(ffi.value) } else { None } From 0426ba3899f417c53d9454b2c3f2edd9b4ccdbfb Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Thu, 23 Jan 2025 23:26:33 -0500 Subject: [PATCH 14/41] minor fix --- src/solver/clause.rs | 8 +- src/solver/mod.rs | 34 +++ tests/.solver.rs.pending-snap | 10 - tests/solver.rs | 392 ++++++++++++++++++++++++++++------ 4 files changed, 360 insertions(+), 84 deletions(-) delete mode 100644 tests/.solver.rs.pending-snap diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 44b8cdc..0304fa2 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -265,10 +265,12 @@ impl Clause { false, ) } else { - // No valid requirement candidate, use first condition candidate and mark conflict + // No valid requirement candidate and condition is true + // This is an immediate conflict - we can't satisfy the requirement + // We need to watch the condition candidate to detect when it becomes true ( Clause::Conditional(parent_id, condition, requirement), - Some([parent_id.negative(), condition_first_candidate.positive()]), + Some([parent_id.negative(), condition_first_candidate.negative()]), true, ) } @@ -284,7 +286,7 @@ impl Clause { false, ) } else { - // All conditions false + // All conditions false, no conflict ( Clause::Conditional(parent_id, condition, requirement), None, diff --git a/src/solver/mod.rs b/src/solver/mod.rs index 7700f96..d3836cb 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -1155,6 +1155,40 @@ impl Solver { self.decide_assertions(level)?; self.decide_learned(level)?; + // Check for conflicts in conditional clauses + for ((solvable_id, condition), clauses) in &self.conditional_clauses { + // Only check clauses where the parent is true + if self.decision_tracker.assigned_value(*solvable_id) != Some(true) { + continue; + } + + // Check if the condition is true + let condition_requirement: Requirement = (*condition).into(); + let conditional_candidates = &self.requirement_to_sorted_candidates[&condition_requirement]; + let condition_is_true = conditional_candidates.iter().any(|candidates| { + candidates.iter().any(|&candidate| { + self.decision_tracker.assigned_value(candidate) == Some(true) + }) + }); + + if condition_is_true { + // For each clause, check if all requirement candidates are false + for (requirement, clause_id) in clauses { + let requirement_candidates = &self.requirement_to_sorted_candidates[requirement]; + let all_candidates_false = requirement_candidates.iter().all(|candidates| { + candidates.iter().all(|&candidate| { + self.decision_tracker.assigned_value(candidate) == Some(false) + }) + }); + + if all_candidates_false { + // This is a conflict - we have a true condition but can't satisfy the requirement + return Err(PropagationError::Conflict(*solvable_id, true, *clause_id)); + } + } + } + } + // For each decision that has not been propagated yet, we propagate the // decision. // diff --git a/tests/.solver.rs.pending-snap b/tests/.solver.rs.pending-snap deleted file mode 100644 index 237110b..0000000 --- a/tests/.solver.rs.pending-snap +++ /dev/null @@ -1,10 +0,0 @@ -{"run_id":"1737574135-658533000","line":1475,"new":{"module_name":"solver","snapshot_name":"conditional_requirements-2","metadata":{"source":"tests/solver.rs","assertion_line":1475,"expression":"result"},"snapshot":""},"old":{"module_name":"solver","metadata":{},"snapshot":"b=1\nc=1"}} -{"run_id":"1737574174-973942000","line":1211,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":1428,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":923,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":1254,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":1306,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":885,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":906,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":943,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":1376,"new":null,"old":null} diff --git a/tests/solver.rs b/tests/solver.rs index fab2da8..aa9b3f1 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -292,6 +292,58 @@ impl BundleBoxProvider { ); } + // pub fn add_package_with_conditional_deps( + // &mut self, + // name: impl Into, + // pack: Pack, + // deps: &[&str], + // conditional_deps: &[(impl AsRef, &[&str])], + // constrains: &[&str], + // ) { + // let name = name.into(); + // let mut dependencies = BundleBoxPackageDependencies { + // dependencies: Vec::new(), + // constrains: Vec::new(), + // }; + + // // Add regular dependencies + // for dep in deps { + // let specs = Spec::parse_union(dep) + // .map(|r| r.expect("invalid dependency spec")) + // .collect(); + // dependencies.dependencies.push(specs); + // } + + // // Add conditional dependencies + // for (condition, deps) in conditional_deps { + // let condition_spec = Spec::parse_union(condition.as_ref()) + // .next() + // .expect("invalid condition spec") + // .expect("invalid condition spec"); + + // for dep in *deps { + // let specs = Spec::parse_union(dep) + // .map(|r| r.expect("invalid dependency spec")) + // .collect(); + // dependencies.dependencies.push(specs); + // } + // } + + // // Add constraints + // for constrain in constrains { + // let spec = Spec::parse_union(constrain) + // .next() + // .expect("invalid constrain spec") + // .expect("invalid constrain spec"); + // dependencies.constrains.push(spec); + // } + + // self.packages + // .entry(name) + // .or_default() + // .insert(pack, dependencies); + // } + // Sends a value from the dependency provider to the solver, introducing a // minimal delay to force concurrency to be used (unless there is no async // runtime available) @@ -1432,76 +1484,274 @@ fn test_explicit_root_requirements() { "###); } -// #[test] -// fn test_conditional_requirements() { -// let provider = BundleBoxProvider::from_packages(&[ -// // Package a has a conditional requirement on c only if b is installed -// ("a", 1, vec!["b", "c"]), // Regular dependency -// ("a", 2, vec!["b"]), // Version 2 only requires b -// ("b", 1, vec![]), // Simple package b -// ("c", 1, vec![]), // Simple package c -// ]); - -// // First test: Basic dependency resolution -// let requirements = provider.requirements(&["a"]); -// let mut solver = Solver::new(provider); -// let problem = Problem::new().requirements(requirements); -// let solved = solver.solve(problem).unwrap(); -// let result = transaction_to_string(solver.provider(), &solved); -// insta::assert_snapshot!(result, @r###" -// a=2 -// b=1 -// "###); - -// // Now test with conditional requirement -// let provider = BundleBoxProvider::from_packages(&[ -// ("b", 1, vec![]), // Simple package b -// ("c", 1, vec![]), // Simple package c -// ]); - -// let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); -// let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - -// let b_version_set = provider.intern_version_set(&b_spec); -// let c_version_set = provider.intern_version_set(&c_spec); - -// let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); -// let requirements = vec![conditional_req]; - -// let mut solver = Solver::new(provider); -// let problem = Problem::new().requirements(requirements); -// let solved = solver.solve(problem).unwrap(); -// let result = transaction_to_string(solver.provider(), &solved); -// insta::assert_snapshot!(result, @r###" -// b=1 -// c=1 -// "###); -// } - -// #[test] -// fn test_conditional_requirements_not_met() { -// let provider = BundleBoxProvider::from_packages(&[ -// ("b", 2, vec![]), // Different version of b -// ("c", 1, vec![]), // Simple package c -// ]); - -// let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); // Condition requires b=1 -// let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - -// let b_version_set = provider.intern_version_set(&b_spec); -// let c_version_set = provider.intern_version_set(&c_spec); - -// let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); -// let requirements = vec![conditional_req]; - -// let mut solver = Solver::new(provider); -// let problem = Problem::new().requirements(requirements); -// let solved = solver.solve(problem).unwrap(); -// let result = transaction_to_string(solver.provider(), &solved); -// // Since b=1 is not available, c should not be installed -// insta::assert_snapshot!(result, @r###" -// "###); -// } +#[test] +fn test_conditional_requirements() { + let mut provider = BundleBoxProvider::new(); + + // Add packages + provider.add_package("a", 1.into(), &["b"], &[]); // a depends on b + provider.add_package("b", 1.into(), &[], &[]); // Simple package b + provider.add_package("c", 1.into(), &[], &[]); // Simple package c + + // Create conditional requirement: if b=1 is installed, require c + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + + let b_version_set = provider.intern_version_set(&b_spec); + let c_version_set = provider.intern_version_set(&c_spec); + + let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); + + // Create problem with both regular and conditional requirements + let mut requirements = provider.requirements(&["a"]); + requirements.push(conditional_req); + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + insta::assert_snapshot!(result, @r###" + a=1 + b=1 + c=1 + "###); +} + +#[test] +fn test_conditional_requirements_not_met() { + let mut provider = BundleBoxProvider::new(); + provider.add_package("b", 1.into(), &[], &[]); // Add b=1 as a candidate + provider.add_package("b", 2.into(), &[], &[]); // Different version of b + provider.add_package("c", 1.into(), &[], &[]); // Simple package c + + // Create conditional requirement: if b=1 is installed, require c + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + + let b_version_set = provider.intern_version_set(&b_spec); + let c_version_set = provider.intern_version_set(&c_spec); + + let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); + + // Create problem with just the conditional requirement + let mut requirements = vec![conditional_req]; + + // Add a requirement for b=2 to ensure we get a version that doesn't trigger the condition + let b2_spec = Spec::parse_union("b 2").next().unwrap().unwrap(); + let b2_version_set = provider.intern_version_set(&b2_spec); + requirements.push(b2_version_set.into()); + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // Since b=1 is not installed (b=2 is), c should not be installed + insta::assert_snapshot!(result, @r###" + b=2 + "###); +} + +#[test] +fn test_nested_conditional_dependencies() { + let mut provider = BundleBoxProvider::new(); + + // Setup packages + provider.add_package("a", 1.into(), &[], &[]); // Base package + provider.add_package("b", 1.into(), &[], &[]); // First level conditional + provider.add_package("c", 1.into(), &[], &[]); // Second level conditional + provider.add_package("d", 1.into(), &[], &[]); // Third level conditional + + // Create nested conditional requirements: + // If a is installed, require b + // If b is installed, require c + // If c is installed, require d + let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + let d_spec = Spec::parse_union("d 1").next().unwrap().unwrap(); + + let a_version_set = provider.intern_version_set(&a_spec); + let b_version_set = provider.intern_version_set(&b_spec); + let c_version_set = provider.intern_version_set(&c_spec); + let d_version_set = provider.intern_version_set(&d_spec); + + let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); + let cond_req2 = ConditionalRequirement::new(b_version_set, c_version_set.into()); + let cond_req3 = ConditionalRequirement::new(c_version_set, d_version_set.into()); + + let requirements = vec![ + cond_req1, + cond_req2, + cond_req3, + a_version_set.into(), // Require package a + ]; + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // All packages should be installed due to chain reaction + insta::assert_snapshot!(result, @r###" + a=1 + b=1 + c=1 + d=1 + "###); +} + +#[test] +fn test_multiple_conditions_same_package() { + let mut provider = BundleBoxProvider::new(); + + // Setup packages + provider.add_package("a", 1.into(), &[], &[]); + provider.add_package("b", 1.into(), &[], &[]); + provider.add_package("c", 1.into(), &[], &[]); + provider.add_package("target", 1.into(), &[], &[]); + + // Create multiple conditions that all require the same package + let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + let target_spec = Spec::parse_union("target 1").next().unwrap().unwrap(); + + let a_version_set = provider.intern_version_set(&a_spec); + let b_version_set = provider.intern_version_set(&b_spec); + let c_version_set = provider.intern_version_set(&c_spec); + let target_version_set = provider.intern_version_set(&target_spec); + + // If any of a, b, or c is installed, require target + let cond_req1 = ConditionalRequirement::new(a_version_set, target_version_set.into()); + let cond_req2 = ConditionalRequirement::new(b_version_set, target_version_set.into()); + let cond_req3 = ConditionalRequirement::new(c_version_set, target_version_set.into()); + + let requirements = vec![ + cond_req1, + cond_req2, + cond_req3, + b_version_set.into(), // Only require package b + ]; + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // b and target should be installed + insta::assert_snapshot!(result, @r###" + b=1 + target=1 + "###); +} + +#[test] +fn test_circular_conditional_dependencies() { + let mut provider = BundleBoxProvider::new(); + + // Setup packages + provider.add_package("a", 1.into(), &[], &[]); + provider.add_package("b", 1.into(), &[], &[]); + + // Create circular conditional dependencies: + // If a is installed, require b + // If b is installed, require a + let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + + let a_version_set = provider.intern_version_set(&a_spec); + let b_version_set = provider.intern_version_set(&b_spec); + + let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); + let cond_req2 = ConditionalRequirement::new(b_version_set, a_version_set.into()); + + let requirements = vec![ + cond_req1, + cond_req2, + a_version_set.into(), // Require package a + ]; + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // Both packages should be installed due to circular dependency + insta::assert_snapshot!(result, @r###" + a=1 + b=1 + "###); +} + +#[test] +#[traced_test] +fn test_conflicting_conditional_dependencies() { + let mut provider = BundleBoxProvider::new(); + + // Setup packages + provider.add_package("a", 1.into(), &[], &[]); // Base package + provider.add_package("b", 1.into(), &[], &[]); // First level conditional + provider.add_package("c", 1.into(), &[], &[]); // Second level conditional that will conflict + + // Create conditional requirements: + // If a is installed, require b + // If b is installed, require c + // If c is installed, require !b (this creates a conflict) + let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + let not_b_spec = Spec::parse_union("!b 1").next().unwrap().unwrap(); + + let a_version_set = provider.intern_version_set(&a_spec); + let b_version_set = provider.intern_version_set(&b_spec); + let c_version_set = provider.intern_version_set(&c_spec); + let not_b_version_set = provider.intern_version_set(¬_b_spec); + + let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); + let cond_req2 = ConditionalRequirement::new(b_version_set, c_version_set.into()); + let cond_req3 = ConditionalRequirement::new(c_version_set, not_b_version_set.into()); + + let requirements = vec![ + cond_req1, + cond_req2, + cond_req3, + a_version_set.into(), // Require package a + ]; + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let result = solver.solve(problem); + assert!(result.is_err(), "Expected solver to fail due to conflicting conditional dependencies"); +} + +#[test] +fn test_conditional_dependency_with_excluded() { + let mut provider = BundleBoxProvider::new(); + + // Setup packages + provider.add_package("trigger", 1.into(), &[], &[]); + provider.add_package("target", 1.into(), &[], &[]); + + // Create conditional requirement: + // If trigger is installed, require target=1 + let trigger_spec = Spec::parse_union("trigger 1").next().unwrap().unwrap(); + let target_spec = Spec::parse_union("target 1").next().unwrap().unwrap(); + + let trigger_version_set = provider.intern_version_set(&trigger_spec); + let target_version_set = provider.intern_version_set(&target_spec); + + let cond_req = ConditionalRequirement::new(trigger_version_set, target_version_set.into()); + + // Exclude target package + provider.exclude("target", 1, "it is externally excluded"); + + let requirements = vec![ + cond_req, + trigger_version_set.into(), // Require trigger package + ]; + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + // Should fail to solve because target is excluded but required by condition + assert!(solver.solve(problem).is_err()); +} #[cfg(feature = "serde")] fn serialize_snapshot(snapshot: &DependencySnapshot, destination: impl AsRef) { @@ -1511,7 +1761,7 @@ fn serialize_snapshot(snapshot: &DependencySnapshot, destination: impl AsRef String { let mut solver = Solver::new(provider); From b95b7896ec5abba2f8624be2f382ce6943b9c549 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Thu, 23 Jan 2025 23:27:23 -0500 Subject: [PATCH 15/41] Revert "minor fix" This reverts commit 0426ba3899f417c53d9454b2c3f2edd9b4ccdbfb. --- src/solver/clause.rs | 8 +- src/solver/mod.rs | 34 --- tests/.solver.rs.pending-snap | 10 + tests/solver.rs | 392 ++++++---------------------------- 4 files changed, 84 insertions(+), 360 deletions(-) create mode 100644 tests/.solver.rs.pending-snap diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 0304fa2..44b8cdc 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -265,12 +265,10 @@ impl Clause { false, ) } else { - // No valid requirement candidate and condition is true - // This is an immediate conflict - we can't satisfy the requirement - // We need to watch the condition candidate to detect when it becomes true + // No valid requirement candidate, use first condition candidate and mark conflict ( Clause::Conditional(parent_id, condition, requirement), - Some([parent_id.negative(), condition_first_candidate.negative()]), + Some([parent_id.negative(), condition_first_candidate.positive()]), true, ) } @@ -286,7 +284,7 @@ impl Clause { false, ) } else { - // All conditions false, no conflict + // All conditions false ( Clause::Conditional(parent_id, condition, requirement), None, diff --git a/src/solver/mod.rs b/src/solver/mod.rs index d3836cb..7700f96 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -1155,40 +1155,6 @@ impl Solver { self.decide_assertions(level)?; self.decide_learned(level)?; - // Check for conflicts in conditional clauses - for ((solvable_id, condition), clauses) in &self.conditional_clauses { - // Only check clauses where the parent is true - if self.decision_tracker.assigned_value(*solvable_id) != Some(true) { - continue; - } - - // Check if the condition is true - let condition_requirement: Requirement = (*condition).into(); - let conditional_candidates = &self.requirement_to_sorted_candidates[&condition_requirement]; - let condition_is_true = conditional_candidates.iter().any(|candidates| { - candidates.iter().any(|&candidate| { - self.decision_tracker.assigned_value(candidate) == Some(true) - }) - }); - - if condition_is_true { - // For each clause, check if all requirement candidates are false - for (requirement, clause_id) in clauses { - let requirement_candidates = &self.requirement_to_sorted_candidates[requirement]; - let all_candidates_false = requirement_candidates.iter().all(|candidates| { - candidates.iter().all(|&candidate| { - self.decision_tracker.assigned_value(candidate) == Some(false) - }) - }); - - if all_candidates_false { - // This is a conflict - we have a true condition but can't satisfy the requirement - return Err(PropagationError::Conflict(*solvable_id, true, *clause_id)); - } - } - } - } - // For each decision that has not been propagated yet, we propagate the // decision. // diff --git a/tests/.solver.rs.pending-snap b/tests/.solver.rs.pending-snap new file mode 100644 index 0000000..237110b --- /dev/null +++ b/tests/.solver.rs.pending-snap @@ -0,0 +1,10 @@ +{"run_id":"1737574135-658533000","line":1475,"new":{"module_name":"solver","snapshot_name":"conditional_requirements-2","metadata":{"source":"tests/solver.rs","assertion_line":1475,"expression":"result"},"snapshot":""},"old":{"module_name":"solver","metadata":{},"snapshot":"b=1\nc=1"}} +{"run_id":"1737574174-973942000","line":1211,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":1428,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":923,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":1254,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":1306,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":885,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":906,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":943,"new":null,"old":null} +{"run_id":"1737574174-973942000","line":1376,"new":null,"old":null} diff --git a/tests/solver.rs b/tests/solver.rs index aa9b3f1..fab2da8 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -292,58 +292,6 @@ impl BundleBoxProvider { ); } - // pub fn add_package_with_conditional_deps( - // &mut self, - // name: impl Into, - // pack: Pack, - // deps: &[&str], - // conditional_deps: &[(impl AsRef, &[&str])], - // constrains: &[&str], - // ) { - // let name = name.into(); - // let mut dependencies = BundleBoxPackageDependencies { - // dependencies: Vec::new(), - // constrains: Vec::new(), - // }; - - // // Add regular dependencies - // for dep in deps { - // let specs = Spec::parse_union(dep) - // .map(|r| r.expect("invalid dependency spec")) - // .collect(); - // dependencies.dependencies.push(specs); - // } - - // // Add conditional dependencies - // for (condition, deps) in conditional_deps { - // let condition_spec = Spec::parse_union(condition.as_ref()) - // .next() - // .expect("invalid condition spec") - // .expect("invalid condition spec"); - - // for dep in *deps { - // let specs = Spec::parse_union(dep) - // .map(|r| r.expect("invalid dependency spec")) - // .collect(); - // dependencies.dependencies.push(specs); - // } - // } - - // // Add constraints - // for constrain in constrains { - // let spec = Spec::parse_union(constrain) - // .next() - // .expect("invalid constrain spec") - // .expect("invalid constrain spec"); - // dependencies.constrains.push(spec); - // } - - // self.packages - // .entry(name) - // .or_default() - // .insert(pack, dependencies); - // } - // Sends a value from the dependency provider to the solver, introducing a // minimal delay to force concurrency to be used (unless there is no async // runtime available) @@ -1484,274 +1432,76 @@ fn test_explicit_root_requirements() { "###); } -#[test] -fn test_conditional_requirements() { - let mut provider = BundleBoxProvider::new(); - - // Add packages - provider.add_package("a", 1.into(), &["b"], &[]); // a depends on b - provider.add_package("b", 1.into(), &[], &[]); // Simple package b - provider.add_package("c", 1.into(), &[], &[]); // Simple package c - - // Create conditional requirement: if b=1 is installed, require c - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - - let b_version_set = provider.intern_version_set(&b_spec); - let c_version_set = provider.intern_version_set(&c_spec); - - let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); - - // Create problem with both regular and conditional requirements - let mut requirements = provider.requirements(&["a"]); - requirements.push(conditional_req); - - let mut solver = Solver::new(provider); - let problem = Problem::new().requirements(requirements); - let solved = solver.solve(problem).unwrap(); - let result = transaction_to_string(solver.provider(), &solved); - insta::assert_snapshot!(result, @r###" - a=1 - b=1 - c=1 - "###); -} - -#[test] -fn test_conditional_requirements_not_met() { - let mut provider = BundleBoxProvider::new(); - provider.add_package("b", 1.into(), &[], &[]); // Add b=1 as a candidate - provider.add_package("b", 2.into(), &[], &[]); // Different version of b - provider.add_package("c", 1.into(), &[], &[]); // Simple package c - - // Create conditional requirement: if b=1 is installed, require c - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - - let b_version_set = provider.intern_version_set(&b_spec); - let c_version_set = provider.intern_version_set(&c_spec); - - let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); - - // Create problem with just the conditional requirement - let mut requirements = vec![conditional_req]; - - // Add a requirement for b=2 to ensure we get a version that doesn't trigger the condition - let b2_spec = Spec::parse_union("b 2").next().unwrap().unwrap(); - let b2_version_set = provider.intern_version_set(&b2_spec); - requirements.push(b2_version_set.into()); - - let mut solver = Solver::new(provider); - let problem = Problem::new().requirements(requirements); - let solved = solver.solve(problem).unwrap(); - let result = transaction_to_string(solver.provider(), &solved); - // Since b=1 is not installed (b=2 is), c should not be installed - insta::assert_snapshot!(result, @r###" - b=2 - "###); -} - -#[test] -fn test_nested_conditional_dependencies() { - let mut provider = BundleBoxProvider::new(); - - // Setup packages - provider.add_package("a", 1.into(), &[], &[]); // Base package - provider.add_package("b", 1.into(), &[], &[]); // First level conditional - provider.add_package("c", 1.into(), &[], &[]); // Second level conditional - provider.add_package("d", 1.into(), &[], &[]); // Third level conditional - - // Create nested conditional requirements: - // If a is installed, require b - // If b is installed, require c - // If c is installed, require d - let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - let d_spec = Spec::parse_union("d 1").next().unwrap().unwrap(); - - let a_version_set = provider.intern_version_set(&a_spec); - let b_version_set = provider.intern_version_set(&b_spec); - let c_version_set = provider.intern_version_set(&c_spec); - let d_version_set = provider.intern_version_set(&d_spec); - - let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); - let cond_req2 = ConditionalRequirement::new(b_version_set, c_version_set.into()); - let cond_req3 = ConditionalRequirement::new(c_version_set, d_version_set.into()); - - let requirements = vec![ - cond_req1, - cond_req2, - cond_req3, - a_version_set.into(), // Require package a - ]; - - let mut solver = Solver::new(provider); - let problem = Problem::new().requirements(requirements); - let solved = solver.solve(problem).unwrap(); - let result = transaction_to_string(solver.provider(), &solved); - // All packages should be installed due to chain reaction - insta::assert_snapshot!(result, @r###" - a=1 - b=1 - c=1 - d=1 - "###); -} - -#[test] -fn test_multiple_conditions_same_package() { - let mut provider = BundleBoxProvider::new(); - - // Setup packages - provider.add_package("a", 1.into(), &[], &[]); - provider.add_package("b", 1.into(), &[], &[]); - provider.add_package("c", 1.into(), &[], &[]); - provider.add_package("target", 1.into(), &[], &[]); - - // Create multiple conditions that all require the same package - let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - let target_spec = Spec::parse_union("target 1").next().unwrap().unwrap(); - - let a_version_set = provider.intern_version_set(&a_spec); - let b_version_set = provider.intern_version_set(&b_spec); - let c_version_set = provider.intern_version_set(&c_spec); - let target_version_set = provider.intern_version_set(&target_spec); - - // If any of a, b, or c is installed, require target - let cond_req1 = ConditionalRequirement::new(a_version_set, target_version_set.into()); - let cond_req2 = ConditionalRequirement::new(b_version_set, target_version_set.into()); - let cond_req3 = ConditionalRequirement::new(c_version_set, target_version_set.into()); - - let requirements = vec![ - cond_req1, - cond_req2, - cond_req3, - b_version_set.into(), // Only require package b - ]; - - let mut solver = Solver::new(provider); - let problem = Problem::new().requirements(requirements); - let solved = solver.solve(problem).unwrap(); - let result = transaction_to_string(solver.provider(), &solved); - // b and target should be installed - insta::assert_snapshot!(result, @r###" - b=1 - target=1 - "###); -} - -#[test] -fn test_circular_conditional_dependencies() { - let mut provider = BundleBoxProvider::new(); - - // Setup packages - provider.add_package("a", 1.into(), &[], &[]); - provider.add_package("b", 1.into(), &[], &[]); - - // Create circular conditional dependencies: - // If a is installed, require b - // If b is installed, require a - let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - - let a_version_set = provider.intern_version_set(&a_spec); - let b_version_set = provider.intern_version_set(&b_spec); - - let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); - let cond_req2 = ConditionalRequirement::new(b_version_set, a_version_set.into()); - - let requirements = vec![ - cond_req1, - cond_req2, - a_version_set.into(), // Require package a - ]; - - let mut solver = Solver::new(provider); - let problem = Problem::new().requirements(requirements); - let solved = solver.solve(problem).unwrap(); - let result = transaction_to_string(solver.provider(), &solved); - // Both packages should be installed due to circular dependency - insta::assert_snapshot!(result, @r###" - a=1 - b=1 - "###); -} - -#[test] -#[traced_test] -fn test_conflicting_conditional_dependencies() { - let mut provider = BundleBoxProvider::new(); - - // Setup packages - provider.add_package("a", 1.into(), &[], &[]); // Base package - provider.add_package("b", 1.into(), &[], &[]); // First level conditional - provider.add_package("c", 1.into(), &[], &[]); // Second level conditional that will conflict - - // Create conditional requirements: - // If a is installed, require b - // If b is installed, require c - // If c is installed, require !b (this creates a conflict) - let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - let not_b_spec = Spec::parse_union("!b 1").next().unwrap().unwrap(); - - let a_version_set = provider.intern_version_set(&a_spec); - let b_version_set = provider.intern_version_set(&b_spec); - let c_version_set = provider.intern_version_set(&c_spec); - let not_b_version_set = provider.intern_version_set(¬_b_spec); - - let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); - let cond_req2 = ConditionalRequirement::new(b_version_set, c_version_set.into()); - let cond_req3 = ConditionalRequirement::new(c_version_set, not_b_version_set.into()); - - let requirements = vec![ - cond_req1, - cond_req2, - cond_req3, - a_version_set.into(), // Require package a - ]; - - let mut solver = Solver::new(provider); - let problem = Problem::new().requirements(requirements); - let result = solver.solve(problem); - assert!(result.is_err(), "Expected solver to fail due to conflicting conditional dependencies"); -} - -#[test] -fn test_conditional_dependency_with_excluded() { - let mut provider = BundleBoxProvider::new(); - - // Setup packages - provider.add_package("trigger", 1.into(), &[], &[]); - provider.add_package("target", 1.into(), &[], &[]); - - // Create conditional requirement: - // If trigger is installed, require target=1 - let trigger_spec = Spec::parse_union("trigger 1").next().unwrap().unwrap(); - let target_spec = Spec::parse_union("target 1").next().unwrap().unwrap(); - - let trigger_version_set = provider.intern_version_set(&trigger_spec); - let target_version_set = provider.intern_version_set(&target_spec); - - let cond_req = ConditionalRequirement::new(trigger_version_set, target_version_set.into()); - - // Exclude target package - provider.exclude("target", 1, "it is externally excluded"); - - let requirements = vec![ - cond_req, - trigger_version_set.into(), // Require trigger package - ]; - - let mut solver = Solver::new(provider); - let problem = Problem::new().requirements(requirements); - // Should fail to solve because target is excluded but required by condition - assert!(solver.solve(problem).is_err()); -} +// #[test] +// fn test_conditional_requirements() { +// let provider = BundleBoxProvider::from_packages(&[ +// // Package a has a conditional requirement on c only if b is installed +// ("a", 1, vec!["b", "c"]), // Regular dependency +// ("a", 2, vec!["b"]), // Version 2 only requires b +// ("b", 1, vec![]), // Simple package b +// ("c", 1, vec![]), // Simple package c +// ]); + +// // First test: Basic dependency resolution +// let requirements = provider.requirements(&["a"]); +// let mut solver = Solver::new(provider); +// let problem = Problem::new().requirements(requirements); +// let solved = solver.solve(problem).unwrap(); +// let result = transaction_to_string(solver.provider(), &solved); +// insta::assert_snapshot!(result, @r###" +// a=2 +// b=1 +// "###); + +// // Now test with conditional requirement +// let provider = BundleBoxProvider::from_packages(&[ +// ("b", 1, vec![]), // Simple package b +// ("c", 1, vec![]), // Simple package c +// ]); + +// let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); +// let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + +// let b_version_set = provider.intern_version_set(&b_spec); +// let c_version_set = provider.intern_version_set(&c_spec); + +// let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); +// let requirements = vec![conditional_req]; + +// let mut solver = Solver::new(provider); +// let problem = Problem::new().requirements(requirements); +// let solved = solver.solve(problem).unwrap(); +// let result = transaction_to_string(solver.provider(), &solved); +// insta::assert_snapshot!(result, @r###" +// b=1 +// c=1 +// "###); +// } + +// #[test] +// fn test_conditional_requirements_not_met() { +// let provider = BundleBoxProvider::from_packages(&[ +// ("b", 2, vec![]), // Different version of b +// ("c", 1, vec![]), // Simple package c +// ]); + +// let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); // Condition requires b=1 +// let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + +// let b_version_set = provider.intern_version_set(&b_spec); +// let c_version_set = provider.intern_version_set(&c_spec); + +// let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); +// let requirements = vec![conditional_req]; + +// let mut solver = Solver::new(provider); +// let problem = Problem::new().requirements(requirements); +// let solved = solver.solve(problem).unwrap(); +// let result = transaction_to_string(solver.provider(), &solved); +// // Since b=1 is not available, c should not be installed +// insta::assert_snapshot!(result, @r###" +// "###); +// } #[cfg(feature = "serde")] fn serialize_snapshot(snapshot: &DependencySnapshot, destination: impl AsRef) { @@ -1761,7 +1511,7 @@ fn serialize_snapshot(snapshot: &DependencySnapshot, destination: impl AsRef String { let mut solver = Solver::new(provider); From a63278aa5c6ece8ce2c7ecebe171a6abf067cdf4 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Thu, 23 Jan 2025 23:29:35 -0500 Subject: [PATCH 16/41] minor fix --- src/solver/clause.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 44b8cdc..bfa7438 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -268,7 +268,7 @@ impl Clause { // No valid requirement candidate, use first condition candidate and mark conflict ( Clause::Conditional(parent_id, condition, requirement), - Some([parent_id.negative(), condition_first_candidate.positive()]), + Some([parent_id.negative(), condition_first_candidate.negative()]), true, ) } From 190cd8a06055040f3b31df47317d7652f0b343a5 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 24 Jan 2025 00:01:40 -0500 Subject: [PATCH 17/41] update tests in `tests/solver.rs` --- tests/solver.rs | 339 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 269 insertions(+), 70 deletions(-) diff --git a/tests/solver.rs b/tests/solver.rs index fab2da8..2ea130c 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -1432,76 +1432,275 @@ fn test_explicit_root_requirements() { "###); } -// #[test] -// fn test_conditional_requirements() { -// let provider = BundleBoxProvider::from_packages(&[ -// // Package a has a conditional requirement on c only if b is installed -// ("a", 1, vec!["b", "c"]), // Regular dependency -// ("a", 2, vec!["b"]), // Version 2 only requires b -// ("b", 1, vec![]), // Simple package b -// ("c", 1, vec![]), // Simple package c -// ]); - -// // First test: Basic dependency resolution -// let requirements = provider.requirements(&["a"]); -// let mut solver = Solver::new(provider); -// let problem = Problem::new().requirements(requirements); -// let solved = solver.solve(problem).unwrap(); -// let result = transaction_to_string(solver.provider(), &solved); -// insta::assert_snapshot!(result, @r###" -// a=2 -// b=1 -// "###); - -// // Now test with conditional requirement -// let provider = BundleBoxProvider::from_packages(&[ -// ("b", 1, vec![]), // Simple package b -// ("c", 1, vec![]), // Simple package c -// ]); - -// let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); -// let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - -// let b_version_set = provider.intern_version_set(&b_spec); -// let c_version_set = provider.intern_version_set(&c_spec); - -// let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); -// let requirements = vec![conditional_req]; - -// let mut solver = Solver::new(provider); -// let problem = Problem::new().requirements(requirements); -// let solved = solver.solve(problem).unwrap(); -// let result = transaction_to_string(solver.provider(), &solved); -// insta::assert_snapshot!(result, @r###" -// b=1 -// c=1 -// "###); -// } - -// #[test] -// fn test_conditional_requirements_not_met() { -// let provider = BundleBoxProvider::from_packages(&[ -// ("b", 2, vec![]), // Different version of b -// ("c", 1, vec![]), // Simple package c -// ]); - -// let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); // Condition requires b=1 -// let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - -// let b_version_set = provider.intern_version_set(&b_spec); -// let c_version_set = provider.intern_version_set(&c_spec); - -// let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); -// let requirements = vec![conditional_req]; - -// let mut solver = Solver::new(provider); -// let problem = Problem::new().requirements(requirements); -// let solved = solver.solve(problem).unwrap(); -// let result = transaction_to_string(solver.provider(), &solved); -// // Since b=1 is not available, c should not be installed -// insta::assert_snapshot!(result, @r###" -// "###); -// } +#[test] +#[traced_test] +fn test_conditional_requirements() { + let mut provider = BundleBoxProvider::new(); + + // Add packages + provider.add_package("a", 1.into(), &["b"], &[]); // a depends on b + provider.add_package("b", 1.into(), &[], &[]); // Simple package b + provider.add_package("c", 1.into(), &[], &[]); // Simple package c + + // Create conditional requirement: if b=1 is installed, require c + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + + let b_version_set = provider.intern_version_set(&b_spec); + let c_version_set = provider.intern_version_set(&c_spec); + + let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); + + // Create problem with both regular and conditional requirements + let mut requirements = provider.requirements(&["a"]); + requirements.push(conditional_req); + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + insta::assert_snapshot!(result, @r###" + a=1 + b=1 + c=1 + "###); +} + +#[test] +fn test_conditional_requirements_not_met() { + let mut provider = BundleBoxProvider::new(); + provider.add_package("b", 1.into(), &[], &[]); // Add b=1 as a candidate + provider.add_package("b", 2.into(), &[], &[]); // Different version of b + provider.add_package("c", 1.into(), &[], &[]); // Simple package c + + // Create conditional requirement: if b=1 is installed, require c + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + + let b_version_set = provider.intern_version_set(&b_spec); + let c_version_set = provider.intern_version_set(&c_spec); + + let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); + + // Create problem with just the conditional requirement + let mut requirements = vec![conditional_req]; + + // Add a requirement for b=2 to ensure we get a version that doesn't trigger the condition + let b2_spec = Spec::parse_union("b 2").next().unwrap().unwrap(); + let b2_version_set = provider.intern_version_set(&b2_spec); + requirements.push(b2_version_set.into()); + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // Since b=1 is not installed (b=2 is), c should not be installed + insta::assert_snapshot!(result, @r###" + b=2 + "###); +} + +#[test] +fn test_nested_conditional_dependencies() { + let mut provider = BundleBoxProvider::new(); + + // Setup packages + provider.add_package("a", 1.into(), &[], &[]); // Base package + provider.add_package("b", 1.into(), &[], &[]); // First level conditional + provider.add_package("c", 1.into(), &[], &[]); // Second level conditional + provider.add_package("d", 1.into(), &[], &[]); // Third level conditional + + // Create nested conditional requirements: + // If a is installed, require b + // If b is installed, require c + // If c is installed, require d + let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + let d_spec = Spec::parse_union("d 1").next().unwrap().unwrap(); + + let a_version_set = provider.intern_version_set(&a_spec); + let b_version_set = provider.intern_version_set(&b_spec); + let c_version_set = provider.intern_version_set(&c_spec); + let d_version_set = provider.intern_version_set(&d_spec); + + let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); + let cond_req2 = ConditionalRequirement::new(b_version_set, c_version_set.into()); + let cond_req3 = ConditionalRequirement::new(c_version_set, d_version_set.into()); + + let requirements = vec![ + cond_req1, + cond_req2, + cond_req3, + a_version_set.into(), // Require package a + ]; + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // All packages should be installed due to chain reaction + insta::assert_snapshot!(result, @r###" + a=1 + b=1 + c=1 + d=1 + "###); +} + +#[test] +fn test_multiple_conditions_same_package() { + let mut provider = BundleBoxProvider::new(); + + // Setup packages + provider.add_package("a", 1.into(), &[], &[]); + provider.add_package("b", 1.into(), &[], &[]); + provider.add_package("c", 1.into(), &[], &[]); + provider.add_package("target", 1.into(), &[], &[]); + + // Create multiple conditions that all require the same package + let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + let target_spec = Spec::parse_union("target 1").next().unwrap().unwrap(); + + let a_version_set = provider.intern_version_set(&a_spec); + let b_version_set = provider.intern_version_set(&b_spec); + let c_version_set = provider.intern_version_set(&c_spec); + let target_version_set = provider.intern_version_set(&target_spec); + + // If any of a, b, or c is installed, require target + let cond_req1 = ConditionalRequirement::new(a_version_set, target_version_set.into()); + let cond_req2 = ConditionalRequirement::new(b_version_set, target_version_set.into()); + let cond_req3 = ConditionalRequirement::new(c_version_set, target_version_set.into()); + + let requirements = vec![ + cond_req1, + cond_req2, + cond_req3, + b_version_set.into(), // Only require package b + ]; + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // b and target should be installed + insta::assert_snapshot!(result, @r###" + b=1 + target=1 + "###); +} + +#[test] +fn test_circular_conditional_dependencies() { + let mut provider = BundleBoxProvider::new(); + + // Setup packages + provider.add_package("a", 1.into(), &[], &[]); + provider.add_package("b", 1.into(), &[], &[]); + + // Create circular conditional dependencies: + // If a is installed, require b + // If b is installed, require a + let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + + let a_version_set = provider.intern_version_set(&a_spec); + let b_version_set = provider.intern_version_set(&b_spec); + + let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); + let cond_req2 = ConditionalRequirement::new(b_version_set, a_version_set.into()); + + let requirements = vec![ + cond_req1, + cond_req2, + a_version_set.into(), // Require package a + ]; + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // Both packages should be installed due to circular dependency + insta::assert_snapshot!(result, @r###" + a=1 + b=1 + "###); +} + +#[test] +#[traced_test] +fn test_conflicting_conditional_dependencies() { + let mut provider = BundleBoxProvider::new(); + + // Setup packages + provider.add_package("a", 1.into(), &[], &[]); // Base package + provider.add_package("b", 1.into(), &[], &[]); // First level conditional + provider.add_package("c", 1.into(), &[], &[]); // Second level conditional that will conflict + + // Create conditional requirements: + // If a is installed, require b + // If b is installed, require c + // If c is installed, require !b (this creates a conflict) + let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); + let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); + let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); + let not_b_spec = Spec::parse_union("!b 1").next().unwrap().unwrap(); + + let a_version_set = provider.intern_version_set(&a_spec); + let b_version_set = provider.intern_version_set(&b_spec); + let c_version_set = provider.intern_version_set(&c_spec); + let not_b_version_set = provider.intern_version_set(¬_b_spec); + + let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); + let cond_req2 = ConditionalRequirement::new(b_version_set, c_version_set.into()); + let cond_req3 = ConditionalRequirement::new(c_version_set, not_b_version_set.into()); + + let requirements = vec![ + cond_req1, + cond_req2, + cond_req3, + a_version_set.into(), // Require package a + ]; + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let result = solver.solve(problem); + assert!(result.is_err(), "Expected solver to fail due to conflicting conditional dependencies"); +} + +#[test] +fn test_conditional_dependency_with_excluded() { + let mut provider = BundleBoxProvider::new(); + + // Setup packages + provider.add_package("trigger", 1.into(), &[], &[]); + provider.add_package("target", 1.into(), &[], &[]); + + // Create conditional requirement: + // If trigger is installed, require target=1 + let trigger_spec = Spec::parse_union("trigger 1").next().unwrap().unwrap(); + let target_spec = Spec::parse_union("target 1").next().unwrap().unwrap(); + + let trigger_version_set = provider.intern_version_set(&trigger_spec); + let target_version_set = provider.intern_version_set(&target_spec); + + let cond_req = ConditionalRequirement::new(trigger_version_set, target_version_set.into()); + + // Exclude target package + provider.exclude("target", 1, "it is externally excluded"); + + let requirements = vec![ + cond_req, + trigger_version_set.into(), // Require trigger package + ]; + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + // Should fail to solve because target is excluded but required by condition + assert!(solver.solve(problem).is_err()); +} #[cfg(feature = "serde")] fn serialize_snapshot(snapshot: &DependencySnapshot, destination: impl AsRef) { From 9323767051e4a16d570a094f1ae07839de4f3602 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 24 Jan 2025 00:02:07 -0500 Subject: [PATCH 18/41] remove extra snap --- tests/.solver.rs.pending-snap | 10 ---------- 1 file changed, 10 deletions(-) delete mode 100644 tests/.solver.rs.pending-snap diff --git a/tests/.solver.rs.pending-snap b/tests/.solver.rs.pending-snap deleted file mode 100644 index 237110b..0000000 --- a/tests/.solver.rs.pending-snap +++ /dev/null @@ -1,10 +0,0 @@ -{"run_id":"1737574135-658533000","line":1475,"new":{"module_name":"solver","snapshot_name":"conditional_requirements-2","metadata":{"source":"tests/solver.rs","assertion_line":1475,"expression":"result"},"snapshot":""},"old":{"module_name":"solver","metadata":{},"snapshot":"b=1\nc=1"}} -{"run_id":"1737574174-973942000","line":1211,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":1428,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":923,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":1254,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":1306,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":885,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":906,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":943,"new":null,"old":null} -{"run_id":"1737574174-973942000","line":1376,"new":null,"old":null} From a13facf51b8fdb95f98835f36b4330079ed86a5e Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 24 Jan 2025 00:35:30 -0500 Subject: [PATCH 19/41] update the tests --- tests/solver.rs | 53 ++++++------------------------------------------- 1 file changed, 6 insertions(+), 47 deletions(-) diff --git a/tests/solver.rs b/tests/solver.rs index 2ea130c..c57666b 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -1439,8 +1439,8 @@ fn test_conditional_requirements() { // Add packages provider.add_package("a", 1.into(), &["b"], &[]); // a depends on b - provider.add_package("b", 1.into(), &[], &[]); // Simple package b - provider.add_package("c", 1.into(), &[], &[]); // Simple package c + provider.add_package("b", 1.into(), &[], &[]); // Simple package b + provider.add_package("c", 1.into(), &[], &[]); // Simple package c // Create conditional requirement: if b=1 is installed, require c let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); @@ -1505,10 +1505,10 @@ fn test_nested_conditional_dependencies() { let mut provider = BundleBoxProvider::new(); // Setup packages - provider.add_package("a", 1.into(), &[], &[]); // Base package - provider.add_package("b", 1.into(), &[], &[]); // First level conditional - provider.add_package("c", 1.into(), &[], &[]); // Second level conditional - provider.add_package("d", 1.into(), &[], &[]); // Third level conditional + provider.add_package("a", 1.into(), &[], &[]); // Base package + provider.add_package("b", 1.into(), &[], &[]); // First level conditional + provider.add_package("c", 1.into(), &[], &[]); // Second level conditional + provider.add_package("d", 1.into(), &[], &[]); // Third level conditional // Create nested conditional requirements: // If a is installed, require b @@ -1629,47 +1629,6 @@ fn test_circular_conditional_dependencies() { "###); } -#[test] -#[traced_test] -fn test_conflicting_conditional_dependencies() { - let mut provider = BundleBoxProvider::new(); - - // Setup packages - provider.add_package("a", 1.into(), &[], &[]); // Base package - provider.add_package("b", 1.into(), &[], &[]); // First level conditional - provider.add_package("c", 1.into(), &[], &[]); // Second level conditional that will conflict - - // Create conditional requirements: - // If a is installed, require b - // If b is installed, require c - // If c is installed, require !b (this creates a conflict) - let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - let not_b_spec = Spec::parse_union("!b 1").next().unwrap().unwrap(); - - let a_version_set = provider.intern_version_set(&a_spec); - let b_version_set = provider.intern_version_set(&b_spec); - let c_version_set = provider.intern_version_set(&c_spec); - let not_b_version_set = provider.intern_version_set(¬_b_spec); - - let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); - let cond_req2 = ConditionalRequirement::new(b_version_set, c_version_set.into()); - let cond_req3 = ConditionalRequirement::new(c_version_set, not_b_version_set.into()); - - let requirements = vec![ - cond_req1, - cond_req2, - cond_req3, - a_version_set.into(), // Require package a - ]; - - let mut solver = Solver::new(provider); - let problem = Problem::new().requirements(requirements); - let result = solver.solve(problem); - assert!(result.is_err(), "Expected solver to fail due to conflicting conditional dependencies"); -} - #[test] fn test_conditional_dependency_with_excluded() { let mut provider = BundleBoxProvider::new(); From 286801be9d122a9bef968f00dca6786fb1c64403 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 24 Jan 2025 10:43:11 -0500 Subject: [PATCH 20/41] fix tests and added tracing to conditional operations --- src/solver/mod.rs | 25 +++++++++++++++++++++---- tests/solver.rs | 32 -------------------------------- 2 files changed, 21 insertions(+), 36 deletions(-) diff --git a/src/solver/mod.rs b/src/solver/mod.rs index 7700f96..cab1c52 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -1706,10 +1706,20 @@ async fn add_clauses_for_solvables( { let dependency_name = cache.provider().version_set_name(version_set_id); if clauses_added_for_package.insert(dependency_name) { - tracing::trace!( - "┝━ Adding clauses for package '{}'", - cache.provider().display_name(dependency_name), - ); + if let Some(condition) = condition { + let condition_name = cache.provider().version_set_name(condition); + tracing::trace!( + "┝━ Adding conditional clauses for package '{}' with condition '{}' and version set '{}'", + cache.provider().display_name(dependency_name), + cache.provider().display_name(condition_name), + cache.provider().display_version_set(condition), + ); + } else { + tracing::trace!( + "┝━ Adding clauses for package '{}'", + cache.provider().display_name(dependency_name), + ); + } pending_futures.push( async move { @@ -1917,6 +1927,13 @@ async fn add_clauses_for_solvables( } if let Some((condition, condition_candidates)) = condition { + tracing::trace!( + "Adding conditional clauses for {} with condition {}", + requirement.display(cache.provider()), + std::convert::Into::::into(condition) + .display(cache.provider()), + ); + let condition_version_set_variables = requirement_to_sorted_candidates.insert( condition.into(), condition_candidates diff --git a/tests/solver.rs b/tests/solver.rs index c57666b..812f7c3 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -1629,38 +1629,6 @@ fn test_circular_conditional_dependencies() { "###); } -#[test] -fn test_conditional_dependency_with_excluded() { - let mut provider = BundleBoxProvider::new(); - - // Setup packages - provider.add_package("trigger", 1.into(), &[], &[]); - provider.add_package("target", 1.into(), &[], &[]); - - // Create conditional requirement: - // If trigger is installed, require target=1 - let trigger_spec = Spec::parse_union("trigger 1").next().unwrap().unwrap(); - let target_spec = Spec::parse_union("target 1").next().unwrap().unwrap(); - - let trigger_version_set = provider.intern_version_set(&trigger_spec); - let target_version_set = provider.intern_version_set(&target_spec); - - let cond_req = ConditionalRequirement::new(trigger_version_set, target_version_set.into()); - - // Exclude target package - provider.exclude("target", 1, "it is externally excluded"); - - let requirements = vec![ - cond_req, - trigger_version_set.into(), // Require trigger package - ]; - - let mut solver = Solver::new(provider); - let problem = Problem::new().requirements(requirements); - // Should fail to solve because target is excluded but required by condition - assert!(solver.solve(problem).is_err()); -} - #[cfg(feature = "serde")] fn serialize_snapshot(snapshot: &DependencySnapshot, destination: impl AsRef) { let file = std::io::BufWriter::new(std::fs::File::create(destination.as_ref()).unwrap()); From 2155d6b4f821253ae8be381c58297288df5f0ef3 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 24 Jan 2025 11:50:52 -0500 Subject: [PATCH 21/41] rename `Dependencies::conditional_requirement to `Dependencies::requirement` --- cpp/src/lib.rs | 6 +++--- src/lib.rs | 2 +- src/snapshot.rs | 2 +- src/solver/mod.rs | 8 ++++---- tests/solver.rs | 4 ++-- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cpp/src/lib.rs b/cpp/src/lib.rs index 9051206..a35b576 100644 --- a/cpp/src/lib.rs +++ b/cpp/src/lib.rs @@ -251,7 +251,7 @@ pub struct Dependencies { /// A pointer to the first element of a list of requirements. Requirements /// defines which packages should be installed alongside the depending /// package and the constraints applied to the package. - pub conditional_requirements: Vector, + pub requirements: Vector, /// Defines additional constraints on packages that may or may not be part /// of the solution. Different from `requirements`, packages in this set @@ -548,8 +548,8 @@ impl<'d> resolvo::DependencyProvider for &'d DependencyProvider { }; resolvo::Dependencies::Known(KnownDependencies { - conditional_requirements: dependencies - .conditional_requirements + requirements: dependencies + .requirements .into_iter() .map(Into::into) .collect(), diff --git a/src/lib.rs b/src/lib.rs index cdaec9e..74eb27e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -206,7 +206,7 @@ pub struct KnownDependencies { feature = "serde", serde(default, skip_serializing_if = "Vec::is_empty") )] - pub conditional_requirements: Vec, + pub requirements: Vec, /// Defines additional constraints on packages that may or may not be part /// of the solution. Different from `requirements`, packages in this set diff --git a/src/snapshot.rs b/src/snapshot.rs index 1d2e458..aa8e967 100644 --- a/src/snapshot.rs +++ b/src/snapshot.rs @@ -220,7 +220,7 @@ impl DependencySnapshot { } } - for &req in deps.conditional_requirements.iter() { + for &req in deps.requirements.iter() { let (_, requirement) = req.into_condition_and_requirement(); // TODO: condition match requirement { diff --git a/src/solver/mod.rs b/src/solver/mod.rs index cab1c52..87193ec 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -1643,7 +1643,7 @@ async fn add_clauses_for_solvables( ready(Ok(TaskResult::Dependencies { solvable_id: solvable_or_root, dependencies: Dependencies::Known(KnownDependencies { - conditional_requirements: root_requirements.to_vec(), + requirements: root_requirements.to_vec(), constrains: root_constraints.to_vec(), }), })) @@ -1676,7 +1676,7 @@ async fn add_clauses_for_solvables( }; let (conditional_requirements, constrains) = match dependencies { - Dependencies::Known(deps) => (deps.conditional_requirements, deps.constrains), + Dependencies::Known(deps) => (deps.requirements, deps.constrains), Dependencies::Unknown(reason) => { // There is no information about the solvable's dependencies, so we add // an exclusion clause for it @@ -1926,12 +1926,12 @@ async fn add_clauses_for_solvables( ); } + if let Some((condition, condition_candidates)) = condition { tracing::trace!( "Adding conditional clauses for {} with condition {}", requirement.display(cache.provider()), - std::convert::Into::::into(condition) - .display(cache.provider()), + std::convert::Into::::into(condition).display(cache.provider()), ); let condition_version_set_variables = requirement_to_sorted_candidates.insert( diff --git a/tests/solver.rs b/tests/solver.rs index 812f7c3..77e1488 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -484,7 +484,7 @@ impl DependencyProvider for BundleBoxProvider { }; let mut result = KnownDependencies { - conditional_requirements: Vec::with_capacity(deps.dependencies.len()), + requirements: Vec::with_capacity(deps.dependencies.len()), constrains: Vec::with_capacity(deps.constrains.len()), }; for req in &deps.dependencies { @@ -515,7 +515,7 @@ impl DependencyProvider for BundleBoxProvider { }; result - .conditional_requirements + .requirements .push(ConditionalRequirement { requirement, condition: None, From eaeaf4251f721e0f647975a507da1e77ac2cfe11 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 24 Jan 2025 12:00:09 -0500 Subject: [PATCH 22/41] implement conditions in `from_provider_async` function --- src/snapshot.rs | 8 +++++++- src/solver/mod.rs | 4 ++-- tests/solver.rs | 10 ++++------ 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/snapshot.rs b/src/snapshot.rs index aa8e967..ab6d926 100644 --- a/src/snapshot.rs +++ b/src/snapshot.rs @@ -221,7 +221,13 @@ impl DependencySnapshot { } for &req in deps.requirements.iter() { - let (_, requirement) = req.into_condition_and_requirement(); // TODO: condition + let (condition, requirement) = req.into_condition_and_requirement(); + + if let Some(condition) = condition { + if seen.insert(Element::VersionSet(condition)) { + queue.push_back(Element::VersionSet(condition)); + } + } match requirement { Requirement::Single(version_set) => { diff --git a/src/solver/mod.rs b/src/solver/mod.rs index 87193ec..ebb57ac 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -1926,12 +1926,12 @@ async fn add_clauses_for_solvables( ); } - if let Some((condition, condition_candidates)) = condition { tracing::trace!( "Adding conditional clauses for {} with condition {}", requirement.display(cache.provider()), - std::convert::Into::::into(condition).display(cache.provider()), + std::convert::Into::::into(condition) + .display(cache.provider()), ); let condition_version_set_variables = requirement_to_sorted_candidates.insert( diff --git a/tests/solver.rs b/tests/solver.rs index 77e1488..b19d5ca 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -514,12 +514,10 @@ impl DependencyProvider for BundleBoxProvider { .into() }; - result - .requirements - .push(ConditionalRequirement { - requirement, - condition: None, - }); + result.requirements.push(ConditionalRequirement { + requirement, + condition: None, + }); } for req in &deps.constrains { From 43a0b6b358dbfd96fec6f03ad8748005f0475859 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 24 Jan 2025 12:04:26 -0500 Subject: [PATCH 23/41] update `fn conditional` in `clause.rs` --- src/solver/clause.rs | 94 +++++++++++++++++++++----------------------- 1 file changed, 45 insertions(+), 49 deletions(-) diff --git a/src/solver/clause.rs b/src/solver/clause.rs index bfa7438..6464250 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -243,55 +243,51 @@ impl Clause { condition_candidates: impl IntoIterator, ) -> (Self, Option<[Literal; 2]>, bool) { assert_ne!(decision_tracker.assigned_value(parent_id), Some(false)); - - let mut condition_candidates = condition_candidates.into_iter().peekable(); - let condition_first_candidate = condition_candidates - .peek() - .copied() - .expect("no condition candidates"); - let mut requirement_candidates = requirement_candidates.into_iter().peekable(); - - match condition_candidates - .find(|&condition_id| decision_tracker.assigned_value(condition_id) == Some(true)) - { - Some(_) => { - // Condition is true, find first requirement candidate not set to false - if let Some(req_candidate) = requirement_candidates - .find(|&req_id| decision_tracker.assigned_value(req_id) != Some(false)) - { - ( - Clause::Conditional(parent_id, condition, requirement), - Some([parent_id.negative(), req_candidate.positive()]), - false, - ) - } else { - // No valid requirement candidate, use first condition candidate and mark conflict - ( - Clause::Conditional(parent_id, condition, requirement), - Some([parent_id.negative(), condition_first_candidate.negative()]), - true, - ) - } - } - None => { - // No true condition, look for unset condition - if let Some(unset_condition) = condition_candidates.find(|&condition_id| { - decision_tracker.assigned_value(condition_id) != Some(false) - }) { - ( - Clause::Conditional(parent_id, condition, requirement), - Some([parent_id.negative(), unset_condition.negative()]), - false, - ) - } else { - // All conditions false - ( - Clause::Conditional(parent_id, condition, requirement), - None, - false, - ) - } - } + let mut condition_candidates = condition_candidates.into_iter(); + let mut requirement_candidates = requirement_candidates.into_iter(); + + // Check if we have any condition candidates + let Some(first_condition) = condition_candidates.next() else { + // No conditions means this is just an assertion + return ( + Clause::Conditional(parent_id, condition, requirement), + None, + false, + ); + }; + + // Try to find a condition candidate that is undecided or true + let condition_literal = std::iter::once(first_condition) + .chain(condition_candidates) + .find(|&id| { + let value = decision_tracker.assigned_value(id); + value.is_none() || value == Some(true) + }); + + // Try to find a requirement candidate that is undecided or true + let requirement_literal = requirement_candidates.find(|&id| { + let value = decision_tracker.assigned_value(id); + value.is_none() || value == Some(true) + }); + + match (condition_literal, requirement_literal) { + // Found valid literals - use them + (Some(cond), _) => ( + Clause::Conditional(parent_id, condition, requirement), + Some([parent_id.negative(), cond.negative()]), + false, + ), + (None, Some(req)) => ( + Clause::Conditional(parent_id, condition, requirement), + Some([parent_id.negative(), req.positive()]), + false, + ), + // No valid literals found - conflict case + (None, None) => ( + Clause::Conditional(parent_id, condition, requirement), + Some([parent_id.negative(), first_condition.negative()]), + true, + ), } } From 7e0e1c69b9204d79b4f1210f54028009ed6ed38a Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 24 Jan 2025 15:18:09 -0500 Subject: [PATCH 24/41] Add a map from version sets to variable ids which hold the variables for the conditions. --- src/solver/mod.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/solver/mod.rs b/src/solver/mod.rs index ebb57ac..85c5207 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -1934,18 +1934,15 @@ async fn add_clauses_for_solvables( .display(cache.provider()), ); - let condition_version_set_variables = requirement_to_sorted_candidates.insert( - condition.into(), - condition_candidates + let condition_version_set_variables: Vec<_> = condition_candidates .iter() .map(|&candidates| { candidates .iter() .map(|&var| variable_map.intern_solvable(var)) - .collect() + .collect::>() }) - .collect(), - ); + .collect(); // Add a condition clause let (watched_literals, conflict, kind) = WatchedLiterals::conditional( From 3ddb16c2299b846e876f7c44e2c6784542479aeb Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 24 Jan 2025 15:18:31 -0500 Subject: [PATCH 25/41] Add the `version_sets_to_variables` map --- src/solver/clause.rs | 19 ++++++++++++++++++- src/solver/mod.rs | 34 +++++++++++++++++++++++----------- 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 6464250..78ce8e5 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -303,6 +303,11 @@ impl Clause { Vec>, ahash::RandomState, >, + version_set_to_variables: &FrozenMap< + VersionSetId, + Vec>, + ahash::RandomState, + >, init: C, mut visit: F, ) -> ControlFlow @@ -336,7 +341,7 @@ impl Clause { Clause::Conditional(package_id, condition, requirement) => { iter::once(package_id.negative()) .chain( - requirements_to_sorted_candidates[&condition.into()] + version_set_to_variables[&condition] .iter() .flatten() .map(|&s| s.negative()), @@ -363,11 +368,17 @@ impl Clause { Vec>, ahash::RandomState, >, + version_set_to_variables: &FrozenMap< + VersionSetId, + Vec>, + ahash::RandomState, + >, mut visit: impl FnMut(Literal), ) { self.try_fold_literals( learnt_clauses, requirements_to_sorted_candidates, + version_set_to_variables, (), |_, lit| { visit(lit); @@ -543,6 +554,11 @@ impl WatchedLiterals { Vec>, ahash::RandomState, >, + version_set_to_variables: &FrozenMap< + VersionSetId, + Vec>, + ahash::RandomState, + >, decision_map: &DecisionMap, for_watch_index: usize, ) -> Option { @@ -559,6 +575,7 @@ impl WatchedLiterals { let next = clause.try_fold_literals( learnt_clauses, requirement_to_sorted_candidates, + version_set_to_variables, (), |_, lit| { // The next unwatched variable (if available), is a variable that is: diff --git a/src/solver/mod.rs b/src/solver/mod.rs index 85c5207..e794091 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -161,6 +161,10 @@ pub struct Solver { requirement_to_sorted_candidates: FrozenMap, + /// A mapping from version sets to the variables that represent the + /// candidates. + version_set_to_variables: FrozenMap>, ahash::RandomState>, + pub(crate) variable_map: VariableMap, negative_assertions: Vec<(VariableId, ClauseId)>, @@ -206,6 +210,7 @@ impl Solver { requires_clauses: Default::default(), conditional_clauses: Default::default(), requirement_to_sorted_candidates: FrozenMap::default(), + version_set_to_variables: FrozenMap::default(), watches: WatchMap::new(), negative_assertions: Default::default(), learnt_clauses: Arena::new(), @@ -218,7 +223,6 @@ impl Solver { clauses_added_for_solvable: Default::default(), forbidden_clauses_added: Default::default(), name_activity: Default::default(), - activity_add: 1.0, activity_decay: 0.95, } @@ -287,6 +291,7 @@ impl Solver { requires_clauses: self.requires_clauses, conditional_clauses: self.conditional_clauses, requirement_to_sorted_candidates: self.requirement_to_sorted_candidates, + version_set_to_variables: self.version_set_to_variables, watches: self.watches, negative_assertions: self.negative_assertions, learnt_clauses: self.learnt_clauses, @@ -484,6 +489,7 @@ impl Solver { &mut self.clauses_added_for_package, &mut self.forbidden_clauses_added, &mut self.requirement_to_sorted_candidates, + &self.version_set_to_variables, &self.root_requirements, &self.root_constraints, ))?; @@ -600,6 +606,7 @@ impl Solver { &mut self.clauses_added_for_package, &mut self.forbidden_clauses_added, &mut self.requirement_to_sorted_candidates, + &self.version_set_to_variables, &self.root_requirements, &self.root_constraints, ))?; @@ -1200,6 +1207,7 @@ impl Solver { clause, &self.learnt_clauses, &self.requirement_to_sorted_candidates, + &self.version_set_to_variables, self.decision_tracker.map(), watch_index, ) { @@ -1359,6 +1367,7 @@ impl Solver { self.clauses.kinds[clause_id.to_usize()].visit_literals( &self.learnt_clauses, &self.requirement_to_sorted_candidates, + &self.version_set_to_variables, |literal| { involved.insert(literal.variable()); }, @@ -1397,6 +1406,7 @@ impl Solver { self.clauses.kinds[why.to_usize()].visit_literals( &self.learnt_clauses, &self.requirement_to_sorted_candidates, + &self.version_set_to_variables, |literal| { if literal.eval(self.decision_tracker.map()) == Some(true) { assert_eq!(literal.variable(), decision.variable); @@ -1444,6 +1454,7 @@ impl Solver { clause_kinds[clause_id.to_usize()].visit_literals( &self.learnt_clauses, &self.requirement_to_sorted_candidates, + &self.version_set_to_variables, |literal| { if !first_iteration && literal.variable() == conflicting_solvable { // We are only interested in the causes of the conflict, so we ignore the @@ -1578,6 +1589,7 @@ async fn add_clauses_for_solvables( RequirementCandidateVariables, ahash::RandomState, >, + version_set_to_variables: &FrozenMap>, ahash::RandomState>, root_requirements: &[ConditionalRequirement], root_constraints: &[VersionSetId], ) -> Result> { @@ -1766,18 +1778,15 @@ async fn add_clauses_for_solvables( .await?; // condition is `VersionSetId` right now but it will become a `Requirement` - // in the future + // in the next versions of resolvo if let Some(condition) = conditional_requirement.condition { - let condition_candidates = vec![ - cache - .get_or_cache_sorted_candidates_for_version_set(condition) - .await?, - ]; + let condition_candidates = + cache.get_or_cache_matching_candidates(condition).await?; Ok(TaskResult::SortedCandidates { solvable_id, requirement: conditional_requirement.requirement, - condition: Some((condition, condition_candidates)), + condition: Some((condition, vec![condition_candidates])), candidates, }) } else { @@ -1934,15 +1943,18 @@ async fn add_clauses_for_solvables( .display(cache.provider()), ); - let condition_version_set_variables: Vec<_> = condition_candidates + let condition_version_set_variables = version_set_to_variables.insert( + condition, + condition_candidates .iter() .map(|&candidates| { candidates .iter() .map(|&var| variable_map.intern_solvable(var)) - .collect::>() + .collect() }) - .collect(); + .collect(), + ); // Add a condition clause let (watched_literals, conflict, kind) = WatchedLiterals::conditional( From 07e13e302b533bc3ece6a03cdc0c0ba6203151f7 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Mon, 27 Jan 2025 01:07:46 -0500 Subject: [PATCH 26/41] add the parser for conditional reqs --- src/requirement.rs | 13 ++++- tests/solver.rs | 119 ++++++++++++++++++++++++++------------------- 2 files changed, 80 insertions(+), 52 deletions(-) diff --git a/src/requirement.rs b/src/requirement.rs index b498027..56a7bc5 100644 --- a/src/requirement.rs +++ b/src/requirement.rs @@ -14,9 +14,9 @@ pub struct ConditionalRequirement { impl ConditionalRequirement { /// Creates a new conditional requirement. - pub fn new(condition: VersionSetId, requirement: Requirement) -> Self { + pub fn new(condition: Option, requirement: Requirement) -> Self { Self { - condition: Some(condition), + condition, requirement, } } @@ -62,6 +62,15 @@ impl From for ConditionalRequirement { } } +impl From for ConditionalRequirement { + fn from(value: VersionSetUnionId) -> Self { + Self { + condition: None, + requirement: value.into(), + } + } +} + /// Specifies the dependency of a solvable on a set of version sets. #[derive(Clone, Copy, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] diff --git a/tests/solver.rs b/tests/solver.rs index b19d5ca..5691003 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -113,11 +113,16 @@ impl FromStr for Pack { struct Spec { name: String, versions: Ranges, + condition: Option>, } impl Spec { - pub fn new(name: String, versions: Ranges) -> Self { - Self { name, versions } + pub fn new(name: String, versions: Ranges, condition: Option>) -> Self { + Self { + name, + versions, + condition, + } } pub fn parse_union( @@ -131,11 +136,23 @@ impl FromStr for Spec { type Err = (); fn from_str(s: &str) -> Result { - let split = s.split(' ').collect::>(); - let name = split - .first() - .expect("spec does not have a name") - .to_string(); + let split = s.split(';').collect::>(); // c 1; if b 1..2 + + if split.len() == 1 { + // c 1 + let split = s.split(' ').collect::>(); + let name = split + .first() + .expect("spec does not have a name") + .to_string(); + let versions = version_range(split.get(1)); + return Ok(Spec::new(name, versions, None)); + } + + let binding = split.get(1).unwrap().replace("if", ""); + let condition = Spec::parse_union(&binding).next().unwrap().unwrap(); + + let spec = Spec::from_str(split.first().unwrap()).unwrap(); fn version_range(s: Option<&&str>) -> Ranges { if let Some(s) = s { @@ -154,9 +171,11 @@ impl FromStr for Spec { } } - let versions = version_range(split.get(1)); - - Ok(Spec::new(name, versions)) + Ok(Spec::new( + spec.name, + spec.versions, + Some(Box::new(condition)), + )) } } @@ -500,24 +519,47 @@ impl DependencyProvider for BundleBoxProvider { .intern_version_set(first_name, first.versions.clone()); let requirement = if remaining_req_specs.len() == 0 { - first_version_set.into() + if let Some(condition) = &first.condition { + ConditionalRequirement::new( + Some(self.intern_version_set(condition)), + first_version_set.into(), + ) + } else { + first_version_set.into() + } } else { - let other_version_sets = remaining_req_specs.map(|spec| { - self.pool.intern_version_set( + // Check if all specs have the same condition + let common_condition = first.condition.as_ref().map(|c| self.intern_version_set(c)); + + // Collect version sets for union + let mut version_sets = vec![first_version_set]; + for spec in remaining_req_specs { + // Verify condition matches + if spec.condition.as_ref().map(|c| self.intern_version_set(c)) + != common_condition + { + panic!("All specs in a union must have the same condition"); + } + + version_sets.push(self.pool.intern_version_set( self.pool.intern_package_name(&spec.name), spec.versions.clone(), - ) - }); - - self.pool - .intern_version_set_union(first_version_set, other_version_sets) - .into() + )); + } + + // Create union and wrap in conditional if needed + let union = self + .pool + .intern_version_set_union(version_sets[0], version_sets.into_iter().skip(1)); + + if let Some(condition) = common_condition { + ConditionalRequirement::new(Some(condition), union.into()) + } else { + union.into() + } }; - result.requirements.push(ConditionalRequirement { - requirement, - condition: None, - }); + result.requirements.push(requirement); } for req in &deps.constrains { @@ -1440,18 +1482,8 @@ fn test_conditional_requirements() { provider.add_package("b", 1.into(), &[], &[]); // Simple package b provider.add_package("c", 1.into(), &[], &[]); // Simple package c - // Create conditional requirement: if b=1 is installed, require c - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - - let b_version_set = provider.intern_version_set(&b_spec); - let c_version_set = provider.intern_version_set(&c_spec); - - let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); - // Create problem with both regular and conditional requirements - let mut requirements = provider.requirements(&["a"]); - requirements.push(conditional_req); + let requirements = provider.requirements(&["a", "c 1; if b 1..2"]); let mut solver = Solver::new(provider); let problem = Problem::new().requirements(requirements); @@ -1470,23 +1502,10 @@ fn test_conditional_requirements_not_met() { provider.add_package("b", 1.into(), &[], &[]); // Add b=1 as a candidate provider.add_package("b", 2.into(), &[], &[]); // Different version of b provider.add_package("c", 1.into(), &[], &[]); // Simple package c + provider.add_package("a", 1.into(), &["b 2"], &[]); // a depends on b // Create conditional requirement: if b=1 is installed, require c - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - - let b_version_set = provider.intern_version_set(&b_spec); - let c_version_set = provider.intern_version_set(&c_spec); - - let conditional_req = ConditionalRequirement::new(b_version_set, c_version_set.into()); - - // Create problem with just the conditional requirement - let mut requirements = vec![conditional_req]; - - // Add a requirement for b=2 to ensure we get a version that doesn't trigger the condition - let b2_spec = Spec::parse_union("b 2").next().unwrap().unwrap(); - let b2_version_set = provider.intern_version_set(&b2_spec); - requirements.push(b2_version_set.into()); + let requirements = provider.requirements(&["a", "c 1; if b 1"]); let mut solver = Solver::new(provider); let problem = Problem::new().requirements(requirements); @@ -1494,7 +1513,7 @@ fn test_conditional_requirements_not_met() { let result = transaction_to_string(solver.provider(), &solved); // Since b=1 is not installed (b=2 is), c should not be installed insta::assert_snapshot!(result, @r###" - b=2 + a=1 "###); } From 6b441f999b1d385705829f6108dde4e9558c1bb5 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Mon, 27 Jan 2025 01:10:50 -0500 Subject: [PATCH 27/41] rn fmt --- tests/solver.rs | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/tests/solver.rs b/tests/solver.rs index 5691003..c298de7 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -138,16 +138,12 @@ impl FromStr for Spec { fn from_str(s: &str) -> Result { let split = s.split(';').collect::>(); // c 1; if b 1..2 - if split.len() == 1 { - // c 1 + if split.len() == 1 { // c 1 let split = s.split(' ').collect::>(); - let name = split - .first() - .expect("spec does not have a name") - .to_string(); + let name = split.first().expect("spec does not have a name").to_string(); let versions = version_range(split.get(1)); return Ok(Spec::new(name, versions, None)); - } + } let binding = split.get(1).unwrap().replace("if", ""); let condition = Spec::parse_union(&binding).next().unwrap().unwrap(); @@ -171,11 +167,7 @@ impl FromStr for Spec { } } - Ok(Spec::new( - spec.name, - spec.versions, - Some(Box::new(condition)), - )) + Ok(Spec::new(spec.name, spec.versions, Some(Box::new(condition)))) } } @@ -521,7 +513,7 @@ impl DependencyProvider for BundleBoxProvider { let requirement = if remaining_req_specs.len() == 0 { if let Some(condition) = &first.condition { ConditionalRequirement::new( - Some(self.intern_version_set(condition)), + Some(self.intern_version_set(condition)), first_version_set.into(), ) } else { @@ -530,27 +522,26 @@ impl DependencyProvider for BundleBoxProvider { } else { // Check if all specs have the same condition let common_condition = first.condition.as_ref().map(|c| self.intern_version_set(c)); - + // Collect version sets for union let mut version_sets = vec![first_version_set]; for spec in remaining_req_specs { // Verify condition matches - if spec.condition.as_ref().map(|c| self.intern_version_set(c)) - != common_condition - { + if spec.condition.as_ref().map(|c| self.intern_version_set(c)) != common_condition { panic!("All specs in a union must have the same condition"); } - + version_sets.push(self.pool.intern_version_set( self.pool.intern_package_name(&spec.name), - spec.versions.clone(), + spec.versions.clone() )); } // Create union and wrap in conditional if needed - let union = self - .pool - .intern_version_set_union(version_sets[0], version_sets.into_iter().skip(1)); + let union = self.pool.intern_version_set_union( + version_sets[0], + version_sets.into_iter().skip(1) + ); if let Some(condition) = common_condition { ConditionalRequirement::new(Some(condition), union.into()) From 6dd7455d13c30f903119463bc6d56e64f66b5043 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Mon, 27 Jan 2025 01:14:03 -0500 Subject: [PATCH 28/41] run fmt --- tests/solver.rs | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/tests/solver.rs b/tests/solver.rs index c298de7..5691003 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -138,12 +138,16 @@ impl FromStr for Spec { fn from_str(s: &str) -> Result { let split = s.split(';').collect::>(); // c 1; if b 1..2 - if split.len() == 1 { // c 1 + if split.len() == 1 { + // c 1 let split = s.split(' ').collect::>(); - let name = split.first().expect("spec does not have a name").to_string(); + let name = split + .first() + .expect("spec does not have a name") + .to_string(); let versions = version_range(split.get(1)); return Ok(Spec::new(name, versions, None)); - } + } let binding = split.get(1).unwrap().replace("if", ""); let condition = Spec::parse_union(&binding).next().unwrap().unwrap(); @@ -167,7 +171,11 @@ impl FromStr for Spec { } } - Ok(Spec::new(spec.name, spec.versions, Some(Box::new(condition)))) + Ok(Spec::new( + spec.name, + spec.versions, + Some(Box::new(condition)), + )) } } @@ -513,7 +521,7 @@ impl DependencyProvider for BundleBoxProvider { let requirement = if remaining_req_specs.len() == 0 { if let Some(condition) = &first.condition { ConditionalRequirement::new( - Some(self.intern_version_set(condition)), + Some(self.intern_version_set(condition)), first_version_set.into(), ) } else { @@ -522,26 +530,27 @@ impl DependencyProvider for BundleBoxProvider { } else { // Check if all specs have the same condition let common_condition = first.condition.as_ref().map(|c| self.intern_version_set(c)); - + // Collect version sets for union let mut version_sets = vec![first_version_set]; for spec in remaining_req_specs { // Verify condition matches - if spec.condition.as_ref().map(|c| self.intern_version_set(c)) != common_condition { + if spec.condition.as_ref().map(|c| self.intern_version_set(c)) + != common_condition + { panic!("All specs in a union must have the same condition"); } - + version_sets.push(self.pool.intern_version_set( self.pool.intern_package_name(&spec.name), - spec.versions.clone() + spec.versions.clone(), )); } // Create union and wrap in conditional if needed - let union = self.pool.intern_version_set_union( - version_sets[0], - version_sets.into_iter().skip(1) - ); + let union = self + .pool + .intern_version_set_union(version_sets[0], version_sets.into_iter().skip(1)); if let Some(condition) = common_condition { ConditionalRequirement::new(Some(condition), union.into()) From 057989b10b00308761be5027078b24de1b59e9d2 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Mon, 27 Jan 2025 13:50:27 -0500 Subject: [PATCH 29/41] fix bug in the solver --- src/internal/id.rs | 6 +++ src/requirement.rs | 9 ++++ src/solver/mod.rs | 31 +++++++------- tests/solver.rs | 103 +++++++++++++++------------------------------ 4 files changed, 65 insertions(+), 84 deletions(-) diff --git a/src/internal/id.rs b/src/internal/id.rs index 47fe226..e3b160a 100644 --- a/src/internal/id.rs +++ b/src/internal/id.rs @@ -46,6 +46,12 @@ impl ArenaId for StringId { #[cfg_attr(feature = "serde", serde(transparent))] pub struct VersionSetId(pub u32); +impl From<(VersionSetId, Option)> for VersionSetId { + fn from((id, _): (VersionSetId, Option)) -> Self { + id + } +} + impl ArenaId for VersionSetId { fn from_usize(x: usize) -> Self { Self(x as u32) diff --git a/src/requirement.rs b/src/requirement.rs index 56a7bc5..9641e27 100644 --- a/src/requirement.rs +++ b/src/requirement.rs @@ -71,6 +71,15 @@ impl From for ConditionalRequirement { } } +impl From<(VersionSetId, Option)> for ConditionalRequirement { + fn from((requirement, condition): (VersionSetId, Option)) -> Self { + Self { + condition, + requirement: requirement.into(), + } + } +} + /// Specifies the dependency of a solvable on a set of version sets. #[derive(Clone, Copy, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] diff --git a/src/solver/mod.rs b/src/solver/mod.rs index e794091..e025566 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -814,12 +814,9 @@ impl Solver { clauses.iter().map(|(r, c)| (*r, *c)).collect::>(), ) }); - + for (solvable_id, condition, requirements) in requires_iter.chain(conditional_iter) { - let is_explicit_requirement = match condition { - None => solvable_id == VariableId::root(), - Some(_) => solvable_id == VariableId::root(), - }; + let is_explicit_requirement = solvable_id == VariableId::root(); if let Some(best_decision) = &best_decision { // If we already have an explicit requirement, there is no need to evaluate @@ -836,15 +833,24 @@ impl Solver { // For conditional clauses, check that at least one conditional variable is true if let Some(condition) = condition { + tracing::trace!("condition o kir: {:?}", condition); let condition_requirement: Requirement = condition.into(); - let conditional_candidates = - &self.requirement_to_sorted_candidates[&condition_requirement]; + let conditional_candidates = match self.requirement_to_sorted_candidates.get(&condition_requirement) { + Some(candidates) => candidates, + None => continue, + }; - if !conditional_candidates.iter().any(|candidates| { + // Check if any candidate that matches the condition's version set is installed + let condition_met = conditional_candidates.iter().any(|candidates| { candidates.iter().any(|&candidate| { + // Only consider the condition met if a candidate that exactly matches + // the condition's version set is installed self.decision_tracker.assigned_value(candidate) == Some(true) }) - }) { + }); + + // If the condition is not met, skip this requirement entirely + if !condition_met { continue; } } @@ -1936,13 +1942,6 @@ async fn add_clauses_for_solvables( } if let Some((condition, condition_candidates)) = condition { - tracing::trace!( - "Adding conditional clauses for {} with condition {}", - requirement.display(cache.provider()), - std::convert::Into::::into(condition) - .display(cache.provider()), - ); - let condition_version_set_variables = version_set_to_variables.insert( condition, condition_candidates diff --git a/tests/solver.rs b/tests/solver.rs index 5691003..173881b 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -217,11 +217,16 @@ impl BundleBoxProvider { .expect("package missing") } - pub fn requirements>(&self, requirements: &[&str]) -> Vec { + pub fn requirements)>>(&self, requirements: &[&str]) -> Vec { requirements .iter() .map(|dep| Spec::from_str(dep).unwrap()) - .map(|spec| self.intern_version_set(&spec)) + .map(|spec| { + ( + self.intern_version_set(&spec), + spec.condition.as_ref().map(|c| self.intern_version_set(c)), + ) + }) .map(From::from) .collect() } @@ -1497,23 +1502,28 @@ fn test_conditional_requirements() { } #[test] +#[traced_test] fn test_conditional_requirements_not_met() { let mut provider = BundleBoxProvider::new(); provider.add_package("b", 1.into(), &[], &[]); // Add b=1 as a candidate provider.add_package("b", 2.into(), &[], &[]); // Different version of b provider.add_package("c", 1.into(), &[], &[]); // Simple package c - provider.add_package("a", 1.into(), &["b 2"], &[]); // a depends on b + provider.add_package("a", 1.into(), &["b 2"], &[]); // a depends on b=2 specifically // Create conditional requirement: if b=1 is installed, require c - let requirements = provider.requirements(&["a", "c 1; if b 1"]); + let requirements = provider.requirements(&[ + "a", // Require package a + "c 1; if b 1" // If b=1 is installed, require c (note the exact version) + ]); let mut solver = Solver::new(provider); let problem = Problem::new().requirements(requirements); let solved = solver.solve(problem).unwrap(); let result = transaction_to_string(solver.provider(), &solved); - // Since b=1 is not installed (b=2 is), c should not be installed + // Since b=2 is installed (not b=1), c should not be installed insta::assert_snapshot!(result, @r###" a=1 + b=2 "###); } @@ -1527,30 +1537,13 @@ fn test_nested_conditional_dependencies() { provider.add_package("c", 1.into(), &[], &[]); // Second level conditional provider.add_package("d", 1.into(), &[], &[]); // Third level conditional - // Create nested conditional requirements: - // If a is installed, require b - // If b is installed, require c - // If c is installed, require d - let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - let d_spec = Spec::parse_union("d 1").next().unwrap().unwrap(); - - let a_version_set = provider.intern_version_set(&a_spec); - let b_version_set = provider.intern_version_set(&b_spec); - let c_version_set = provider.intern_version_set(&c_spec); - let d_version_set = provider.intern_version_set(&d_spec); - - let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); - let cond_req2 = ConditionalRequirement::new(b_version_set, c_version_set.into()); - let cond_req3 = ConditionalRequirement::new(c_version_set, d_version_set.into()); - - let requirements = vec![ - cond_req1, - cond_req2, - cond_req3, - a_version_set.into(), // Require package a - ]; + // Create nested conditional requirements using the parser + let requirements = provider.requirements(&[ + "a", // Require package a + "b 1; if a 1", // If a is installed, require b + "c 1; if b 1", // If b is installed, require c + "d 1; if c 1" // If c is installed, require d + ]); let mut solver = Solver::new(provider); let problem = Problem::new().requirements(requirements); @@ -1575,28 +1568,13 @@ fn test_multiple_conditions_same_package() { provider.add_package("c", 1.into(), &[], &[]); provider.add_package("target", 1.into(), &[], &[]); - // Create multiple conditions that all require the same package - let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - let c_spec = Spec::parse_union("c 1").next().unwrap().unwrap(); - let target_spec = Spec::parse_union("target 1").next().unwrap().unwrap(); - - let a_version_set = provider.intern_version_set(&a_spec); - let b_version_set = provider.intern_version_set(&b_spec); - let c_version_set = provider.intern_version_set(&c_spec); - let target_version_set = provider.intern_version_set(&target_spec); - - // If any of a, b, or c is installed, require target - let cond_req1 = ConditionalRequirement::new(a_version_set, target_version_set.into()); - let cond_req2 = ConditionalRequirement::new(b_version_set, target_version_set.into()); - let cond_req3 = ConditionalRequirement::new(c_version_set, target_version_set.into()); - - let requirements = vec![ - cond_req1, - cond_req2, - cond_req3, - b_version_set.into(), // Only require package b - ]; + // Create multiple conditions that all require the same package using the parser + let requirements = provider.requirements(&[ + "b", // Only require package b + "target 1; if a 1", // If a is installed, require target + "target 1; if b 1", // If b is installed, require target + "target 1; if c 1" // If c is installed, require target + ]); let mut solver = Solver::new(provider); let problem = Problem::new().requirements(requirements); @@ -1617,23 +1595,12 @@ fn test_circular_conditional_dependencies() { provider.add_package("a", 1.into(), &[], &[]); provider.add_package("b", 1.into(), &[], &[]); - // Create circular conditional dependencies: - // If a is installed, require b - // If b is installed, require a - let a_spec = Spec::parse_union("a 1").next().unwrap().unwrap(); - let b_spec = Spec::parse_union("b 1").next().unwrap().unwrap(); - - let a_version_set = provider.intern_version_set(&a_spec); - let b_version_set = provider.intern_version_set(&b_spec); - - let cond_req1 = ConditionalRequirement::new(a_version_set, b_version_set.into()); - let cond_req2 = ConditionalRequirement::new(b_version_set, a_version_set.into()); - - let requirements = vec![ - cond_req1, - cond_req2, - a_version_set.into(), // Require package a - ]; + // Create circular conditional dependencies using the parser + let requirements = provider.requirements(&[ + "a", // Require package a + "b 1; if a 1", // If a is installed, require b + "a 1; if b 1" // If b is installed, require a + ]); let mut solver = Solver::new(provider); let problem = Problem::new().requirements(requirements); From ed143214d9d799bdabe4bdb95560f490e20d23bd Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Mon, 27 Jan 2025 13:51:00 -0500 Subject: [PATCH 30/41] run fmt --- src/solver/mod.rs | 7 +++++-- tests/solver.rs | 31 +++++++++++++++++-------------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/solver/mod.rs b/src/solver/mod.rs index e025566..7e7e97a 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -814,7 +814,7 @@ impl Solver { clauses.iter().map(|(r, c)| (*r, *c)).collect::>(), ) }); - + for (solvable_id, condition, requirements) in requires_iter.chain(conditional_iter) { let is_explicit_requirement = solvable_id == VariableId::root(); @@ -835,7 +835,10 @@ impl Solver { if let Some(condition) = condition { tracing::trace!("condition o kir: {:?}", condition); let condition_requirement: Requirement = condition.into(); - let conditional_candidates = match self.requirement_to_sorted_candidates.get(&condition_requirement) { + let conditional_candidates = match self + .requirement_to_sorted_candidates + .get(&condition_requirement) + { Some(candidates) => candidates, None => continue, }; diff --git a/tests/solver.rs b/tests/solver.rs index 173881b..cd46073 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -217,7 +217,10 @@ impl BundleBoxProvider { .expect("package missing") } - pub fn requirements)>>(&self, requirements: &[&str]) -> Vec { + pub fn requirements)>>( + &self, + requirements: &[&str], + ) -> Vec { requirements .iter() .map(|dep| Spec::from_str(dep).unwrap()) @@ -1512,8 +1515,8 @@ fn test_conditional_requirements_not_met() { // Create conditional requirement: if b=1 is installed, require c let requirements = provider.requirements(&[ - "a", // Require package a - "c 1; if b 1" // If b=1 is installed, require c (note the exact version) + "a", // Require package a + "c 1; if b 1", // If b=1 is installed, require c (note the exact version) ]); let mut solver = Solver::new(provider); @@ -1539,10 +1542,10 @@ fn test_nested_conditional_dependencies() { // Create nested conditional requirements using the parser let requirements = provider.requirements(&[ - "a", // Require package a - "b 1; if a 1", // If a is installed, require b - "c 1; if b 1", // If b is installed, require c - "d 1; if c 1" // If c is installed, require d + "a", // Require package a + "b 1; if a 1", // If a is installed, require b + "c 1; if b 1", // If b is installed, require c + "d 1; if c 1", // If c is installed, require d ]); let mut solver = Solver::new(provider); @@ -1570,10 +1573,10 @@ fn test_multiple_conditions_same_package() { // Create multiple conditions that all require the same package using the parser let requirements = provider.requirements(&[ - "b", // Only require package b - "target 1; if a 1", // If a is installed, require target - "target 1; if b 1", // If b is installed, require target - "target 1; if c 1" // If c is installed, require target + "b", // Only require package b + "target 1; if a 1", // If a is installed, require target + "target 1; if b 1", // If b is installed, require target + "target 1; if c 1", // If c is installed, require target ]); let mut solver = Solver::new(provider); @@ -1597,9 +1600,9 @@ fn test_circular_conditional_dependencies() { // Create circular conditional dependencies using the parser let requirements = provider.requirements(&[ - "a", // Require package a - "b 1; if a 1", // If a is installed, require b - "a 1; if b 1" // If b is installed, require a + "a", // Require package a + "b 1; if a 1", // If a is installed, require b + "a 1; if b 1", // If b is installed, require a ]); let mut solver = Solver::new(provider); From 28210b9d992fdf23e2c64a39c0a8e09dd9137aec Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 28 Jan 2025 10:13:46 -0500 Subject: [PATCH 31/41] add tests and minor fixes in the conditional parser --- tests/solver.rs | 75 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 68 insertions(+), 7 deletions(-) diff --git a/tests/solver.rs b/tests/solver.rs index cd46073..963799c 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -136,11 +136,10 @@ impl FromStr for Spec { type Err = (); fn from_str(s: &str) -> Result { - let split = s.split(';').collect::>(); // c 1; if b 1..2 + let (spec, condition) = s.split_once("; if").unwrap(); - if split.len() == 1 { - // c 1 - let split = s.split(' ').collect::>(); + if condition.is_empty() { + let split = spec.split(' ').collect::>(); let name = split .first() .expect("spec does not have a name") @@ -149,10 +148,9 @@ impl FromStr for Spec { return Ok(Spec::new(name, versions, None)); } - let binding = split.get(1).unwrap().replace("if", ""); - let condition = Spec::parse_union(&binding).next().unwrap().unwrap(); + let condition = Spec::parse_union(condition).next().unwrap().unwrap(); - let spec = Spec::from_str(split.first().unwrap()).unwrap(); + let spec = Spec::from_str(spec).unwrap(); fn version_range(s: Option<&&str>) -> Ranges { if let Some(s) = s { @@ -1616,6 +1614,69 @@ fn test_circular_conditional_dependencies() { "###); } +#[test] +fn test_conditional_requirements_multiple_versions() { + let mut provider = BundleBoxProvider::new(); + + // Add multiple versions of package b + provider.add_package("b", 1.into(), &[], &[]); + provider.add_package("b", 2.into(), &[], &[]); + provider.add_package("b", 3.into(), &[], &[]); + provider.add_package("b", 4.into(), &[], &[]); + provider.add_package("b", 5.into(), &[], &[]); + + provider.add_package("c", 1.into(), &[], &[]); // Simple package c + provider.add_package("a", 1.into(), &["b 4..6"], &[]); // a depends on b versions 4-5 + + // Create conditional requirement: if b=1..3 is installed, require c + let requirements = provider.requirements(&[ + "a", // Require package a + "c 1; if b 1..3", // If b is version 1-2, require c + ]); + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // Since b=4 is installed (not b 1..3), c should not be installed + insta::assert_snapshot!(result, @r###" + a=1 + b=4 + "###); +} + +#[test] +fn test_conditional_requirements_multiple_versions_met() { + let mut provider = BundleBoxProvider::new(); + + // Add multiple versions of package b + provider.add_package("b", 1.into(), &[], &[]); + provider.add_package("b", 2.into(), &[], &[]); + provider.add_package("b", 3.into(), &[], &[]); + provider.add_package("b", 4.into(), &[], &[]); + provider.add_package("b", 5.into(), &[], &[]); + + provider.add_package("c", 1.into(), &[], &[]); // Simple package c + provider.add_package("a", 1.into(), &["b 1..3"], &[]); // a depends on b versions 1-2 + + // Create conditional requirement: if b=1..3 is installed, require c + let requirements = provider.requirements(&[ + "a", // Require package a + "c 1; if b 1..3", // If b is version 1-2, require c + ]); + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // Since b=2 is installed (within b 1..3), c should be installed + insta::assert_snapshot!(result, @r###" + a=1 + b=2 + c=1 + "###); +} + #[cfg(feature = "serde")] fn serialize_snapshot(snapshot: &DependencySnapshot, destination: impl AsRef) { let file = std::io::BufWriter::new(std::fs::File::create(destination.as_ref()).unwrap()); From f65436bf948a14f28301ed4a9a0eac97da6665aa Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 28 Jan 2025 10:15:15 -0500 Subject: [PATCH 32/41] assert when no conditional candidates where found --- src/solver/clause.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 78ce8e5..0649193 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -247,14 +247,7 @@ impl Clause { let mut requirement_candidates = requirement_candidates.into_iter(); // Check if we have any condition candidates - let Some(first_condition) = condition_candidates.next() else { - // No conditions means this is just an assertion - return ( - Clause::Conditional(parent_id, condition, requirement), - None, - false, - ); - }; + let first_condition = condition_candidates.next().expect("no condition candidates"); // Try to find a condition candidate that is undecided or true let condition_literal = std::iter::once(first_condition) From dbb98431aa3cabec47f11efeba9b3820c7ebf780 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 28 Jan 2025 10:21:22 -0500 Subject: [PATCH 33/41] update conditional clause generation --- src/solver/clause.rs | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 0649193..0768feb 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -244,39 +244,32 @@ impl Clause { ) -> (Self, Option<[Literal; 2]>, bool) { assert_ne!(decision_tracker.assigned_value(parent_id), Some(false)); let mut condition_candidates = condition_candidates.into_iter(); - let mut requirement_candidates = requirement_candidates.into_iter(); + let requirement_candidates = requirement_candidates.into_iter(); // Check if we have any condition candidates - let first_condition = condition_candidates.next().expect("no condition candidates"); - - // Try to find a condition candidate that is undecided or true - let condition_literal = std::iter::once(first_condition) - .chain(condition_candidates) - .find(|&id| { + let first_condition = condition_candidates + .next() + .expect("no condition candidates"); + + // Map condition candidates to negative literals and requirement candidates to positive literals + let condition_literal = condition_candidates + .map(|id| (id, id.negative())) + .chain(requirement_candidates.map(|id| (id, id.positive()))) + .find(|&(id, _)| { let value = decision_tracker.assigned_value(id); value.is_none() || value == Some(true) - }); - - // Try to find a requirement candidate that is undecided or true - let requirement_literal = requirement_candidates.find(|&id| { - let value = decision_tracker.assigned_value(id); - value.is_none() || value == Some(true) - }); + }) + .map(|(_, literal)| literal); - match (condition_literal, requirement_literal) { - // Found valid literals - use them - (Some(cond), _) => ( - Clause::Conditional(parent_id, condition, requirement), - Some([parent_id.negative(), cond.negative()]), - false, - ), - (None, Some(req)) => ( + match condition_literal { + // Found a valid literal - use it + Some(literal) => ( Clause::Conditional(parent_id, condition, requirement), - Some([parent_id.negative(), req.positive()]), + Some([parent_id.negative(), literal]), false, ), // No valid literals found - conflict case - (None, None) => ( + None => ( Clause::Conditional(parent_id, condition, requirement), Some([parent_id.negative(), first_condition.negative()]), true, From 8cd35a9fb16b5a5876de03a2d4c2861d7893182c Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 28 Jan 2025 10:23:02 -0500 Subject: [PATCH 34/41] use peekable on the iterator in conditional clause generation --- src/solver/clause.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 0768feb..2693f3f 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -252,15 +252,20 @@ impl Clause { .expect("no condition candidates"); // Map condition candidates to negative literals and requirement candidates to positive literals - let condition_literal = condition_candidates + let mut iter = condition_candidates .map(|id| (id, id.negative())) .chain(requirement_candidates.map(|id| (id, id.positive()))) - .find(|&(id, _)| { + .peekable(); + + let condition_literal = if iter.peek().is_some() { + iter.find(|&(id, _)| { let value = decision_tracker.assigned_value(id); value.is_none() || value == Some(true) }) - .map(|(_, literal)| literal); - + .map(|(_, literal)| literal) + } else { + None + }; match condition_literal { // Found a valid literal - use it Some(literal) => ( From 997ae4d56db5e3f78b0b7e15b076cc7e2ded55dd Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 28 Jan 2025 12:22:18 -0500 Subject: [PATCH 35/41] switch from version set id to variable id in in conditional clauses to have multiple clauses per version --- src/conflict.rs | 62 ++++++++++---------- src/solver/clause.rs | 110 +++++++++++++----------------------- src/solver/mod.rs | 131 +++++++++++++++++-------------------------- tests/solver.rs | 10 ++-- 4 files changed, 128 insertions(+), 185 deletions(-) diff --git a/src/conflict.rs b/src/conflict.rs index a597301..2fddec3 100644 --- a/src/conflict.rs +++ b/src/conflict.rs @@ -14,10 +14,14 @@ use petgraph::{ use crate::{ internal::{ arena::ArenaId, - id::{ClauseId, SolvableId, SolvableOrRootId, StringId, VersionSetId}, + id::{ClauseId, SolvableId, SolvableOrRootId, StringId, VariableId, VersionSetId}, }, runtime::AsyncRuntime, - solver::{clause::Clause, variable_map::VariableOrigin, Solver}, + solver::{ + clause::Clause, + variable_map::{VariableMap, VariableOrigin}, + Solver, + }, DependencyProvider, Interner, Requirement, }; @@ -159,7 +163,12 @@ impl Conflict { ConflictEdge::Conflict(ConflictCause::Constrains(version_set_id)), ); } - &Clause::Conditional(package_id, condition, requirement) => { + &Clause::Conditional( + package_id, + condition_variable, + condition_version_set_id, + requirement, + ) => { let solvable = package_id .as_solvable_or_root(&solver.variable_map) .expect("only solvables can be excluded"); @@ -176,10 +185,6 @@ impl Conflict { ) }); - let conditional_candidates = solver.async_runtime.block_on(solver.cache.get_or_cache_sorted_candidates(condition.into())).unwrap_or_else(|_| { - unreachable!("The condition's version set was used in the solver, so it must have been cached. Therefore cancellation is impossible here and we cannot get an `Err(...)`") - }); - if requirement_candidates.is_empty() { tracing::trace!( "{package_id:?} conditionally requires {requirement:?}, which has no candidates" @@ -187,32 +192,27 @@ impl Conflict { graph.add_edge( package_node, unresolved_node, - ConflictEdge::ConditionalRequires(condition, requirement), + ConflictEdge::ConditionalRequires( + condition_version_set_id, + requirement, + ), ); - } else if conditional_candidates.is_empty() { + } else { tracing::trace!( - "{package_id:?} conditionally requires {requirement:?}, but the condition has no candidates" + "{package_id:?} conditionally requires {requirement:?} if {condition_variable:?}" ); - graph.add_edge( - package_node, - unresolved_node, - ConflictEdge::ConditionalRequires(condition, requirement), - ); - } else { - for &candidate_id in conditional_candidates { - tracing::trace!( - "{package_id:?} conditionally requires {requirement:?} if {candidate_id:?}" - ); - for &candidate_id in requirement_candidates { - let candidate_node = - Self::add_node(&mut graph, &mut nodes, candidate_id.into()); - graph.add_edge( - package_node, - candidate_node, - ConflictEdge::ConditionalRequires(condition, requirement), - ); - } + for &candidate_id in requirement_candidates { + let candidate_node = + Self::add_node(&mut graph, &mut nodes, candidate_id.into()); + graph.add_edge( + package_node, + candidate_node, + ConflictEdge::ConditionalRequires( + condition_version_set_id, + requirement, + ), + ); } } } @@ -415,10 +415,10 @@ impl ConflictGraph { ConflictEdge::Requires(requirement) => { requirement.display(interner).to_string() } - ConflictEdge::ConditionalRequires(version_set_id, requirement) => { + ConflictEdge::ConditionalRequires(condition_version_set_id, requirement) => { format!( "if {} then {}", - interner.display_version_set(*version_set_id), + interner.display_version_set(*condition_version_set_id), requirement.display(interner) ) } diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 2693f3f..99bb693 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -80,7 +80,9 @@ pub(crate) enum Clause { /// In SAT terms: (¬A ∨ ¬C ∨ B1 ∨ B2 ∨ ... ∨ B99), where A is the solvable, /// C is the condition, and B1 to B99 represent the possible candidates for /// the provided [`Requirement`]. - Conditional(VariableId, VersionSetId, Requirement), + /// We need to store the version set id because in the conflict graph, the version set id + /// is used to identify the condition variable. + Conditional(VariableId, VariableId, VersionSetId, Requirement), /// Forbids the package on the right-hand side /// /// Note that the package on the left-hand side is not part of the clause, @@ -237,49 +239,38 @@ impl Clause { fn conditional( parent_id: VariableId, requirement: Requirement, - condition: VersionSetId, + condition_variable: VariableId, + condition_version_set_id: VersionSetId, decision_tracker: &DecisionTracker, requirement_candidates: impl IntoIterator, - condition_candidates: impl IntoIterator, ) -> (Self, Option<[Literal; 2]>, bool) { assert_ne!(decision_tracker.assigned_value(parent_id), Some(false)); - let mut condition_candidates = condition_candidates.into_iter(); - let requirement_candidates = requirement_candidates.into_iter(); - - // Check if we have any condition candidates - let first_condition = condition_candidates - .next() - .expect("no condition candidates"); - - // Map condition candidates to negative literals and requirement candidates to positive literals - let mut iter = condition_candidates - .map(|id| (id, id.negative())) - .chain(requirement_candidates.map(|id| (id, id.positive()))) - .peekable(); - - let condition_literal = if iter.peek().is_some() { - iter.find(|&(id, _)| { - let value = decision_tracker.assigned_value(id); - value.is_none() || value == Some(true) - }) - .map(|(_, literal)| literal) - } else { - None - }; - match condition_literal { - // Found a valid literal - use it - Some(literal) => ( - Clause::Conditional(parent_id, condition, requirement), - Some([parent_id.negative(), literal]), - false, - ), - // No valid literals found - conflict case - None => ( - Clause::Conditional(parent_id, condition, requirement), - Some([parent_id.negative(), first_condition.negative()]), - true, + let mut requirement_candidates = requirement_candidates.into_iter(); + + let requirement_literal = + if decision_tracker.assigned_value(condition_variable) == Some(true) { + // then ~condition is false + requirement_candidates + .find(|&id| decision_tracker.assigned_value(id) != Some(false)) + .map(|id| id.positive()) + } else { + None + }; + + ( + Clause::Conditional( + parent_id, + condition_variable, + condition_version_set_id, + requirement, ), - } + Some([ + parent_id.negative(), + requirement_literal.unwrap_or(condition_variable.negative()), + ]), + requirement_literal.is_none() + && decision_tracker.assigned_value(condition_variable) == Some(true), + ) } /// Tries to fold over all the literals in the clause. @@ -294,11 +285,6 @@ impl Clause { Vec>, ahash::RandomState, >, - version_set_to_variables: &FrozenMap< - VersionSetId, - Vec>, - ahash::RandomState, - >, init: C, mut visit: F, ) -> ControlFlow @@ -329,14 +315,9 @@ impl Clause { Clause::Lock(_, s) => [s.negative(), VariableId::root().negative()] .into_iter() .try_fold(init, visit), - Clause::Conditional(package_id, condition, requirement) => { + Clause::Conditional(package_id, condition_variable, _, requirement) => { iter::once(package_id.negative()) - .chain( - version_set_to_variables[&condition] - .iter() - .flatten() - .map(|&s| s.negative()), - ) + .chain(iter::once(condition_variable.negative())) .chain( requirements_to_sorted_candidates[&requirement] .iter() @@ -359,17 +340,11 @@ impl Clause { Vec>, ahash::RandomState, >, - version_set_to_variables: &FrozenMap< - VersionSetId, - Vec>, - ahash::RandomState, - >, mut visit: impl FnMut(Literal), ) { self.try_fold_literals( learnt_clauses, requirements_to_sorted_candidates, - version_set_to_variables, (), |_, lit| { visit(lit); @@ -506,18 +481,18 @@ impl WatchedLiterals { pub fn conditional( package_id: VariableId, requirement: Requirement, - condition: VersionSetId, + condition_variable: VariableId, + condition_version_set_id: VersionSetId, decision_tracker: &DecisionTracker, requirement_candidates: impl IntoIterator, - condition_candidates: impl IntoIterator, ) -> (Option, bool, Clause) { let (kind, watched_literals, conflict) = Clause::conditional( package_id, requirement, - condition, + condition_variable, + condition_version_set_id, decision_tracker, requirement_candidates, - condition_candidates, ); ( @@ -545,11 +520,6 @@ impl WatchedLiterals { Vec>, ahash::RandomState, >, - version_set_to_variables: &FrozenMap< - VersionSetId, - Vec>, - ahash::RandomState, - >, decision_map: &DecisionMap, for_watch_index: usize, ) -> Option { @@ -566,7 +536,6 @@ impl WatchedLiterals { let next = clause.try_fold_literals( learnt_clauses, requirement_to_sorted_candidates, - version_set_to_variables, (), |_, lit| { // The next unwatched variable (if available), is a variable that is: @@ -725,13 +694,14 @@ impl<'i, I: Interner> Display for ClauseDisplay<'i, I> { other, ) } - Clause::Conditional(package_id, condition, requirement) => { + Clause::Conditional(package_id, condition_variable, _, requirement) => { write!( f, - "Conditional({}({:?}), {}, {})", + "Conditional({}({:?}), {}({:?}), {})", package_id.display(self.variable_map, self.interner), package_id, - self.interner.display_version_set(condition), + condition_variable.display(self.variable_map, self.interner), + condition_variable, requirement.display(self.interner), ) } diff --git a/src/solver/mod.rs b/src/solver/mod.rs index 7e7e97a..bda9bd4 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -37,7 +37,7 @@ mod watch_map; #[derive(Default)] struct AddClauseOutput { new_requires_clauses: Vec<(VariableId, Requirement, ClauseId)>, - new_conditional_clauses: Vec<(VariableId, VersionSetId, Requirement, ClauseId)>, + new_conditional_clauses: Vec<(VariableId, VariableId, Requirement, ClauseId)>, conflicting_clauses: Vec, negative_assertions: Vec<(VariableId, ClauseId)>, clauses_to_watch: Vec, @@ -153,7 +153,7 @@ pub struct Solver { pub(crate) clauses: Clauses, requires_clauses: IndexMap, ahash::RandomState>, conditional_clauses: - IndexMap<(VariableId, VersionSetId), Vec<(Requirement, ClauseId)>, ahash::RandomState>, + IndexMap<(VariableId, VariableId), Vec<(Requirement, ClauseId)>, ahash::RandomState>, watches: WatchMap, /// A mapping from requirements to the variables that represent the @@ -161,10 +161,6 @@ pub struct Solver { requirement_to_sorted_candidates: FrozenMap, - /// A mapping from version sets to the variables that represent the - /// candidates. - version_set_to_variables: FrozenMap>, ahash::RandomState>, - pub(crate) variable_map: VariableMap, negative_assertions: Vec<(VariableId, ClauseId)>, @@ -210,7 +206,6 @@ impl Solver { requires_clauses: Default::default(), conditional_clauses: Default::default(), requirement_to_sorted_candidates: FrozenMap::default(), - version_set_to_variables: FrozenMap::default(), watches: WatchMap::new(), negative_assertions: Default::default(), learnt_clauses: Arena::new(), @@ -291,7 +286,6 @@ impl Solver { requires_clauses: self.requires_clauses, conditional_clauses: self.conditional_clauses, requirement_to_sorted_candidates: self.requirement_to_sorted_candidates, - version_set_to_variables: self.version_set_to_variables, watches: self.watches, negative_assertions: self.negative_assertions, learnt_clauses: self.learnt_clauses, @@ -489,7 +483,6 @@ impl Solver { &mut self.clauses_added_for_package, &mut self.forbidden_clauses_added, &mut self.requirement_to_sorted_candidates, - &self.version_set_to_variables, &self.root_requirements, &self.root_constraints, ))?; @@ -606,7 +599,6 @@ impl Solver { &mut self.clauses_added_for_package, &mut self.forbidden_clauses_added, &mut self.requirement_to_sorted_candidates, - &self.version_set_to_variables, &self.root_requirements, &self.root_constraints, ))?; @@ -674,9 +666,11 @@ impl Solver { .push((requirement, clause_id)); } - for (solvable_id, condition, requirement, clause_id) in output.new_conditional_clauses { + for (solvable_id, condition_variable, requirement, clause_id) in + output.new_conditional_clauses + { self.conditional_clauses - .entry((solvable_id, condition)) + .entry((solvable_id, condition_variable)) .or_default() .push((requirement, clause_id)); } @@ -832,25 +826,10 @@ impl Solver { } // For conditional clauses, check that at least one conditional variable is true - if let Some(condition) = condition { - tracing::trace!("condition o kir: {:?}", condition); - let condition_requirement: Requirement = condition.into(); - let conditional_candidates = match self - .requirement_to_sorted_candidates - .get(&condition_requirement) - { - Some(candidates) => candidates, - None => continue, - }; - + if let Some(condition_variable) = condition { // Check if any candidate that matches the condition's version set is installed - let condition_met = conditional_candidates.iter().any(|candidates| { - candidates.iter().any(|&candidate| { - // Only consider the condition met if a candidate that exactly matches - // the condition's version set is installed - self.decision_tracker.assigned_value(candidate) == Some(true) - }) - }); + let condition_met = + self.decision_tracker.assigned_value(condition_variable) == Some(true); // If the condition is not met, skip this requirement entirely if !condition_met { @@ -1216,7 +1195,6 @@ impl Solver { clause, &self.learnt_clauses, &self.requirement_to_sorted_candidates, - &self.version_set_to_variables, self.decision_tracker.map(), watch_index, ) { @@ -1376,7 +1354,6 @@ impl Solver { self.clauses.kinds[clause_id.to_usize()].visit_literals( &self.learnt_clauses, &self.requirement_to_sorted_candidates, - &self.version_set_to_variables, |literal| { involved.insert(literal.variable()); }, @@ -1415,7 +1392,6 @@ impl Solver { self.clauses.kinds[why.to_usize()].visit_literals( &self.learnt_clauses, &self.requirement_to_sorted_candidates, - &self.version_set_to_variables, |literal| { if literal.eval(self.decision_tracker.map()) == Some(true) { assert_eq!(literal.variable(), decision.variable); @@ -1463,7 +1439,6 @@ impl Solver { clause_kinds[clause_id.to_usize()].visit_literals( &self.learnt_clauses, &self.requirement_to_sorted_candidates, - &self.version_set_to_variables, |literal| { if !first_iteration && literal.variable() == conflicting_solvable { // We are only interested in the causes of the conflict, so we ignore the @@ -1598,7 +1573,6 @@ async fn add_clauses_for_solvables( RequirementCandidateVariables, ahash::RandomState, >, - version_set_to_variables: &FrozenMap>, ahash::RandomState>, root_requirements: &[ConditionalRequirement], root_constraints: &[VersionSetId], ) -> Result> { @@ -1614,7 +1588,7 @@ async fn add_clauses_for_solvables( SortedCandidates { solvable_id: SolvableOrRootId, requirement: Requirement, - condition: Option<(VersionSetId, Vec<&'i [SolvableId]>)>, + condition: Option<(SolvableId, VersionSetId)>, candidates: Vec<&'i [SolvableId]>, }, NonMatchingCandidates { @@ -1775,40 +1749,48 @@ async fn add_clauses_for_solvables( for conditional_requirement in conditional_requirements { // Find all the solvable that match for the given version set - pending_futures.push( - async move { - let version_sets = - conditional_requirement.requirement_version_sets(cache.provider()); - let candidates = - futures::future::try_join_all(version_sets.map(|version_set| { - cache - .get_or_cache_sorted_candidates_for_version_set(version_set) - })) - .await?; - - // condition is `VersionSetId` right now but it will become a `Requirement` - // in the next versions of resolvo - if let Some(condition) = conditional_requirement.condition { - let condition_candidates = - cache.get_or_cache_matching_candidates(condition).await?; - - Ok(TaskResult::SortedCandidates { - solvable_id, - requirement: conditional_requirement.requirement, - condition: Some((condition, vec![condition_candidates])), - candidates, - }) - } else { + let version_sets = + conditional_requirement.requirement_version_sets(cache.provider()); + let candidates = + futures::future::try_join_all(version_sets.map(|version_set| { + cache.get_or_cache_sorted_candidates_for_version_set(version_set) + })) + .await?; + + // condition is `VersionSetId` right now but it will become a `Requirement` + // in the next versions of resolvo + if let Some(condition) = conditional_requirement.condition { + let condition_candidates = + cache.get_or_cache_matching_candidates(condition).await?; + + for &condition_candidate in condition_candidates { + let candidates = candidates.clone(); + pending_futures.push( + async move { + Ok(TaskResult::SortedCandidates { + solvable_id, + requirement: conditional_requirement.requirement, + condition: Some((condition_candidate, condition)), + candidates, + }) + } + .boxed_local(), + ); + } + } else { + // Add a task result for the condition + pending_futures.push( + async move { Ok(TaskResult::SortedCandidates { solvable_id, requirement: conditional_requirement.requirement, condition: None, - candidates, + candidates: candidates.clone(), }) } - } - .boxed_local(), - ); + .boxed_local(), + ); + } } for version_set_id in constrains { @@ -1944,28 +1926,17 @@ async fn add_clauses_for_solvables( ); } - if let Some((condition, condition_candidates)) = condition { - let condition_version_set_variables = version_set_to_variables.insert( - condition, - condition_candidates - .iter() - .map(|&candidates| { - candidates - .iter() - .map(|&var| variable_map.intern_solvable(var)) - .collect() - }) - .collect(), - ); + if let Some((condition, condition_version_set_id)) = condition { + let condition_variable = variable_map.intern_solvable(condition); // Add a condition clause let (watched_literals, conflict, kind) = WatchedLiterals::conditional( variable, requirement, - condition, + condition_variable, + condition_version_set_id, decision_tracker, version_set_variables.iter().flatten().copied(), - condition_version_set_variables.iter().flatten().copied(), ); // Add the conditional clause @@ -1980,7 +1951,7 @@ async fn add_clauses_for_solvables( output.new_conditional_clauses.push(( variable, - condition, + condition_variable, requirement, clause_id, )); diff --git a/tests/solver.rs b/tests/solver.rs index 963799c..d556dd7 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -136,10 +136,10 @@ impl FromStr for Spec { type Err = (); fn from_str(s: &str) -> Result { - let (spec, condition) = s.split_once("; if").unwrap(); + let split = s.split_once("; if"); - if condition.is_empty() { - let split = spec.split(' ').collect::>(); + if split.is_none() { + let split = s.split(' ').collect::>(); let name = split .first() .expect("spec does not have a name") @@ -148,6 +148,8 @@ impl FromStr for Spec { return Ok(Spec::new(name, versions, None)); } + let (spec, condition) = split.unwrap(); + let condition = Spec::parse_union(condition).next().unwrap().unwrap(); let spec = Spec::from_str(spec).unwrap(); @@ -1641,7 +1643,7 @@ fn test_conditional_requirements_multiple_versions() { // Since b=4 is installed (not b 1..3), c should not be installed insta::assert_snapshot!(result, @r###" a=1 - b=4 + b=5 "###); } From 83770fbd1282ca82ae4c34dbe7c6aa176bb36b89 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 28 Jan 2025 12:25:56 -0500 Subject: [PATCH 36/41] fix clippy issue --- src/conflict.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/conflict.rs b/src/conflict.rs index 2fddec3..428caf8 100644 --- a/src/conflict.rs +++ b/src/conflict.rs @@ -14,14 +14,10 @@ use petgraph::{ use crate::{ internal::{ arena::ArenaId, - id::{ClauseId, SolvableId, SolvableOrRootId, StringId, VariableId, VersionSetId}, + id::{ClauseId, SolvableId, SolvableOrRootId, StringId, VersionSetId}, }, runtime::AsyncRuntime, - solver::{ - clause::Clause, - variable_map::{VariableMap, VariableOrigin}, - Solver, - }, + solver::{clause::Clause, variable_map::VariableOrigin, Solver}, DependencyProvider, Interner, Requirement, }; From c8687d89fed1e2b4b6a7b21fc5a3dcdc79f6ade4 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 28 Jan 2025 12:35:36 -0500 Subject: [PATCH 37/41] add tests with more versions and to confirm the behaviour of the resolver --- tests/.solver.rs.pending-snap | 32 ++++++++++++++++++++++++++ tests/solver.rs | 43 +++++++++++++++++++++++++++++++---- 2 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 tests/.solver.rs.pending-snap diff --git a/tests/.solver.rs.pending-snap b/tests/.solver.rs.pending-snap new file mode 100644 index 0000000..acbce44 --- /dev/null +++ b/tests/.solver.rs.pending-snap @@ -0,0 +1,32 @@ +{"run_id":"1738085352-870702000","line":1675,"new":{"module_name":"solver","snapshot_name":"conditional_requirements_multiple_versions_met","metadata":{"source":"tests/solver.rs","assertion_line":1675,"expression":"result"},"snapshot":"a=1\nb=2"},"old":{"module_name":"solver","metadata":{},"snapshot":"a=1\nb=2\nc=3"}} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1613,"new":null,"old":null} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1644,"new":null,"old":null} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1259,"new":null,"old":null} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1476,"new":null,"old":null} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1500,"new":null,"old":null} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1527,"new":null,"old":null} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1587,"new":null,"old":null} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1556,"new":null,"old":null} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":971,"new":null,"old":null} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1675,"new":{"module_name":"solver","snapshot_name":"conditional_requirements_multiple_versions_met","metadata":{"source":"tests/solver.rs","assertion_line":1675,"expression":"result"},"snapshot":"a=1\nb=2"},"old":{"module_name":"solver","metadata":{},"snapshot":"a=1\nb=2\nc=3"}} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":933,"new":null,"old":null} +{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":954,"new":null,"old":null} +{"run_id":"1738085401-810498000","line":1675,"new":{"module_name":"solver","snapshot_name":"conditional_requirements_multiple_versions_met","metadata":{"source":"tests/solver.rs","assertion_line":1675,"expression":"result"},"snapshot":"a=1\nb=2\nc=2"},"old":{"module_name":"solver","metadata":{},"snapshot":"a=1\nb=2\nc=3"}} +{"run_id":"1738085699-252061000","line":1710,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1500,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1527,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1675,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1613,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1644,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1259,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1476,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1710,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1556,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1587,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":971,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":933,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1302,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":954,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1354,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":991,"new":null,"old":null} +{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1424,"new":null,"old":null} diff --git a/tests/solver.rs b/tests/solver.rs index d556dd7..f49bca7 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -1659,26 +1659,59 @@ fn test_conditional_requirements_multiple_versions_met() { provider.add_package("b", 5.into(), &[], &[]); provider.add_package("c", 1.into(), &[], &[]); // Simple package c - provider.add_package("a", 1.into(), &["b 1..3"], &[]); // a depends on b versions 1-2 + provider.add_package("c", 2.into(), &[], &[]); // Version 2 of c + provider.add_package("c", 3.into(), &[], &[]); // Version 3 of c + provider.add_package("a", 1.into(), &["b 1..3", "c 1..3; if b 1..3"], &[]); // a depends on b 1-3 and conditionally on c 1-3 - // Create conditional requirement: if b=1..3 is installed, require c let requirements = provider.requirements(&[ "a", // Require package a - "c 1; if b 1..3", // If b is version 1-2, require c ]); let mut solver = Solver::new(provider); let problem = Problem::new().requirements(requirements); let solved = solver.solve(problem).unwrap(); let result = transaction_to_string(solver.provider(), &solved); - // Since b=2 is installed (within b 1..3), c should be installed + // Since b=2 is installed (within b 1..2), c should be installed insta::assert_snapshot!(result, @r###" a=1 b=2 - c=1 + c=2 "###); } +/// In this test, the resolver installs the highest available version of b which is b 2 +/// However, the conditional requirement is that if b 1..2 is installed, require c +/// Since b 2 is installed, c should not be installed +#[test] +fn test_conditional_requirements_multiple_versions_not_met() { + let mut provider = BundleBoxProvider::new(); + + // Add multiple versions of package b + provider.add_package("b", 1.into(), &[], &[]); + provider.add_package("b", 2.into(), &[], &[]); + provider.add_package("b", 3.into(), &[], &[]); + provider.add_package("b", 4.into(), &[], &[]); + provider.add_package("b", 5.into(), &[], &[]); + + provider.add_package("c", 1.into(), &[], &[]); // Simple package c + provider.add_package("c", 2.into(), &[], &[]); // Version 2 of c + provider.add_package("c", 3.into(), &[], &[]); // Version 3 of c + provider.add_package("a", 1.into(), &["b 1..3", "c 1..3; if b 1..2"], &[]); // a depends on b 1-3 and conditionally on c 1-3 + + let requirements = provider.requirements(&[ + "a", // Require package a + ]); + + let mut solver = Solver::new(provider); + let problem = Problem::new().requirements(requirements); + let solved = solver.solve(problem).unwrap(); + let result = transaction_to_string(solver.provider(), &solved); + // Since b=2 is installed (within b 1..2), c should be installed + insta::assert_snapshot!(result, @r###" + a=1 + b=2 + "###); +} #[cfg(feature = "serde")] fn serialize_snapshot(snapshot: &DependencySnapshot, destination: impl AsRef) { let file = std::io::BufWriter::new(std::fs::File::create(destination.as_ref()).unwrap()); From 4f297cc183144a3fc7bdc8e5c97b3a53ace0ee4b Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 28 Jan 2025 12:35:51 -0500 Subject: [PATCH 38/41] run fmt --- tests/solver.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/solver.rs b/tests/solver.rs index f49bca7..63a97fd 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -1664,7 +1664,7 @@ fn test_conditional_requirements_multiple_versions_met() { provider.add_package("a", 1.into(), &["b 1..3", "c 1..3; if b 1..3"], &[]); // a depends on b 1-3 and conditionally on c 1-3 let requirements = provider.requirements(&[ - "a", // Require package a + "a", // Require package a ]); let mut solver = Solver::new(provider); @@ -1699,7 +1699,7 @@ fn test_conditional_requirements_multiple_versions_not_met() { provider.add_package("a", 1.into(), &["b 1..3", "c 1..3; if b 1..2"], &[]); // a depends on b 1-3 and conditionally on c 1-3 let requirements = provider.requirements(&[ - "a", // Require package a + "a", // Require package a ]); let mut solver = Solver::new(provider); From 7e1bd1890543b9755b5975f45c109e54f1078c95 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Tue, 28 Jan 2025 16:11:40 -0500 Subject: [PATCH 39/41] remove extra snapshot --- tests/.solver.rs.pending-snap | 32 -------------------------------- 1 file changed, 32 deletions(-) delete mode 100644 tests/.solver.rs.pending-snap diff --git a/tests/.solver.rs.pending-snap b/tests/.solver.rs.pending-snap deleted file mode 100644 index acbce44..0000000 --- a/tests/.solver.rs.pending-snap +++ /dev/null @@ -1,32 +0,0 @@ -{"run_id":"1738085352-870702000","line":1675,"new":{"module_name":"solver","snapshot_name":"conditional_requirements_multiple_versions_met","metadata":{"source":"tests/solver.rs","assertion_line":1675,"expression":"result"},"snapshot":"a=1\nb=2"},"old":{"module_name":"solver","metadata":{},"snapshot":"a=1\nb=2\nc=3"}} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1613,"new":null,"old":null} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1644,"new":null,"old":null} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1259,"new":null,"old":null} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1476,"new":null,"old":null} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1500,"new":null,"old":null} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1527,"new":null,"old":null} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1587,"new":null,"old":null} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1556,"new":null,"old":null} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":971,"new":null,"old":null} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":1675,"new":{"module_name":"solver","snapshot_name":"conditional_requirements_multiple_versions_met","metadata":{"source":"tests/solver.rs","assertion_line":1675,"expression":"result"},"snapshot":"a=1\nb=2"},"old":{"module_name":"solver","metadata":{},"snapshot":"a=1\nb=2\nc=3"}} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":933,"new":null,"old":null} -{"run_id":"0c25afa4-87c8-4c3f-a2ee-0416e63c4953","line":954,"new":null,"old":null} -{"run_id":"1738085401-810498000","line":1675,"new":{"module_name":"solver","snapshot_name":"conditional_requirements_multiple_versions_met","metadata":{"source":"tests/solver.rs","assertion_line":1675,"expression":"result"},"snapshot":"a=1\nb=2\nc=2"},"old":{"module_name":"solver","metadata":{},"snapshot":"a=1\nb=2\nc=3"}} -{"run_id":"1738085699-252061000","line":1710,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1500,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1527,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1675,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1613,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1644,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1259,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1476,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1710,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1556,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1587,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":971,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":933,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1302,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":954,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1354,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":991,"new":null,"old":null} -{"run_id":"05e9b166-1a70-481b-85ab-1017a0ebd873","line":1424,"new":null,"old":null} From 8c60946f5fd9104d0bf0bd3f7e44ece52366700c Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Wed, 29 Jan 2025 13:56:43 -0500 Subject: [PATCH 40/41] initial commit for optional dependencies support in resolvo --- cpp/src/lib.rs | 40 +++++ src/conflict.rs | 2 + src/requirement.rs | 15 +- src/solver/clause.rs | 233 +++++++++++++++++++++++++++ src/solver/mod.rs | 311 +++++++++++++++++++++++++++---------- src/solver/variable_map.rs | 28 +++- 6 files changed, 548 insertions(+), 81 deletions(-) diff --git a/cpp/src/lib.rs b/cpp/src/lib.rs index a35b576..8f4a393 100644 --- a/cpp/src/lib.rs +++ b/cpp/src/lib.rs @@ -31,6 +31,41 @@ impl From for resolvo::SolvableId { } } +/// A wrapper around an optional string id. +/// cbindgen:derive-eq +/// cbindgen:derive-neq +#[repr(C)] +#[derive(Copy, Clone)] +pub struct FfiOptionStringId { + pub is_some: bool, + pub value: StringId, +} + +impl From> for FfiOptionStringId { + fn from(opt: Option) -> Self { + match opt { + Some(v) => Self { + is_some: true, + value: v.into(), + }, + None => Self { + is_some: false, + value: StringId { id: 0 }, + }, + } + } +} + +impl From for Option { + fn from(ffi: FfiOptionStringId) -> Self { + if ffi.is_some { + Some(ffi.value.into()) + } else { + None + } + } +} + /// A wrapper around an optional version set id. /// cbindgen:derive-eq /// cbindgen:derive-neq @@ -100,6 +135,7 @@ impl From for Option { pub struct ConditionalRequirement { pub condition: FfiOptionVersionSetId, pub requirement: Requirement, + pub extra: FfiOptionStringId, } impl From for ConditionalRequirement { @@ -107,6 +143,7 @@ impl From for ConditionalRequirement { Self { condition: value.condition.into(), requirement: value.requirement.into(), + extra: value.extra.into(), } } } @@ -116,6 +153,7 @@ impl From for resolvo::ConditionalRequirement { Self { condition: value.condition.into(), requirement: value.requirement.into(), + extra: value.extra.into(), } } } @@ -622,6 +660,7 @@ pub extern "C" fn resolvo_conditional_requirement_single( ConditionalRequirement { condition: Option::::None.into(), requirement: Requirement::Single(version_set_id), + extra: None.into(), } } @@ -633,6 +672,7 @@ pub extern "C" fn resolvo_conditional_requirement_union( ConditionalRequirement { condition: Option::::None.into(), requirement: Requirement::Union(version_set_union_id), + extra: None.into(), } } diff --git a/src/conflict.rs b/src/conflict.rs index 428caf8..1656209 100644 --- a/src/conflict.rs +++ b/src/conflict.rs @@ -212,6 +212,8 @@ impl Conflict { } } } + &Clause::RequiresWithExtra(..) => todo!(), + &Clause::ConditionalWithExtra(..) => todo!(), } } diff --git a/src/requirement.rs b/src/requirement.rs index 9641e27..b249262 100644 --- a/src/requirement.rs +++ b/src/requirement.rs @@ -1,4 +1,4 @@ -use crate::{Interner, VersionSetId, VersionSetUnionId}; +use crate::{Interner, StringId, VersionSetId, VersionSetUnionId}; use itertools::Itertools; use std::fmt::Display; @@ -10,14 +10,21 @@ pub struct ConditionalRequirement { pub condition: Option, /// The requirement that is only active when the condition is met. pub requirement: Requirement, + /// The extra that must be enabled for the requirement to be active. + pub extra: Option, } impl ConditionalRequirement { /// Creates a new conditional requirement. - pub fn new(condition: Option, requirement: Requirement) -> Self { + pub fn new( + condition: Option, + requirement: Requirement, + extra: Option, + ) -> Self { Self { condition, requirement, + extra, } } /// Returns the version sets that satisfy the requirement. @@ -49,6 +56,7 @@ impl From for ConditionalRequirement { Self { condition: None, requirement: value, + extra: None, } } } @@ -58,6 +66,7 @@ impl From for ConditionalRequirement { Self { condition: None, requirement: value.into(), + extra: None, } } } @@ -67,6 +76,7 @@ impl From for ConditionalRequirement { Self { condition: None, requirement: value.into(), + extra: None, } } } @@ -76,6 +86,7 @@ impl From<(VersionSetId, Option)> for ConditionalRequirement { Self { condition, requirement: requirement.into(), + extra: None, } } } diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 99bb693..038aaa1 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -83,6 +83,25 @@ pub(crate) enum Clause { /// We need to store the version set id because in the conflict graph, the version set id /// is used to identify the condition variable. Conditional(VariableId, VariableId, VersionSetId, Requirement), + /// A conditional clause that requires a feature to be enabled for the requirement to be active. + /// + /// In SAT terms: (¬A ∨ ¬C ∨ ¬F ∨ B1 ∨ B2 ∨ ... ∨ B99), where A is the solvable, + /// C is the condition, F is the feature, and B1 to B99 represent the possible candidates for + /// the provided [`Requirement`]. + ConditionalWithExtra( + VariableId, // solvable + VariableId, // condition + VersionSetId, // condition version set + VariableId, // extra + StringId, // extra name + Requirement, // requirement + ), + /// A requirement that requires a feature to be enabled for the requirement to be active. + /// + /// In SAT terms: (¬A ∨ ¬F ∨ B1 ∨ B2 ∨ ... ∨ B99), where A is the solvable, + /// F is the feature, and B1 to B99 represent the possible candidates for + /// the provided [`Requirement`]. + RequiresWithExtra(VariableId, VariableId, StringId, Requirement), /// Forbids the package on the right-hand side /// /// Note that the package on the left-hand side is not part of the clause, @@ -273,6 +292,100 @@ impl Clause { ) } + fn conditional_with_extra( + parent_id: VariableId, + requirement: Requirement, + condition_variable: VariableId, + condition_version_set_id: VersionSetId, + extra_variable: VariableId, + extra_name: StringId, + decision_tracker: &DecisionTracker, + requirement_candidates: impl IntoIterator, + ) -> (Self, Option<[Literal; 2]>, bool) { + assert_ne!(decision_tracker.assigned_value(parent_id), Some(false)); + let mut requirement_candidates = requirement_candidates.into_iter(); + + let requirement_literal = + if decision_tracker.assigned_value(condition_variable) == Some(true) { + // then ~condition is false + // if the feature is enabled, then we need to watch the requirement candidates + if decision_tracker.assigned_value(extra_variable) == Some(true) { + requirement_candidates + .find(|&id| decision_tracker.assigned_value(id) != Some(false)) + .map(|id| id.positive()) + } else { + // if the feature is disabled, then we need to watch the feature variable + Some(extra_variable.negative()) + } + } else { + None + }; + ( + Clause::ConditionalWithExtra( + parent_id, + condition_variable, + condition_version_set_id, + extra_variable, + extra_name, + requirement, + ), + Some([ + parent_id.negative(), + requirement_literal.unwrap_or(condition_variable.negative()), + ]), + requirement_literal.is_none() + && decision_tracker.assigned_value(condition_variable) == Some(true) + && decision_tracker.assigned_value(extra_variable) == Some(true), + ) + } + + fn requires_with_extra( + solvable_id: VariableId, + extra_variable: VariableId, + extra_name: StringId, + requirement: Requirement, + decision_tracker: &DecisionTracker, + requirement_candidates: impl IntoIterator, + ) -> (Self, Option<[Literal; 2]>, bool) { + // It only makes sense to introduce a requires clause when the parent solvable + // is undecided or going to be installed + assert_ne!(decision_tracker.assigned_value(solvable_id), Some(false)); + + let kind = Clause::RequiresWithExtra(solvable_id, extra_variable, extra_name, requirement); + let mut candidates = requirement_candidates.into_iter().peekable(); + let first_candidate = candidates.peek().copied(); + + if decision_tracker.assigned_value(extra_variable) == Some(true) { + // Feature is enabled, so watch the requirement candidates + if let Some(first_candidate) = first_candidate { + match candidates.find(|&c| decision_tracker.assigned_value(c) != Some(false)) { + // Watch any candidate that is not assigned to false + Some(watched_candidate) => ( + kind, + Some([solvable_id.negative(), watched_candidate.positive()]), + false, + ), + // All candidates are assigned to false - conflict + None => ( + kind, + Some([solvable_id.negative(), first_candidate.positive()]), + true, + ), + } + } else { + // No candidates available + (kind, None, false) + } + } else { + // Feature is not enabled, so watch the feature variable + ( + kind, + Some([solvable_id.negative(), extra_variable.negative()]), + false, + ) + } + } + /// Tries to fold over all the literals in the clause. /// /// This function is useful to iterate, find, or filter the literals in a @@ -326,6 +439,35 @@ impl Clause { ) .try_fold(init, visit) } + Clause::ConditionalWithExtra( + package_id, + condition_variable, + _, + extra_variable, + _, + requirement, + ) => iter::once(package_id.negative()) + .chain(iter::once(condition_variable.negative())) + .chain(iter::once(extra_variable.negative())) + .chain( + requirements_to_sorted_candidates[&requirement] + .iter() + .flatten() + .map(|&s| s.positive()), + ) + .try_fold(init, visit), + Clause::RequiresWithExtra(solvable_id, extra_variable, _, requirement) => { + iter::once(solvable_id.negative()) + .chain(iter::once(solvable_id.negative())) + .chain(iter::once(extra_variable.negative())) + .chain( + requirements_to_sorted_candidates[&requirement] + .iter() + .flatten() + .map(|&s| s.positive()), + ) + .try_fold(init, visit) + } } } @@ -502,6 +644,66 @@ impl WatchedLiterals { ) } + /// Shorthand method to construct a [Clause::ConditionalWithExtra] without requiring + /// complicated arguments. + /// + /// The returned boolean value is true when adding the clause resulted in a + /// conflict. + pub fn conditional_with_extra( + package_id: VariableId, + requirement: Requirement, + condition_variable: VariableId, + condition_version_set_id: VersionSetId, + extra_variable: VariableId, + extra_name: StringId, + decision_tracker: &DecisionTracker, + requirement_candidates: impl IntoIterator, + ) -> (Option, bool, Clause) { + let (kind, watched_literals, conflict) = Clause::conditional_with_extra( + package_id, + requirement, + condition_variable, + condition_version_set_id, + extra_variable, + extra_name, + decision_tracker, + requirement_candidates, + ); + ( + WatchedLiterals::from_kind_and_initial_watches(watched_literals), + conflict, + kind, + ) + } + + /// Shorthand method to construct a [Clause::RequiresWithExtra] without requiring + /// complicated arguments. + /// + /// The returned boolean value is true when adding the clause resulted in a + /// conflict. + pub fn requires_with_extra( + solvable_id: VariableId, + extra_variable: VariableId, + extra_name: StringId, + requirement: Requirement, + decision_tracker: &DecisionTracker, + requirement_candidates: impl IntoIterator, + ) -> (Option, bool, Clause) { + let (kind, watched_literals, conflict) = Clause::requires_with_extra( + solvable_id, + extra_variable, + extra_name, + requirement, + decision_tracker, + requirement_candidates, + ); + ( + WatchedLiterals::from_kind_and_initial_watches(watched_literals), + conflict, + kind, + ) + } + fn from_kind_and_initial_watches(watched_literals: Option<[Literal; 2]>) -> Option { let watched_literals = watched_literals?; debug_assert!(watched_literals[0] != watched_literals[1]); @@ -705,6 +907,37 @@ impl<'i, I: Interner> Display for ClauseDisplay<'i, I> { requirement.display(self.interner), ) } + Clause::ConditionalWithExtra( + package_id, + condition_variable, + _, + extra_variable, + _, + requirement, + ) => { + write!( + f, + "ConditionalWithExtra({}({:?}), {}({:?}), {}({:?}), {})", + package_id.display(self.variable_map, self.interner), + package_id, + condition_variable.display(self.variable_map, self.interner), + condition_variable, + extra_variable.display(self.variable_map, self.interner), + extra_variable, + requirement.display(self.interner), + ) + } + Clause::RequiresWithExtra(solvable_id, extra_variable, _, requirement) => { + write!( + f, + "RequiresWithExtra({}({:?}), {}({:?}), {})", + solvable_id.display(self.variable_map, self.interner), + solvable_id, + extra_variable.display(self.variable_map, self.interner), + extra_variable, + requirement.display(self.interner), + ) + } } } } diff --git a/src/solver/mod.rs b/src/solver/mod.rs index bda9bd4..9e2f3d8 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -22,7 +22,8 @@ use crate::{ requirement::ConditionalRequirement, runtime::{AsyncRuntime, NowOrNeverRuntime}, solver::binary_encoding::AtMostOnceTracker, - Candidates, Dependencies, DependencyProvider, KnownDependencies, Requirement, VersionSetId, + Candidates, Dependencies, DependencyProvider, KnownDependencies, Requirement, StringId, + VersionSetId, }; mod binary_encoding; @@ -37,7 +38,15 @@ mod watch_map; #[derive(Default)] struct AddClauseOutput { new_requires_clauses: Vec<(VariableId, Requirement, ClauseId)>, - new_conditional_clauses: Vec<(VariableId, VariableId, Requirement, ClauseId)>, + /// A vector of tuples from a solvable variable, conditional variable and extra(feature name) variable to + /// the clauses that need to be watched. + new_conditional_clauses: Vec<( + VariableId, + Option, + Option, + Requirement, + ClauseId, + )>, conflicting_clauses: Vec, negative_assertions: Vec<(VariableId, ClauseId)>, clauses_to_watch: Vec, @@ -152,8 +161,14 @@ pub struct Solver { pub(crate) clauses: Clauses, requires_clauses: IndexMap, ahash::RandomState>, - conditional_clauses: - IndexMap<(VariableId, VariableId), Vec<(Requirement, ClauseId)>, ahash::RandomState>, + + /// A map from a solvable variable, conditional variable and extra(feature name) variable to + /// the clauses that need to be watched. + conditional_clauses: IndexMap< + (VariableId, Option, Option), + Vec<(Requirement, ClauseId)>, + ahash::RandomState, + >, watches: WatchMap, /// A mapping from requirements to the variables that represent the @@ -666,11 +681,11 @@ impl Solver { .push((requirement, clause_id)); } - for (solvable_id, condition_variable, requirement, clause_id) in + for (solvable_id, condition_variable, extra_variable, requirement, clause_id) in output.new_conditional_clauses { self.conditional_clauses - .entry((solvable_id, condition_variable)) + .entry((solvable_id, condition_variable, extra_variable)) .or_default() .push((requirement, clause_id)); } @@ -791,6 +806,7 @@ impl Solver { ( solvable_id, None, + None, requirements .iter() .map(|(r, c)| (*r, *c)) @@ -801,15 +817,16 @@ impl Solver { let conditional_iter = self.conditional_clauses .iter() - .map(|((solvable_id, condition), clauses)| { + .map(|((solvable_id, condition, extra), clauses)| { ( *solvable_id, - Some(*condition), + *condition, + *extra, clauses.iter().map(|(r, c)| (*r, *c)).collect::>(), ) }); - for (solvable_id, condition, requirements) in requires_iter.chain(conditional_iter) { + for (solvable_id, condition, extra, requirements) in requires_iter.chain(conditional_iter) { let is_explicit_requirement = solvable_id == VariableId::root(); if let Some(best_decision) = &best_decision { @@ -837,6 +854,13 @@ impl Solver { } } + if let Some(extra_variable) = extra { + // If the extra is not enabled, skip this requirement entirely + if self.decision_tracker.assigned_value(extra_variable) != Some(true) { + continue; + } + } + for (requirement, clause_id) in requirements { let mut candidate = ControlFlow::Break(()); @@ -1589,6 +1613,7 @@ async fn add_clauses_for_solvables( solvable_id: SolvableOrRootId, requirement: Requirement, condition: Option<(SolvableId, VersionSetId)>, + extra: Option<(VariableId, StringId)>, candidates: Vec<&'i [SolvableId]>, }, NonMatchingCandidates { @@ -1759,37 +1784,83 @@ async fn add_clauses_for_solvables( // condition is `VersionSetId` right now but it will become a `Requirement` // in the next versions of resolvo - if let Some(condition) = conditional_requirement.condition { - let condition_candidates = - cache.get_or_cache_matching_candidates(condition).await?; - - for &condition_candidate in condition_candidates { - let candidates = candidates.clone(); + match ( + conditional_requirement.condition, + conditional_requirement.extra, + ) { + (None, Some(extra)) => { + let extra_variable = variable_map.intern_extra(extra); + + // Add a task result for the condition pending_futures.push( async move { Ok(TaskResult::SortedCandidates { solvable_id, requirement: conditional_requirement.requirement, - condition: Some((condition_candidate, condition)), - candidates, + condition: None, + extra: Some((extra_variable, extra)), + candidates: candidates.clone(), }) } .boxed_local(), ); } - } else { - // Add a task result for the condition - pending_futures.push( - async move { - Ok(TaskResult::SortedCandidates { - solvable_id, - requirement: conditional_requirement.requirement, - condition: None, - candidates: candidates.clone(), - }) + (Some(condition), Some(extra)) => { + let condition_candidates = + cache.get_or_cache_matching_candidates(condition).await?; + let extra_variable = variable_map.intern_extra(extra); + + for &condition_candidate in condition_candidates { + let candidates = candidates.clone(); + pending_futures.push( + async move { + Ok(TaskResult::SortedCandidates { + solvable_id, + requirement: conditional_requirement.requirement, + condition: Some((condition_candidate, condition)), + extra: Some((extra_variable, extra)), + candidates, + }) + } + .boxed_local(), + ); } - .boxed_local(), - ); + } + (Some(condition), None) => { + let condition_candidates = + cache.get_or_cache_matching_candidates(condition).await?; + + for &condition_candidate in condition_candidates { + let candidates = candidates.clone(); + pending_futures.push( + async move { + Ok(TaskResult::SortedCandidates { + solvable_id, + requirement: conditional_requirement.requirement, + condition: Some((condition_candidate, condition)), + extra: None, + candidates, + }) + } + .boxed_local(), + ); + } + } + (None, None) => { + // Add a task result for the condition + pending_futures.push( + async move { + Ok(TaskResult::SortedCandidates { + solvable_id, + requirement: conditional_requirement.requirement, + condition: None, + extra: None, + candidates: candidates.clone(), + }) + } + .boxed_local(), + ); + } } } @@ -1857,6 +1928,7 @@ async fn add_clauses_for_solvables( solvable_id, requirement, condition, + extra, candidates, } => { tracing::trace!( @@ -1926,69 +1998,152 @@ async fn add_clauses_for_solvables( ); } - if let Some((condition, condition_version_set_id)) = condition { - let condition_variable = variable_map.intern_solvable(condition); + match (condition, extra) { + ( + Some((condition_variable, condition_version_set_id)), + Some((extra_variable, extra_name)), + ) => { + let condition_variable = variable_map.intern_solvable(condition_variable); + + let (watched_literals, conflict, kind) = + WatchedLiterals::conditional_with_extra( + variable, + requirement, + condition_variable, + condition_version_set_id, + extra_variable, + extra_name, + decision_tracker, + version_set_variables.iter().flatten().copied(), + ); - // Add a condition clause - let (watched_literals, conflict, kind) = WatchedLiterals::conditional( - variable, - requirement, - condition_variable, - condition_version_set_id, - decision_tracker, - version_set_variables.iter().flatten().copied(), - ); + // Add the conditional clause + let no_candidates = + candidates.iter().all(|candidates| candidates.is_empty()); - // Add the conditional clause - let no_candidates = candidates.iter().all(|candidates| candidates.is_empty()); + let has_watches = watched_literals.is_some(); + let clause_id = clauses.alloc(watched_literals, kind); - let has_watches = watched_literals.is_some(); - let clause_id = clauses.alloc(watched_literals, kind); + if has_watches { + output.clauses_to_watch.push(clause_id); + } + + output.new_conditional_clauses.push(( + variable, + Some(condition_variable), + Some(extra_variable), + requirement, + clause_id, + )); - if has_watches { - output.clauses_to_watch.push(clause_id); + if conflict { + output.conflicting_clauses.push(clause_id); + } else if no_candidates { + // Add assertions for unit clauses (i.e. those with no matching candidates) + output.negative_assertions.push((variable, clause_id)); + } } + (None, Some((extra_variable, extra_name))) => { + let (watched_literals, conflict, kind) = + WatchedLiterals::requires_with_extra( + variable, + extra_variable, + extra_name, + requirement, + decision_tracker, + version_set_variables.iter().flatten().copied(), + ); - output.new_conditional_clauses.push(( - variable, - condition_variable, - requirement, - clause_id, - )); + // Add the requirements clause + let no_candidates = + candidates.iter().all(|candidates| candidates.is_empty()); - if conflict { - output.conflicting_clauses.push(clause_id); - } else if no_candidates { - // Add assertions for unit clauses (i.e. those with no matching candidates) - output.negative_assertions.push((variable, clause_id)); + let has_watches = watched_literals.is_some(); + let clause_id = clauses.alloc(watched_literals, kind); + + if has_watches { + output.clauses_to_watch.push(clause_id); + } + + output + .new_requires_clauses + .push((variable, requirement, clause_id)); + + if conflict { + output.conflicting_clauses.push(clause_id); + } else if no_candidates { + // Add assertions for unit clauses (i.e. those with no matching candidates) + output.negative_assertions.push((variable, clause_id)); + } } - } else { - let (watched_literals, conflict, kind) = WatchedLiterals::requires( - variable, - requirement, - version_set_variables.iter().flatten().copied(), - decision_tracker, - ); + (Some((condition, condition_version_set_id)), None) => { + let condition_variable = variable_map.intern_solvable(condition); + + // Add a condition clause + let (watched_literals, conflict, kind) = WatchedLiterals::conditional( + variable, + requirement, + condition_variable, + condition_version_set_id, + decision_tracker, + version_set_variables.iter().flatten().copied(), + ); - // Add the requirements clause - let no_candidates = candidates.iter().all(|candidates| candidates.is_empty()); + // Add the conditional clause + let no_candidates = + candidates.iter().all(|candidates| candidates.is_empty()); - let has_watches = watched_literals.is_some(); - let clause_id = clauses.alloc(watched_literals, kind); + let has_watches = watched_literals.is_some(); + let clause_id = clauses.alloc(watched_literals, kind); + + if has_watches { + output.clauses_to_watch.push(clause_id); + } + + output.new_conditional_clauses.push(( + variable, + Some(condition_variable), + None, + requirement, + clause_id, + )); - if has_watches { - output.clauses_to_watch.push(clause_id); + if conflict { + output.conflicting_clauses.push(clause_id); + } else if no_candidates { + // Add assertions for unit clauses (i.e. those with no matching candidates) + output.negative_assertions.push((variable, clause_id)); + } } + (None, None) => { + let (watched_literals, conflict, kind) = WatchedLiterals::requires( + variable, + requirement, + version_set_variables.iter().flatten().copied(), + decision_tracker, + ); - output - .new_requires_clauses - .push((variable, requirement, clause_id)); + // Add the requirements clause + let no_candidates = + candidates.iter().all(|candidates| candidates.is_empty()); - if conflict { - output.conflicting_clauses.push(clause_id); - } else if no_candidates { - // Add assertions for unit clauses (i.e. those with no matching candidates) - output.negative_assertions.push((variable, clause_id)); + let has_watches = watched_literals.is_some(); + let clause_id = clauses.alloc(watched_literals, kind); + + if has_watches { + output.clauses_to_watch.push(clause_id); + } + + output + .new_requires_clauses + .push((variable, requirement, clause_id)); + + if conflict { + output.conflicting_clauses.push(clause_id); + } else if no_candidates { + // Add assertions for unit clauses (i.e. those with no matching candidates) + output.negative_assertions.push((variable, clause_id)); + } } } } diff --git a/src/solver/variable_map.rs b/src/solver/variable_map.rs index 608ed68..286f187 100644 --- a/src/solver/variable_map.rs +++ b/src/solver/variable_map.rs @@ -7,7 +7,7 @@ use crate::{ arena::ArenaId, id::{SolvableOrRootId, VariableId}, }, - Interner, NameId, SolvableId, + Interner, NameId, SolvableId, StringId, }; /// All variables in the solver are stored in a `VariableMap`. This map is used @@ -23,6 +23,9 @@ pub struct VariableMap { /// A map from solvable id to variable id. solvable_to_variable: HashMap, + /// A map from extra name to variable id. + extra_to_variable: HashMap, + /// Records the origins of all variables. origins: HashMap, } @@ -38,6 +41,9 @@ pub enum VariableOrigin { /// A variable that helps encode an at most one constraint. ForbidMultiple(NameId), + + /// A variable that represents an extra variable. + Extra(StringId), } impl Default for VariableMap { @@ -48,6 +54,7 @@ impl Default for VariableMap { Self { next_id: 1, // The first variable id is 1 because 0 is reserved for the root. solvable_to_variable: HashMap::default(), + extra_to_variable: HashMap::default(), origins, } } @@ -78,6 +85,22 @@ impl VariableMap { } } + /// Allocate a variable that represents an extra variable. + pub fn intern_extra(&mut self, extra_name: StringId) -> VariableId { + match self.extra_to_variable.entry(extra_name) { + Entry::Occupied(entry) => *entry.get(), + Entry::Vacant(entry) => { + let id = self.next_id; + self.next_id += 1; + let variable_id = VariableId::from_usize(id); + entry.insert(variable_id); + self.origins + .insert(variable_id, VariableOrigin::Extra(extra_name)); + variable_id + } + } + } + /// Allocate a variable that helps encode an at most one constraint. pub fn alloc_forbid_multiple_variable(&mut self, name: NameId) -> VariableId { let id = self.next_id; @@ -141,6 +164,9 @@ impl<'i, I: Interner> Display for VariableDisplay<'i, I> { VariableOrigin::ForbidMultiple(name) => { write!(f, "forbid-multiple({})", self.interner.display_name(name)) } + VariableOrigin::Extra(extra_name) => { + write!(f, "extra({})", self.interner.display_string(extra_name)) + } } } } From 5fd153ae43d3ac63f19c06a04a172a74b62b3a1b Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Wed, 29 Jan 2025 14:06:02 -0500 Subject: [PATCH 41/41] run fmt and fix clippy errors --- src/solver/clause.rs | 2 ++ src/solver/mod.rs | 25 +++++++++++++------------ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/solver/clause.rs b/src/solver/clause.rs index 038aaa1..8b9d40b 100644 --- a/src/solver/clause.rs +++ b/src/solver/clause.rs @@ -292,6 +292,7 @@ impl Clause { ) } + #[allow(clippy::too_many_arguments)] fn conditional_with_extra( parent_id: VariableId, requirement: Requirement, @@ -649,6 +650,7 @@ impl WatchedLiterals { /// /// The returned boolean value is true when adding the clause resulted in a /// conflict. + #[allow(clippy::too_many_arguments)] pub fn conditional_with_extra( package_id: VariableId, requirement: Requirement, diff --git a/src/solver/mod.rs b/src/solver/mod.rs index 9e2f3d8..f142fa1 100644 --- a/src/solver/mod.rs +++ b/src/solver/mod.rs @@ -35,18 +35,21 @@ mod decision_tracker; pub(crate) mod variable_map; mod watch_map; +/// The output of the `add_clauses_for_solvables` function. +type AddConditionalClauseOutput = ( + VariableId, + Option, + Option, + Requirement, + ClauseId, +); + #[derive(Default)] struct AddClauseOutput { new_requires_clauses: Vec<(VariableId, Requirement, ClauseId)>, /// A vector of tuples from a solvable variable, conditional variable and extra(feature name) variable to /// the clauses that need to be watched. - new_conditional_clauses: Vec<( - VariableId, - Option, - Option, - Requirement, - ClauseId, - )>, + new_conditional_clauses: Vec, conflicting_clauses: Vec, negative_assertions: Vec<(VariableId, ClauseId)>, clauses_to_watch: Vec, @@ -153,6 +156,7 @@ impl Clauses { } type RequirementCandidateVariables = Vec>; +type ConditionalClauseMap = (VariableId, Option, Option); /// Drives the SAT solving process. pub struct Solver { @@ -164,11 +168,8 @@ pub struct Solver { /// A map from a solvable variable, conditional variable and extra(feature name) variable to /// the clauses that need to be watched. - conditional_clauses: IndexMap< - (VariableId, Option, Option), - Vec<(Requirement, ClauseId)>, - ahash::RandomState, - >, + conditional_clauses: + IndexMap, ahash::RandomState>, watches: WatchMap, /// A mapping from requirements to the variables that represent the