Skip to content
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

Feat: optional feature support in resolvo #105

Closed
wants to merge 41 commits into from

Conversation

prsabahrami
Copy link
Contributor

@prsabahrami prsabahrami commented Jan 29, 2025

This adds support for optional features in resolvo on top of conditionals. Hence, I think it's better to merge the conditionals first and then move on to this one.
I added 2 new clauses:

    /// A conditional clause that requires a feature to be enabled for the requirement to be active.
    ///
    /// In SAT terms: (¬A ∨ ¬C ∨ ¬F ∨ B1 ∨ B2 ∨ ... ∨ B99), where A is the solvable,
    /// C is the condition, F is the feature, and B1 to B99 represent the possible candidates for
    /// the provided [`Requirement`].
    ConditionalWithExtra(
        VariableId,   // solvable
        VariableId,   // condition
        VersionSetId, // condition version set
        VariableId,   // extra
        StringId,     // extra name
        Requirement,  // requirement
    ),
    /// A requirement that requires a feature to be enabled for the requirement to be active.
    ///
    /// In SAT terms: (¬A ∨ ¬F ∨ B1 ∨ B2 ∨ ... ∨ B99), where A is the solvable,
    /// F is the feature, and B1 to B99 represent the possible candidates for
    /// the provided [`Requirement`].
    RequiresWithExtra(VariableId, VariableId, StringId, Requirement),

which are then mapped properly to the correct set of conditionals or extras enabled.
Also, this is how requirements are shown now:

pub struct ConditionalRequirement {
    /// The condition that must be met for the requirement to be active.
    pub condition: Option<VersionSetId>,
    /// The requirement that is only active when the condition is met.
    pub requirement: Requirement,
    /// The extra that must be enabled for the requirement to be active.
    pub extra: Option<StringId>,
}

Adding support for record datas with below spec

"optional_depends": [
        {
            "if": ["__win"],
            "requires": "winapi"
        },
        {
            "feature": "feature_a",
            "requires": "qt-main"
        },
        {
            "feature": "feature_b",
            "requires": "sphinx >=7",
            "if": ["python 3.11.*"]
        }
        {
            "feature": "foobar",
            "requires": "python >=3.8"
        }
        {
            "feature": "foobar",
            "requires": "sphinx >=7",
            "if": ["python 3.11.*"]
        }
    ]
}

Becomes much easier with this format.

@baszalmstra
Copy link
Contributor

Im stuggling a bit to review this PR while the changes from conditional dependencies are also here. So I'll do a proper review later.

I quickly looked through the tests to understand the behavior but could not find any. Is it correct that you have no tests for extras yet? I highly recommend that you start doing more test-driven development and coming up with edge cases. I also would like to see the conflict reports that some tests result in.

How are extras currently assigned a value? In the API I can see that a condition may also include an extra which as far as I understand it means that "if the extra is true" the requirement must be met. E.g. ~Foo@1 v ~Extra v Req1 v ... Reqn. But how is Extra assigned a value? How do you express that a package A depends on package B with the extra C?

It feels to me like we are adding a lot of complexity with this feature but I feel like we could capture this behavior and conditional dependencies in a similar way. E.g. both semantically and in SAT terms, conditional dependencies and extras are very similar.

We have the conditions on other dependencies (or versionsets) and we have conditions on whether a specific extra is selected for a package. Both of these can be expressed as another boolean expression. Although I think its nice if the API is explicit about the different features, I dont think the clauses need to be like that.

If we would write any combination of boolean conditions (package or extra) as a simplified DNF boolean expression we could derive any conditional requirement clause from that. E.g. the condition foo 1..2 AND bar 1..2 e.g.

> (foo@1∨foo@2)∧(bar@1∨bar@2)
# in DNF
> (foo@1∧bar@1)∨(foo@1∧bar@2)∨(foo@2∧bar@1)∨(foo@2∧bar@2)

You would write the requirement that says lorum _requires_ ipsum;_ if foo 1..2 AND bar 1..2 as 4 separate clauses:

1. ~lorum@1 v ~(foo@1∧bar@1) v ipsum
2. ~lorum@1 v ~(foo@1∧bar@2) v ipsum
3. ~lorum@1 v ~(foo@2∧bar@1) v ipsum
4. ~lorum@1 v ~(foo@2∧bar@2) v ipsum

Note the 4 clauses from the DNF form.

Then also a clause with a condition or without one could look very similar.

Ill just leave this braindump here. Food for thought.

@prsabahrami
Copy link
Contributor Author

prsabahrami commented Jan 30, 2025

But how is Extra assigned a value?

What I had in mind was that this is really done from the rattler side when we are parsing the matchspecs, all of the extra are added as requirements and also the ones in the initial spec are set to true. But also these can be really seen as conditionals as well if while the solving is happening a specific is extra is set to true. So I guess what you are saying does make sense.

So what we are looking at is essentially a way of having a clause that looks like
VariableId (~lorum@1) then an optional set of variables which all need to be true for the clause to be true and then the requirement. This does make everything much more beautiful and we'll have the option to sort of support AND or even OR clauses as well. Hence the Requirements would have like a Vec<Conditions> and then Condition can be a string or a VersionSetId, right?

@prsabahrami
Copy link
Contributor Author

Closing this as the support for extras is also being added in #101

@prsabahrami prsabahrami closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants