From b95b7896ec5abba2f8624be2f382ce6943b9c549 Mon Sep 17 00:00:00 2001 From: prsabahrami Date: Thu, 23 Jan 2025 23:27:23 -0500 Subject: [PATCH] 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);