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

Allow DependencyOnTheEmptySet #133

Merged
merged 1 commit into from
Oct 22, 2023
Merged

Allow DependencyOnTheEmptySet #133

merged 1 commit into from
Oct 22, 2023

Conversation

Eh2406
Copy link
Member

@Eh2406 Eh2406 commented Oct 4, 2023

This closes #125

I started by adding a panic to the sat solver when it is had a DependencyOnTheEmptySet, all tests passed because proptest was not configured to generate them. I modified proptest and confirmed that this test now failed. meta_test_deep_trees_from_strategy then informed me that I had too many ways for package to be invalid. Removing allow_deps and reducing the probability of generating DependencyOnTheEmptySet made it happy. Then I could get to the meat of the experiment. I removed the code that generates DependencyOnTheEmptySet and all test pass. Well prop test passing once don't mean much, so I increased the cases in the ProptestConfig. It has been running for a long time without finding a problem.

Excitement over this progress is a little premature. This library is surprisingly robust against violations of its internal assumptions. In a number of places we filter out and assume that no term of an incompatibility is Term::any(), which turns out to be exactly what's generated when DependencyOnTheEmptySet. Adding a check for that property in merge_incompatibility makes everything unhappy. This is easy enough, when we generate the incompatibility for the dependency just leave off that term. Unfortunately, this causes another problem if the root package has one of these dependencies then it is automatically terminal, leading to the unhelpful error message "Root package depends on itself at a different version?". That check is always been suspect, it even has a comment about removing it. If we do remove it, everything passes. The next call is to add_version which does a more complicated check and decides not to add the package. So the loop continues to unit_propagation which correctly generates a proof of unSAT.

@Eh2406
Copy link
Member Author

Eh2406 commented Oct 19, 2023

I think this is correct and a way to add an important use case. But there are lots of subtle interactions. @mpizenberg, do you see something I missed?

Copy link
Member

@mpizenberg mpizenberg left a comment

Choose a reason for hiding this comment

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

Thanks for the call and explanations

@Eh2406 Eh2406 merged commit 75618a0 into dev Oct 22, 2023
5 checks passed
@Eh2406 Eh2406 deleted the dep-on-none branch October 22, 2023 22:16
@Eh2406
Copy link
Member Author

Eh2406 commented Oct 23, 2023

In retrospect this should probably have non-prop tests as well. I can make a PR soonish. There where existing tests.

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