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

Rewrite the conflict explanation extraction mechanism #4349

Merged
merged 15 commits into from
Sep 11, 2020

Conversation

AltGr
Copy link
Member

@AltGr AltGr commented Sep 10, 2020

Finally! This needs more testing, so please everyone gather your
extreme conflict messages and send them to me; but this is very
promising.

@kit-ty-kate
Copy link
Member

Really amazing! I tried it quickly locally and I have a few comments.

Before:

$ opam install extlib-compat
The following dependencies couldn't be met:
  - ocaml-variants → ocaml = 4.11.0
  - extlib-compat → ocaml < 4.07.0 → ocaml-variants < 4.06.2~
      incompatible with the switch invariant ["ocaml-variants" {= "4.11.0+trunk"}] (use `--update-invariant' to force)
  - extlib-compat → ocaml < 4.07.0 → ocaml-base-compiler (>= 3.07+2 & != 3.09.2 & != 3.10.2 & != 4.01.0 & != 4.02.2 & != 4.04.1 & != 4.06.0 & < 4.07.0)
      conflict with the base packages of this switch
Your request can't be satisfied:
  - No available version of ocaml satisfies the constraints
  - No available version of ocaml-variants satisfies the constraints
  - ocaml-variants (!= 4.00.1+BER & != 4.03.0+beta1+flambda & < 4.03.0+statistical-memprof | >= 4.04.0+BER & != 4.05.0+beta3 & < 4.06.0+32bit) is in conflict with ocaml-base-compiler

No solution found, exiting

After:

$ opam install extlib-compat
[ERROR] Package conflict!
  * No agreement on the version of ocaml:
    - (invariant) → ocaml-variants = 4.11.0+trunk → ocaml = 4.11.0
    - extlib-compat → ocaml < 4.07.0
    You can temporarily relax the switch invariant with `--update-invariant'
  * No agreement on the version of ocaml-variants:
    - (invariant) → ocaml-variants = 4.11.0+trunk
    - extlib-compat → ocaml < 4.05.0 → ocaml-variants < 3.09.2~
    You can temporarily relax the switch invariant with `--update-invariant'

No solution found, exiting

I feel like the second bullet point could be removed for an even shorter conflict message. However I suppose this might be needed for much difficult constraints.

I'm also not sure this is a good idea to tell users about --update-invariant. What about:

You might want to use another switch or update your current switch invariant.

or something like that? I'm not sure what would be the best sentence here but I'm sure we can find something.
In particular I don't think users using the "one version of the compiler per switch" scheme need that. I think most users will continue to use this and maybe some will have a default switch, however this one is already vague enough so --update-invariant wouldn't be necessary.

I also think this could be said only once as supposed to twice.

@kit-ty-kate
Copy link
Member

Related issue: #4171

[ERROR] Package conflict!
* No agreement on the version of core:
- core != 112.17.00
- core != 112.17.00
Copy link
Member

Choose a reason for hiding this comment

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

what does it mean? :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

The message is off, but there is actually little we can easily do about it :/ ; my opinion is that in this case the error is clear anyway so it's still OK (but yeah, we should at least filter out duplicate lines).

As for the explanation why, this is a different instance of #2229: we are working on a package graph where we can't really distinguish between conjunction and disjunction between edges. In this case, we have core<112.17.00 & core>112.17.00, which actually resolves to false, but without knowing about the & we wrongly reduce it as an |, which reduces to core != 112.17.00.
I'll check if I can find a way to detect such cases, and skip the "formula reduction" when they happen.

Copy link
Member

Choose a reason for hiding this comment

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

ha yes I was talking about the duplication -- but you are right, it's a bit hard to suggest something more actionnable to the user here.

@AltGr AltGr merged commit 3a2d69e into ocaml:master Sep 11, 2020
@AltGr
Copy link
Member Author

AltGr commented Sep 11, 2020

I feel like the second bullet point could be removed for an even shorter conflict message. However I suppose this might be needed for much difficult constraints.

I totally agree... but I couldn't find a generic way to determine that the first message is strictly better and supersedes the second... I already have quite a few heuristics to remove (lots of) spurious or secondary messages, but here the reason why the first is better is not totally clear, especially without resorting to independent knowledge of the package graph. We have a similar case here, where actually the "missing dependency" is extra information too.

In your case, there is another message issue though: a ocaml-variants < 3.09.2~ is appearing; this one would require some work, basically when no package version matches a constraint after conversion to cudf, we encode it as less than min existing version (this was raised in #4344 as well). I need to check if we can find a way to cure this.

I'm also not sure this is a good idea to tell users about --update-invariant

Indeed, it might help if you're stuck but may not be the best advice in general. I at least removed the duplication, but am open to removing or changing the message as well (with your suggestion, unless something better comes up ?)

Thanks!

@rjbou rjbou added this to the 2.1.0~beta2 milestone Sep 14, 2020
@rjbou rjbou linked an issue Sep 14, 2020 that may be closed by this pull request
@rjbou rjbou added the AREA: UI label Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2.1.0~alpha] Solver failures are too verbose
4 participants