Add more SAT resolver tests #14614
Add more SAT resolver tests #14614
Conversation
rustbot
added
A-dependency-resolution
|
First off, thank you so much for splitting the PR into independently understandable commits. It makes reviewing this PR much more feasible. For example the first 2 commits look 100% and ready for +1. If the other commits require extensive discussion, maybe we should get those first two merged asap. Secondly, thank you for giving descriptive names to the pubgrub tests! Would you mind if I took your good names back to that project? (Or even better do you want to do a PR?) |
x-hgg-x
force-pushed
the
more-sat-resolver-tests
branch
6 times, most recently
from
e92e958 to
fddd9cb
Compare
I posted a PR: Eh2406/pubgrub-crates-benchmark#10. |
|
I meant to do a new round of review today, looking at your new changes and being sure your old changes still made sense with fresh eyes and making sure the tests had translated correctly. (If I had to do the translations I would've made more than a few copy/paste errors.) Unfortunately, had a family emergency come up overnight. Luckily, everyone's okay. The only casualty was my sleep/concentration schedule. With the holy days the end of this week, I cannot guarantee availability of review time. I will make time for this Monday, if I have not gotten to it sooner. Given how enthusiastic I am to see your progress on this work, I am very sorry at how I'm slowing you down. |
x-hgg-x
force-pushed
the
more-sat-resolver-tests
branch
6 times, most recently
from
27083f1 to
480e8fc
Compare
|
SAT is slippery, I wouldn't be surprised if we find more weird cases as we add more test cases/fuzzing. But I don't know what they are. So for now this looks good. Thank you! @bors r+ |
bors
added
S-waiting-on-bors
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
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
x-hgg-x
force-pushed
the
more-sat-resolver-tests
branch
from
480e8fc to
d1ef31c
Compare
|
The |
Add new intermediate boolean variables for each dependency and each dependency feature declared in a package. This is used to simplify computation of the sat clauses. Add support for multiple dependencies with the same name, if their kind or target is different. A non-weak dependency feature for an optional dependency now activates a feature of the same name in the sat resolver.
x-hgg-x
force-pushed
the
more-sat-resolver-tests
branch
from
d1ef31c to
dedf251
Compare
|
It should be good now after a rebase on |
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 8 commits in ad074abe3a18ce8444c06f962ceecfd056acfc73..15fbd2f607d4defc87053b8b76bf5038f2483cf4 2024-10-04 18:18:15 +0000 to 2024-10-08 21:08:11 +0000 - initial version of checksum based freshness (rust-lang/cargo#14137) - feat: Add custom completer for completing registry name (rust-lang/cargo#14656) - Document build-plan as being deprecated (rust-lang/cargo#14657) - fix(complete): Don't complete files for any value (rust-lang/cargo#14653) - Add more SAT resolver tests (rust-lang/cargo#14614) - fix: avoid inserting duplicate `dylib_path_envvar` when calling `cargo run` recursively (rust-lang/cargo#14464) - chore(deps): bump gix-path from 0.10.9 to 0.10.11 (rust-lang/cargo#14489) - improve error reporting when feature not found in `activated_features` (rust-lang/cargo#14647) --- This also adds three license exceptions to Cargo. * arrayref — BSD-2-Clause * blake3 — CC0-1.0 OR Apache-2.0 OR Apache-2.0 WITH LLVM-exception * constant_time_eq — CC0-1.0 OR MIT-0 OR Apache-2.0 These exceptions were added to rustc in rust-lang#126930, so should be fine for Cargo as well.
test: add fixes in the sat resolver ### What does this PR try to resolve? This is a follow-up of #14614. ### How should we test and review this PR? Commit 1 removes duplicate variables in the sat resolver. Commit 2 removes useless clones in the sat resolver. r? Eh2406
What does this PR try to resolve?
This is a follow-up of #14583.
How should we test and review this PR?
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.
r? Eh2406