feat!: add support for conditional dependencies #136
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@prsabahrami @jjerphan If you have time I would appreciate a few more eyes on this PR! 👍 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for conditional dependencies by introducing new types and methods to capture conditions and their conversion into SAT clauses. Key changes include:
- Adding new data structures (e.g. ConditionalSpec, ConditionalRequirement, Condition, LogicalOperator) to encode conditions.
- Extending the solver internals to handle conditional requirements via disjunctions and auxiliary variables.
- Updating C++ bindings and tests to account for the new conditional dependency support.
Reviewed Changes
Copilot reviewed 59 out of 59 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/solver/bundle_box/conditional_spec.rs | New test support for conditional dependencies. |
| tests/snapshots/solver__root_constraints.snap | Snapshot changes reflecting updated solver behavior. |
| src/solver/variable_map.rs | Added variable variant and allocation function for at-least-one semantics. |
| src/solver/mod.rs | Updated API and internal state to carry ConditionalRequirement and disjunctions. |
| src/solver/encoding.rs | Major changes to encode conditional requirements including disjunction conversion and clause registration. |
| src/solver/conditions.rs | New module implementing condition conversion to DNF and related types. |
| src/solver/clause.rs | Modified clause representation to support optional condition disjunctions. |
| Other files (binary_encoding.rs, conditional_requirement.rs, C++ bindings, Cargo.toml, etc.) | Adapted to support conditional dependencies and updated language features. |
Comments suppressed due to low confidence (2)
src/solver/encoding.rs:292
- [nitpick] The variable name 'conditions' is used to store disjunction IDs produced from the DNF conversion of a requirement condition. To improve clarity, consider renaming it to 'disjunction_ids' or similar.
let mut conditions = Vec::with_capacity(condition.as_ref().map_or(0, |(_, dnf)| dnf.len()));
cpp/src/lib.rs:646
- [nitpick] Using .cloned() instead of .copied() is appropriate here because ConditionalRequirement may not implement Copy; verify that this change aligns with the intended type traits for ConditionalRequirement.
.cloned()
4 tasks
tdejager
requested changes
May 15, 2025
Comment on lines
+54
to
+55
| //! in DNF if it is a **disjunction (OR)** of one or more **conjunctive clauses | ||
| //! (AND)**. |
There was a problem hiding this comment.
Suggested change
| //! in DNF if it is a **disjunction (OR)** of one or more **conjunctive clauses | |
| //! (AND)**. | |
| //! in DNF if the operands on the variables inside the clauses are **AND** while the clauses themselves are connected by **OR**'s |
There was a problem hiding this comment.
I first interpreted the other as the other way around, this is my attempt to clarify while straying from the regular nomenclature. You decide!
Comment on lines
+8
to
+16
| // You can also use just a single number for a range like `package 0` which | ||
| // means the range from 0..1 (excluding the end) | ||
| // | ||
| // Lets call the tuples of (Name, Version) a `Pack` and the tuples of (Name, | ||
| // Ranges<u32>) a `Spec` | ||
| // | ||
| // We also need to create a custom provider that tells us how to sort the | ||
| // candidates. This is unique to each packaging ecosystem. Let's call our | ||
| // ecosystem 'BundleBox' so that how we call the provider as well. |
There was a problem hiding this comment.
I think this description should be expanded to include conditionals :)
| /// boolean formula in disjunctive normal form (DNF). Each inner Vec represents | ||
| /// a conjunction (AND group) and the outer Vec represents the disjunction (OR | ||
| /// group). | ||
| pub fn convert_conditions_to_dnf<I: Interner>( |
There was a problem hiding this comment.
Could we make some unit tests for these :)?
| #[test] | ||
| fn test_conditional_complex() { | ||
| let mut provider = BundleBoxProvider::from_packages(&[ | ||
| ("foo", 1, vec!["icon; if bar and baz or menu"]), |
There was a problem hiding this comment.
Maybe another test with menu instead of bar and baz just to be sure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for conditional dependencies. A conditional dependency is a dependency on a package that is only active if another requirement is satisfied. E.g.
A requires B if C. Conditions can be combined withandandor. This allows depending on packages that are only active in certain scenarios, e.g.linux-specific; if __linuxnumpy <=1.22; if python<3.12,numpy >1.22; if python >=3.13).Care is taken that conditional dependencies only incur an overhead when used. So this PR should not affect the performance of problems without conditional dependencies. Even when they are used the overhead should be minimal.
I've added a lot of documentation that describes how conditional dependencies are implemented.
Todo