-
Notifications
You must be signed in to change notification settings - Fork 109
feat: parse matchspec conditions and translate to resolvo #1545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements parsing and translation of conditional dependencies in MatchSpecs according to the conda CEP specification. It enables dependencies to be conditionally included based on conditions like Python version or virtual packages using syntax such as numpy; if python >=3.12 or foobar; if __unix.
- Adds parsing support for condition syntax in MatchSpecs with logical operators (and, or) and parentheses
- Implements translation from parsed conditions to resolvo's conditional requirement system
- Updates dependency processing to handle conditional requirements throughout the solver pipeline
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rattler_conda_types/src/match_spec/condition.rs | New module implementing condition parsing with logical operators and MatchSpec evaluation |
| crates/rattler_conda_types/src/match_spec/parse.rs | Updated parser to extract and parse condition clauses from MatchSpec strings |
| crates/rattler_conda_types/src/match_spec/mod.rs | Added condition field to MatchSpec struct and Display implementation |
| crates/rattler_solve/src/resolvo/mod.rs | Modified dependency provider to translate conditions and handle conditional requirements |
| crates/rattler_solve/tests/backends.rs | Added comprehensive test for conditional dependency resolution |
| crates/rattler_pty/src/unix/pty_session.rs | Removed continue statement in error handling loop |
| Cargo.toml | Updated resolvo dependency to development branch with condition support |
|
|
||
| /// Represents a condition in a match spec, which can be a match spec itself or a logical combination | ||
| #[derive(Debug, Clone, PartialEq, Serialize, Eq, Hash)] | ||
| pub enum MatchSpecCondition { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One slight issue I have with this approach is that some information is lost when converting from a string. E.g. converting back to a string loses the order and parenthesis.
Im wondering if we can structure this differently or add a wrapping struct that holds the original statement or something along those lines. Ideally we build a concrete syntax tree.
The reason this is important is that when we report the conditions in an unsat error you want the information to closely resemble the users input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually never parse the condition back to a string so this is not true.
|
@baszalmstra actually we have a few corner cases with our proposed syntax: Or in other words, we cannot use Maybe it would be better to go for a "C-like" syntax, using |
baszalmstra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the liberty to change the code a little bit. I would be happy to merge this now!
|
|
||
| /// Represents a condition in a match spec, which can be a match spec itself or a logical combination | ||
| #[derive(Debug, Clone, PartialEq, Serialize, Eq, Hash)] | ||
| pub enum MatchSpecCondition { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually never parse the condition back to a string so this is not true.
| #[cfg(feature = "conditional_dependencies")] | ||
| pub condition: Option<MatchSpecCondition>, | ||
| /// The condition under which this match spec applies (disabled without conditional_dependencies feature). | ||
| #[cfg(not(feature = "conditional_dependencies"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to keep this field here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I removed it.
|
I updated Cargo.lock and merged main :) |
This PR implements parsing for conditions in MatchSpecs, following the syntax described in conda/ceps#111
Conditions can be used to enable or disable dependencies under certain conditions (such as other packages or virtual packages in the same environment).
They are written in the following way:
numpy; if python >=3.12pywin32; if __winfoobar; if __unix and python >=3.12complex; if (a or b) and (b and c)Conditions are only allowed in run dependencies, and not in constraints. Nesting conditions is also not allowed (e.g.
foobar; if __win; if __python >=3.12is invalid).