-
Notifications
You must be signed in to change notification settings - Fork 101
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
base: main
Are you sure you want to change the base?
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 |
#[derive(Debug, Clone, PartialEq, Serialize)] | ||
pub struct Statement { | ||
pub prefix: String, | ||
pub condition: Option<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.
I had a hard time understanding what a Statement
is. Especially the prefix
field. Maybe add some comments to clarify.
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.
This seems to only be used in the tests
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.
Yeah this is indeed only used in the tests. I thought I moved it there.
|
||
/// 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.
@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 |
Cargo.toml
Outdated
reqwest-middleware = "0.4.2" | ||
reqwest-retry = "0.7.0" | ||
resolvo = { version = "0.10.0" } | ||
resolvo = "0.10.0" |
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.
0.10.1
also seems to be available. Maybe we can bump to that in the lock-file.
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.12
pywin32; if __win
foobar; if __unix and python >=3.12
complex; 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.12
is invalid).