feat: add support for conditional dependencies #101
Conversation
|
@baszalmstra In the |
|
Mmm since we iterate over all requirements to find the best requirement to make a decision on I think it might be the case that we decide on a conditional requirement before all requirements have been decided on. You have to early out if a decision is already available for a non-conditional requirement. |
|
Did you already add tests? I think that will help finding edge cases early! |
This reverts commit 0426ba3.
src/solver/clause.rs
Outdated
| Clause::Conditional(package_id, condition, requirement) => { | ||
| iter::once(package_id.negative()) | ||
| .chain( | ||
| requirements_to_sorted_candidates[&condition.into()] |
There was a problem hiding this comment.
This doesnt need to be sorted at all. :)
There was a problem hiding this comment.
I've added another map to DependencyProvider which maps VersionSetId to VariableIds. The cache did not directly provide this and I don't think we need it in the cache as we are only using it in two places and so it would make more sense to have them on the DependencyProvider side.
There was a problem hiding this comment.
I assume you mean the Solver instead of the DependencyProvider?
Just a thought but Im wondering if we ever care about the solvables instead of the variables? Perhaps it makes sense to store that in the cache instead?
tests/solver.rs
Outdated
| 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()); |
There was a problem hiding this comment.
Ideally you add a little string conversion for parsing this so you can do e.g. c 1; if b 1..2 that would make the tests more readable.
| } | ||
|
|
||
| #[test] | ||
| fn test_circular_conditional_dependencies() { |
There was a problem hiding this comment.
I think you should add a few tests that include more versions for a single package.
|
When I look at the tests, it looks like it's hitting some asserts? :) |
There was a problem hiding this comment.
I was thinking, there might be a bug in the code as is.
Given the conditional requirement:
A requires C 1..3; if B 1..2
e.g.
A∧(B1∨B2)⟹C1∨C2∨C3
in CNF form that becomes:
¬A∨¬B1∨C1∨C2∨C3
¬A∨¬B2∨C1∨C2∨C3
But currently we convert it to one clause:
¬A∨¬B1 v ¬B2∨C1∨C2∨C3
Which will always evaluate to true if any version of B is selected.
…o have multiple clauses per version
|
@baszalmstra I have added two tests with more versions. One is the example you provided above
In this example, |
No description provided.