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

test: add support for features in the sat resolver #14583

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

x-hgg-x
Copy link
Contributor

@x-hgg-x x-hgg-x commented Sep 23, 2024

What does this PR try to resolve?

This PR implements the first step of #11938 (comment).

How should we test and review this PR?

The first commit does some refactorings, and the second commit updates the SAT resolver.

List of boolean variables in the SAT resolver:

  • One variable representing the activation of each registry package.
  • One variable representing the activation of each feature of a given registry package.
  • In the sat_resolve() method, we create an additional representing the activation of the root package.

List of clauses in the SAT resolver:

  • If a package feature is activated, then the package should be activated.
  • No two packages with the same links set.
  • No two semver compatible versions of the same package.
  • For each package:
    • For each feature:
      • If the package feature is activated and it depends of another feature of the same package, then it is also activated.
      • If the package feature is activated and it depends of a dependency, then at least one of the compatible dependency package should be activated.
      • If the package feature is activated and it depends of a feature of a dependency, then the feature of a compatible dependency package should be activated only if the compatible dependency package is activated. If this is not a weak dependency feature, then at least one of the compatible dependency package should be activated.
    • For each dependency, if the package is activated and if the dependency is non-optional or has been activated, then at least one of the compatible dependency package and its required features should be activated.
  • The root package has the same dependency clauses but has no features.

List of assumptions in the SAT resolver:

  • The root package is activated.
  • Old root packages from previous calls to sat_resolve() are deactivated. This is necessary since the proptest call sat_resolve() several times with a different root package using the same SAT resolver, and clauses relative to the root package are not removable.

@rustbot
Copy link
Collaborator

rustbot commented Sep 23, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2024
@Eh2406 Eh2406 self-requested a review September 24, 2024 18:24
@Eh2406
Copy link
Contributor

Eh2406 commented Sep 24, 2024

Thank you so much for working on this! I look forward to digging into what you've done.

That being said, there's a lot here. Any advice for where to start? Is it possible to split up the change into smaller logically separable commits? For example changing resolve to take a Summary instead of a list of deps, could probably be its own commit that happens before the code that actually looks for the feature table. That might make it easier to review what is "just" mechanically moving the arguments around, and what is fundamental changes. And would be a good place for me to ask "would it be clearer if we leave resolve with the same simple API for the tests that exist and add a resolve_with_features for things that use the new functionality?".

@x-hgg-x x-hgg-x force-pushed the features-sat-resolver branch 3 times, most recently from 10188ec to 0f35b9f Compare September 24, 2024 21:19
@x-hgg-x
Copy link
Contributor Author

x-hgg-x commented Sep 24, 2024

I splitted the PR in two commits and updated the PR description.

@x-hgg-x x-hgg-x force-pushed the features-sat-resolver branch from 0f35b9f to c490757 Compare September 25, 2024 10:47
@Eh2406
Copy link
Contributor

Eh2406 commented Sep 25, 2024

In reviewing this I'm noticing how very long lib.rs and resolve.rs are getting. Let's not do it in this PR, but as a follow-up it would be really nice to split these monoliths into separate files.

@ehuss
Copy link
Contributor

ehuss commented Sep 25, 2024

r? Eh2406

@rustbot rustbot assigned Eh2406 and unassigned ehuss Sep 25, 2024
@x-hgg-x x-hgg-x force-pushed the features-sat-resolver branch 4 times, most recently from 1fb869d to 9614c87 Compare September 26, 2024 16:40
@x-hgg-x x-hgg-x force-pushed the features-sat-resolver branch 2 times, most recently from 86d1760 to 9951a2a Compare September 26, 2024 19:55
Copy link
Contributor

@Eh2406 Eh2406 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm down to some nits at this point. Thank you so much for all the work.

crates/resolver-tests/src/lib.rs Outdated Show resolved Hide resolved
crates/resolver-tests/src/lib.rs Show resolved Hide resolved
crates/resolver-tests/src/lib.rs Outdated Show resolved Hide resolved
@x-hgg-x x-hgg-x force-pushed the features-sat-resolver branch from 9951a2a to 6f1315b Compare September 27, 2024 20:09
@Eh2406
Copy link
Contributor

