-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Conversation
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 |
10188ec
to
0f35b9f
Compare
I splitted the PR in two commits and updated the PR description. |
0f35b9f
to
c490757
Compare
In reviewing this I'm noticing how very long |
r? Eh2406 |
1fb869d
to
9614c87
Compare
86d1760
to
9951a2a
Compare
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'm down to some nits at this point. Thank you so much for all the work.
9951a2a
to
6f1315b
Compare
Thanks for the work! |
☀️ Test successful - checks-actions |
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)
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
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
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:
sat_resolve()
method, we create an additional representing the activation of the root package.List of clauses in the SAT resolver:
List of assumptions in the SAT resolver:
sat_resolve()
are deactivated. This is necessary since the proptest callsat_resolve()
several times with a different root package using the same SAT resolver, and clauses relative to the root package are not removable.