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 empty conflict explanations #4982

Merged
merged 3 commits into from
Jan 25, 2022

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Jan 5, 2022

Fixes #4373

@kit-ty-kate kit-ty-kate added the PR: WIP Not for merge at this stage label Jan 5, 2022
src/solver/opamCudf.ml Outdated Show resolved Hide resolved
@kit-ty-kate kit-ty-kate force-pushed the fix-empty-conflict-explain branch 2 times, most recently from eb5eef5 to 626d140 Compare January 5, 2022 16:19
src/solver/opamCudf.ml Outdated Show resolved Hide resolved
let dsc = match Hashtbl.length missing with
| 0 -> ds
| _ -> ds %% all_conflicting
in
Copy link
Member

Choose a reason for hiding this comment

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

This feels a bit fragile; maybe we could at least check that there is no "missing" below p in the dep tree rather than in general ?

This said, it already avoids the very problematic issue of no messages at all, and thank you a lot for finding a workaround.

@AltGr
Copy link
Member

AltGr commented Jan 19, 2022

This may not be perfect, but already works around most problematic cases in practice, so I think it should be merged (once rebased). Thanks Kate!

@kit-ty-kate kit-ty-kate removed the PR: WIP Not for merge at this stage label Jan 19, 2022
@kit-ty-kate kit-ty-kate marked this pull request as ready for review January 19, 2022 20:43
@kit-ty-kate kit-ty-kate added this to PR in progress in Opam 2.2.0 via automation Jan 19, 2022
@kit-ty-kate kit-ty-kate added this to the 2.2.0~alpha milestone Jan 19, 2022
@kit-ty-kate kit-ty-kate moved this from PR in progress to PR to review in Opam 2.2.0 Jan 19, 2022
@AltGr AltGr merged commit 73d06ce into ocaml:master Jan 25, 2022
Opam 2.2.0 automation moved this from PR to review to Done Jan 25, 2022
rjbou pushed a commit to rjbou/opam that referenced this pull request Apr 22, 2022
@rjbou rjbou mentioned this pull request Apr 22, 2022
4 tasks
rjbou pushed a commit to rjbou/opam that referenced this pull request Apr 22, 2022
rjbou pushed a commit to rjbou/opam that referenced this pull request Apr 27, 2022
rjbou pushed a commit to rjbou/opam that referenced this pull request Jul 12, 2022
@rjbou rjbou added this to PR in Progress in Opam 2.1.x via automation Jul 4, 2023
@rjbou rjbou moved this from PR in Progress to Done in Opam 2.1.x Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Opam 2.1.x
  
Done
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

Unhelpful conflict messages
2 participants