Eh2406 commented Sep 27, 2024

Thanks for the work!
@bors r+

@bors
Copy link
Contributor

bors commented Sep 27, 2024

📌 Commit 6f1315b has been approved by Eh2406

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2024
@bors
Copy link
Contributor

bors commented Sep 27, 2024

⌛ Testing commit 6f1315b with merge 2d368ed...

@bors
Copy link
Contributor

bors commented Sep 27, 2024

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 2d368ed to master...

@bors bors merged commit 2d368ed into rust-lang:master Sep 27, 2024
22 checks passed
@bors bors mentioned this pull request Sep 27, 2024
@x-hgg-x x-hgg-x deleted the features-sat-resolver branch September 27, 2024 22:12
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 5, 2024
Update cargo

17 commits in 80d82ca22abbee5fb7b51fa1abeb1ae34e99e88a..ad074abe3a18ce8444c06f962ceecfd056acfc73
2024-09-27 17:56:01 +0000 to 2024-10-04 18:18:15 +0000
- test: Remove the last of our custom json assertions (rust-lang/cargo#14576)
- docs(ref): Expand on MSRV (rust-lang/cargo#14636)
- docs: Minor re-grouping of pages (rust-lang/cargo#14620)
- docs(ref): Highleft whats left for msrv-policy (rust-lang/cargo#14638)
- Fix `cargo:version_number` - has only one `:` (rust-lang/cargo#14637)
- docs: Declare support level for each crate in our Charter / docs (rust-lang/cargo#14600)
- chore(deps): update tar to 0.4.42 (rust-lang/cargo#14632)
- docs(charter): Declare new Intentional Artifacts as 'small' changes (rust-lang/cargo#14599)
- fix: Remove implicit feature removal (rust-lang/cargo#14630)
- docs(config): make `--config <PATH>` more prominent (rust-lang/cargo#14631)
- chore(deps): update rust crate unicode-width to 0.2.0 (rust-lang/cargo#14624)
- chore(deps): update embarkstudios/cargo-deny-action action to v2 (rust-lang/cargo#14628)
- docs(ref): Clean up language for `package.rust-version` (rust-lang/cargo#14619)
- docs: clarify `target.'cfg(...)'`  doesnt respect cfg from build script (rust-lang/cargo#14312)
- test: relax compiler panic assertions (rust-lang/cargo#14618)
- refactor(compiler): zero-copy deserialization when possible (rust-lang/cargo#14608)
- test: add support for features in the sat resolver (rust-lang/cargo#14583)
@rustbot rustbot added this to the 1.83.0 milestone Oct 5, 2024
bors added a commit that referenced this pull request Oct 8, 2024
Add more SAT resolver tests

### What does this PR try to resolve?

This is a follow-up of #14583.

### How should we test and review this PR?

* Commit 1 splits tests into smaller modules.
* Commit 2 adds more helper trait methods.
* Commit 3 refactors computation of the SAT clauses, by introducing intermediate boolean variables for each dependency and each dependency feature declared in a package.
The old behavior was incorrect: package features specified in an optional dependency were activated if the package was activated, even if the dependency wasn't activated.
* Commit 4 add tests from https://github.com/Eh2406/pubgrub-crates-benchmark/tree/main/out/index_ron.

r? Eh2406
bors added a commit that referenced this pull request Oct 8, 2024
Add more SAT resolver tests

### What does this PR try to resolve?

This is a follow-up of #14583.

### How should we test and review this PR?

* Commit 1 splits tests into smaller modules.
* Commit 2 adds more helper trait methods.
* Commit 3 refactors computation of the SAT clauses, by introducing intermediate boolean variables for each dependency and each dependency feature declared in a package.
The old behavior was incorrect: package features specified in an optional dependency were activated if the package was activated, even if the dependency wasn't activated.
* Commit 4 add tests from https://github.com/Eh2406/pubgrub-crates-benchmark/tree/main/out/index_ron.

r? Eh2406
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants