From cd3c9a30589ddcb6c38de8c197ccc81201093a0f Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Fri, 31 Jan 2025 15:34:06 -0500 Subject: [PATCH] fix the conditionals' text printing --- src/conflict.rs | 285 +++++++++++++++++++++++++++++++++++------------- tests/solver.rs | 44 +++++--- 2 files changed, 236 insertions(+), 93 deletions(-) diff --git a/src/conflict.rs b/src/conflict.rs index 2dc4af4..3d08547 100644 --- a/src/conflict.rs +++ b/src/conflict.rs @@ -263,7 +263,7 @@ impl Conflict { } /// A node in the graph representation of a [`Conflict`] -#[derive(Copy, Clone, Eq, PartialEq)] +#[derive(Copy, Clone, Eq, PartialEq, Debug)] pub(crate) enum ConflictNode { /// Node corresponding to a solvable Solvable(SolvableOrRootId), @@ -292,7 +292,7 @@ impl ConflictNode { } /// An edge in the graph representation of a [`Conflict`] -#[derive(Clone, Copy, Hash, Eq, PartialEq, Ord, PartialOrd)] +#[derive(Clone, Copy, Hash, Eq, PartialEq, Ord, PartialOrd, Debug)] pub(crate) enum ConflictEdge { /// The target node is a candidate for the dependency specified by the /// [`Requirement`] @@ -304,19 +304,21 @@ pub(crate) enum ConflictEdge { } impl ConflictEdge { - fn try_requires(self) -> Option { + fn try_requires_or_conditional(self) -> Option<(Requirement, Option)> { match self { - ConflictEdge::Requires(match_spec_id) => Some(match_spec_id), - ConflictEdge::ConditionalRequires(_, _) => None, + ConflictEdge::Requires(match_spec_id) => Some((match_spec_id, None)), + ConflictEdge::ConditionalRequires(version_set_id, match_spec_id) => { + Some((match_spec_id, Some(version_set_id))) + } ConflictEdge::Conflict(_) => None, } } - fn requires(self) -> Requirement { + fn requires_or_conditional(self) -> (Requirement, Option) { match self { - ConflictEdge::Requires(match_spec_id) => match_spec_id, - ConflictEdge::ConditionalRequires(_, _) => { - panic!("expected requires edge, found conditional requires") + ConflictEdge::Requires(match_spec_id) => (match_spec_id, None), + ConflictEdge::ConditionalRequires(version_set_id, match_spec_id) => { + (match_spec_id, Some(version_set_id)) } ConflictEdge::Conflict(_) => panic!("expected requires edge, found conflict"), } @@ -324,7 +326,7 @@ impl ConflictEdge { } /// Conflict causes -#[derive(Copy, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)] +#[derive(Copy, Clone, Hash, Eq, PartialEq, Ord, PartialOrd, Debug)] pub(crate) enum ConflictCause { /// The solvable is locked Locked(SolvableId), @@ -334,8 +336,6 @@ pub(crate) enum ConflictCause { ForbidMultipleInstances, /// The node was excluded Excluded, - /// The condition for the dependency is not met - ConditionNotMet(VersionSetId), } /// Represents a node that has been merged with others @@ -421,12 +421,6 @@ impl ConflictGraph { requirement.display(interner) ) } - ConflictEdge::Conflict(ConflictCause::ConditionNotMet(version_set_id)) => { - format!( - "condition not met: {}", - interner.display_version_set(*version_set_id) - ) - } ConflictEdge::Conflict(ConflictCause::Constrains(version_set_id)) => { interner.display_version_set(*version_set_id).to_string() } @@ -572,13 +566,15 @@ impl ConflictGraph { .graph .edges_directed(nx, Direction::Outgoing) .map(|e| match e.weight() { - ConflictEdge::Requires(req) => (req, e.target()), - ConflictEdge::ConditionalRequires(_, req) => (req, e.target()), + ConflictEdge::Requires(req) => ((req, None), e.target()), + ConflictEdge::ConditionalRequires(condition, req) => { + ((req, Some(condition)), e.target()) + } ConflictEdge::Conflict(_) => unreachable!(), }) .collect::>() .into_iter() - .chunk_by(|(&version_set_id, _)| version_set_id); + .chunk_by(|((&version_set_id, condition), _)| (version_set_id, *condition)); for (_, mut deps) in &dependencies { if deps.all(|(_, target)| !installable.contains(&target)) { @@ -621,15 +617,15 @@ 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(version_set_id) => ((version_set_id, None), e.target()), + ConflictEdge::ConditionalRequires(condition, version_set_id) => { + ((version_set_id, Some(condition)), e.target()) } ConflictEdge::Conflict(_) => unreachable!(), }) .collect::>() .into_iter() - .chunk_by(|(&version_set_id, _)| version_set_id); + .chunk_by(|((&version_set_id, condition), _)| (version_set_id, *condition)); // Missing if at least one dependency is missing if dependencies @@ -716,42 +712,6 @@ impl Indenter { } } -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_indenter_without_top_level_indent() { - let indenter = Indenter::new(false); - - let indenter = indenter.push_level_with_order(ChildOrder::Last); - assert_eq!(indenter.get_indent(), ""); - - let indenter = indenter.push_level_with_order(ChildOrder::Last); - assert_eq!(indenter.get_indent(), "└─ "); - } - - #[test] - fn test_indenter_with_multiple_siblings() { - let indenter = Indenter::new(true); - - let indenter = indenter.push_level_with_order(ChildOrder::Last); - assert_eq!(indenter.get_indent(), "└─ "); - - let indenter = indenter.push_level_with_order(ChildOrder::HasRemainingSiblings); - assert_eq!(indenter.get_indent(), " ├─ "); - - let indenter = indenter.push_level_with_order(ChildOrder::Last); - assert_eq!(indenter.get_indent(), " │ └─ "); - - let indenter = indenter.push_level_with_order(ChildOrder::Last); - assert_eq!(indenter.get_indent(), " │ └─ "); - - let indenter = indenter.push_level_with_order(ChildOrder::HasRemainingSiblings); - assert_eq!(indenter.get_indent(), " │ ├─ "); - } -} - /// A struct implementing [`fmt::Display`] that generates a user-friendly /// representation of a conflict graph pub struct DisplayUnsat<'i, I: Interner> { @@ -784,11 +744,13 @@ impl<'i, I: Interner> DisplayUnsat<'i, I> { top_level_indent: bool, ) -> fmt::Result { pub enum DisplayOp { + ConditionalRequirement((Requirement, VersionSetId), Vec), Requirement(Requirement, Vec), Candidate(NodeIndex), } let graph = &self.graph.graph; + println!("graph {:?}", graph); let installable_nodes = &self.installable_set; let mut reported: HashSet = HashSet::new(); @@ -796,21 +758,26 @@ impl<'i, I: Interner> DisplayUnsat<'i, I> { let indenter = Indenter::new(top_level_indent); let mut stack = top_level_edges .iter() - .filter(|e| e.weight().try_requires().is_some()) - .chunk_by(|e| e.weight().requires()) + .filter(|e| e.weight().try_requires_or_conditional().is_some()) + .chunk_by(|e| e.weight().requires_or_conditional()) .into_iter() - .map(|(version_set_id, group)| { + .map(|(version_set_id_with_condition, group)| { let edges: Vec<_> = group.map(|e| e.id()).collect(); - (version_set_id, edges) + (version_set_id_with_condition, edges) }) - .sorted_by_key(|(_version_set_id, edges)| { + .sorted_by_key(|(_version_set_id_with_condition, edges)| { edges .iter() .any(|&edge| installable_nodes.contains(&graph.edge_endpoints(edge).unwrap().1)) }) - .map(|(version_set_id, edges)| { + .map(|((version_set_id, condition), edges)| { ( - DisplayOp::Requirement(version_set_id, edges), + if let Some(condition) = condition { + println!("conditional requirement"); + DisplayOp::ConditionalRequirement((version_set_id, condition), edges) + } else { + DisplayOp::Requirement(version_set_id, edges) + }, indenter.push_level(), ) }) @@ -1044,7 +1011,7 @@ impl<'i, I: Interner> DisplayUnsat<'i, I> { writeln!(f, "{indent}{version} would require",)?; let mut requirements = graph .edges(candidate) - .chunk_by(|e| e.weight().requires()) + .chunk_by(|e| e.weight().requires_or_conditional()) .into_iter() .map(|(version_set_id, group)| { let edges: Vec<_> = group.map(|e| e.id()).collect(); @@ -1056,9 +1023,16 @@ impl<'i, I: Interner> DisplayUnsat<'i, I> { .contains(&graph.edge_endpoints(edge).unwrap().1) }) }) - .map(|(version_set_id, edges)| { + .map(|((version_set_id, condition), edges)| { ( - DisplayOp::Requirement(version_set_id, edges), + if let Some(condition) = condition { + DisplayOp::ConditionalRequirement( + (version_set_id, condition), + edges, + ) + } else { + DisplayOp::Requirement(version_set_id, edges) + }, indenter.push_level(), ) }) @@ -1071,6 +1045,132 @@ impl<'i, I: Interner> DisplayUnsat<'i, I> { stack.extend(requirements); } } + DisplayOp::ConditionalRequirement((requirement, condition), edges) => { + debug_assert!(!edges.is_empty()); + + let installable = edges.iter().any(|&e| { + let (_, target) = graph.edge_endpoints(e).unwrap(); + installable_nodes.contains(&target) + }); + + let req = requirement.display(self.interner).to_string(); + let condition = self.interner.display_version_set(condition); + + let target_nx = graph.edge_endpoints(edges[0]).unwrap().1; + let missing = + edges.len() == 1 && graph[target_nx] == ConflictNode::UnresolvedDependency; + if missing { + // No candidates for requirement + if top_level { + writeln!(f, "{indent} the condition {condition} is true but no candidates were found for {req}.")?; + } else { + writeln!(f, "{indent}{req}, for which no candidates were found.",)?; + } + } else if installable { + // Package can be installed (only mentioned for top-level requirements) + if top_level { + writeln!( + f, + "{indent}due to the condition {condition}, {req} can be installed with any of the following options:" + )?; + } else { + writeln!(f, "{indent}{req}, which can be installed with any of the following options:")?; + } + + let children: Vec<_> = edges + .iter() + .filter(|&&e| { + installable_nodes.contains(&graph.edge_endpoints(e).unwrap().1) + }) + .map(|&e| { + ( + DisplayOp::Candidate(graph.edge_endpoints(e).unwrap().1), + indenter.push_level(), + ) + }) + .collect(); + + // TODO: this is an utterly ugly hack that should be burnt to ashes + let mut deduplicated_children = Vec::new(); + let mut merged_and_seen = HashSet::new(); + for child in children { + let (DisplayOp::Candidate(child_node), _) = child else { + unreachable!() + }; + let solvable_id = graph[child_node].solvable_or_root(); + let Some(solvable_id) = solvable_id.solvable() else { + continue; + }; + + let merged = self.merged_candidates.get(&solvable_id); + + // Skip merged stuff that we have already seen + if merged_and_seen.contains(&solvable_id) { + continue; + } + + if let Some(merged) = merged { + merged_and_seen.extend(merged.ids.iter().copied()) + } + + deduplicated_children.push(child); + } + + if !deduplicated_children.is_empty() { + deduplicated_children[0].1.set_last(); + } + + stack.extend(deduplicated_children); + } else { + // Package cannot be installed (the conflicting requirement is further down + // the tree) + if top_level { + writeln!(f, "{indent}The condition {condition} is true but {req} cannot be installed because there are no viable options:")?; + } else { + writeln!(f, "{indent}{req}, which cannot be installed because there are no viable options:")?; + } + + let children: Vec<_> = edges + .iter() + .map(|&e| { + ( + DisplayOp::Candidate(graph.edge_endpoints(e).unwrap().1), + indenter.push_level(), + ) + }) + .collect(); + + // TODO: this is an utterly ugly hack that should be burnt to ashes + let mut deduplicated_children = Vec::new(); + let mut merged_and_seen = HashSet::new(); + for child in children { + let (DisplayOp::Candidate(child_node), _) = child else { + unreachable!() + }; + let Some(solvable_id) = graph[child_node].solvable() else { + continue; + }; + let merged = self.merged_candidates.get(&solvable_id); + + // Skip merged stuff that we have already seen + if merged_and_seen.contains(&solvable_id) { + continue; + } + + if let Some(merged) = merged { + merged_and_seen.extend(merged.ids.iter().copied()) + } + + deduplicated_children.push(child); + } + + if !deduplicated_children.is_empty() { + deduplicated_children[0].1.set_last(); + } + + stack.extend(deduplicated_children); + } + } } } @@ -1132,13 +1232,6 @@ impl<'i, I: Interner> fmt::Display for DisplayUnsat<'i, I> { self.interner.display_merged_solvables(&[solvable_id]), )?; } - ConflictCause::ConditionNotMet(version_set_id) => { - writeln!( - f, - "{indent}condition not met: {}", - self.interner.display_version_set(*version_set_id), - )?; - } ConflictCause::Excluded => continue, }; } @@ -1147,3 +1240,39 @@ impl<'i, I: Interner> fmt::Display for DisplayUnsat<'i, I> { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_indenter_without_top_level_indent() { + let indenter = Indenter::new(false); + + let indenter = indenter.push_level_with_order(ChildOrder::Last); + assert_eq!(indenter.get_indent(), ""); + + let indenter = indenter.push_level_with_order(ChildOrder::Last); + assert_eq!(indenter.get_indent(), "└─ "); + } + + #[test] + fn test_indenter_with_multiple_siblings() { + let indenter = Indenter::new(true); + + let indenter = indenter.push_level_with_order(ChildOrder::Last); + assert_eq!(indenter.get_indent(), "└─ "); + + let indenter = indenter.push_level_with_order(ChildOrder::HasRemainingSiblings); + assert_eq!(indenter.get_indent(), " ├─ "); + + let indenter = indenter.push_level_with_order(ChildOrder::Last); + assert_eq!(indenter.get_indent(), " │ └─ "); + + let indenter = indenter.push_level_with_order(ChildOrder::Last); + assert_eq!(indenter.get_indent(), " │ └─ "); + + let indenter = indenter.push_level_with_order(ChildOrder::HasRemainingSiblings); + assert_eq!(indenter.get_indent(), " │ ├─ "); + } +} diff --git a/tests/solver.rs b/tests/solver.rs index 6de3c4d..7650bb9 100644 --- a/tests/solver.rs +++ b/tests/solver.rs @@ -1696,7 +1696,12 @@ fn test_conditional_requirements_conflict() { provider.add_package("d", 1.into(), &[], &[]); provider.add_package("d", 2.into(), &[], &[]); - provider.add_package("a", 1.into(), &["b 1", "c 1; if b 1", "d 2", "c 2; if b 2"], &[]); + provider.add_package( + "a", + 1.into(), + &["b 1", "c 1; if b 1", "d 2", "c 2; if b 2"], + &[], + ); let requirements = provider.requirements(&[ "a", // Require package a @@ -1710,20 +1715,29 @@ fn test_conditional_requirements_conflict() { // 2. c 1 requires d 1, but a requires d 2 // 3. d 1 and d 2 cannot be installed together - let solved = solver.solve(problem); - assert!(solved.is_err()); - - let conflict = solved.unwrap_err(); - match conflict { - UnsolvableOrCancelled::Unsolvable(conflict) => { - let graph = conflict.graph(&solver); - let mut output = stderr(); - graph - .graphviz(&mut output, solver.provider(), true) - .unwrap(); - } - _ => panic!("Expected a conflict"), - } + let solved = solver + .solve(problem) + .map_err(|e| match e { + UnsolvableOrCancelled::Unsolvable(conflict) => { + conflict.display_user_friendly(&solver).to_string() + } + UnsolvableOrCancelled::Cancelled(_) => "kir".to_string(), + }) + .unwrap_err(); + + assert_snapshot!(solved, @r" + The following packages are incompatible + └─ a * cannot be installed because there are no viable options: + └─ a 1 would require + ├─ b >=1, <2, which can be installed with any of the following options: + │ └─ b 1 + ├─ d >=2, <3, which can be installed with any of the following options: + │ └─ d 2 + └─ c >=1, <2, which cannot be installed because there are no viable options: + └─ c 1 would require + └─ d >=1, <2, which cannot be installed because there are no viable options: + └─ d 1, which conflicts with the versions reported above. + "); } /// In this test, the resolver installs the highest available version of b which is b 2