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

Fix most unhelpful conflict explanation message by merging all formulas together #6106

Merged
merged 3 commits into from
Sep 3, 2024

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Jul 18, 2024

Fixes #4373

This PR fixes it by:

  • using every available hints given by dose3 instead of just taking the first hint for a given set of packages (removal of ChainSet.precludes)
  • merging all the extra information together

In some cases this makes the message as verbose but at least it is correct and helpful.
We can try to make them even more readable in a later work but at least this fixes the biggest issue at hand: conflict messages telling lies and being unreadable.

Built on top of #5210
Also fixes #5422 (alcotest now only takes 45s to print the explanation instead of +infinit)

@kit-ty-kate kit-ty-kate added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Jul 18, 2024
@kit-ty-kate kit-ty-kate added this to the 2.3.0~alpha milestone Jul 18, 2024
src/solver/opamCudf.ml Outdated Show resolved Hide resolved
@rjbou
Copy link
Collaborator

rjbou commented Aug 20, 2024

related to #5130 too

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

I've put some remarks on the code (non linear) but the idea is good, it is working well!

With the third commit, the title of the PR should change to something more general like "Conflict messages: fix most of unhelpful explanations and display full dependency path"

tests/reftests/conflict-4373.test Show resolved Hide resolved
tests/reftests/deps-only.test Show resolved Hide resolved
tests/reftests/deps-only.test Show resolved Hide resolved
src/solver/opamCudf.ml Show resolved Hide resolved
src/solver/opamCudf.ml Outdated Show resolved Hide resolved
src/solver/opamCudf.ml Show resolved Hide resolved
src/solver/opamCudf.ml Outdated Show resolved Hide resolved
src/solver/opamCudf.ml Outdated Show resolved Hide resolved
src/solver/opamCudf.ml Show resolved Hide resolved
src/solver/opamCudf.ml Show resolved Hide resolved
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

All comments resolved in meeting, thanks!

@kit-ty-kate
Copy link
Member Author

squashed

@kit-ty-kate kit-ty-kate merged commit 86f2dbd into ocaml:master Sep 3, 2024
29 checks passed
@kit-ty-kate kit-ty-kate deleted the fix-most-4373 branch September 3, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diverse solver functions take too long to check some requests Unhelpful conflict messages
2 participants