Skip to content

Commit

Permalink
fix bug in the solver
Browse files Browse the repository at this point in the history
  • Loading branch information
@prsabahrami
prsabahrami committed Jan 27, 2025
1 parent 6dd7455 commit 057989b
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 84 deletions.
6 changes: 6 additions & 0 deletions src/internal/id.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ impl ArenaId for StringId {
#[cfg_attr(feature = "serde", serde(transparent))]
pub struct VersionSetId(pub u32);

impl From<(VersionSetId, Option<VersionSetId>)> for VersionSetId {
fn from((id, _): (VersionSetId, Option<VersionSetId>)) -> Self {
id
}
}

impl ArenaId for VersionSetId {
fn from_usize(x: usize) -> Self {
Self(x as u32)
Expand Down
9 changes: 9 additions & 0 deletions src/requirement.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,15 @@ impl From<VersionSetUnionId> for ConditionalRequirement {
}
}

impl From<(VersionSetId, Option<VersionSetId>)> for ConditionalRequirement {
fn from((requirement, condition): (VersionSetId, Option<VersionSetId>)) -> 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))]
Expand Down
31 changes: 15 additions & 16 deletions src/solver/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -814,12 +814,9 @@ impl<D: DependencyProvider, RT: AsyncRuntime> Solver<D, RT> {
clauses.iter().map(|(r, c)| (*r, *c)).collect::<Vec<_>>(),
)
});

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
Expand All @@ -836,15 +833,24 @@ impl<D: DependencyProvider, RT: AsyncRuntime> Solver<D, RT> {

// 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;
}
}
Expand Down Expand Up @@ -1936,13 +1942,6 @@ async fn add_clauses_for_solvables<D: DependencyProvider>(
}

if let Some((condition, condition_candidates)) = condition {
tracing::trace!(
"Adding conditional clauses for {} with condition {}",
requirement.display(cache.provider()),
std::convert::Into::<Requirement>::into(condition)
.display(cache.provider()),
);

let condition_version_set_variables = version_set_to_variables.insert(
condition,
condition_candidates
Expand Down
103 changes: 35 additions & 68 deletions tests/solver.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,11 +217,16 @@ impl BundleBoxProvider {
.expect("package missing")
}

pub fn requirements<R: From<VersionSetId>>(&self, requirements: &[&str]) -> Vec<R> {
pub fn requirements<R: From<(VersionSetId, Option<VersionSetId>)>>(&self, requirements: &[&str]) -> Vec<R> {
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()
}
Expand Down Expand Up @@ -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
"###);
}

Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down

0 comments on commit 057989b

Please sign in to comment.