opamCudf: provide machine-readable information on conflicts caused by cycles

[gasche]

For conflicts caused by cyclic actions, the opamCudf API would
previously only allow to observe the cycle with actions described as
strings for human pretty-printing.

We would like to write a script, using the opam API, that can react to
solution errors caused by cycles by patching the query and calling the
solver again. For this we need a machine-readable representation of
conflict cycles. ("We" here is myself and @Armael.)

The present commit makes the minimal change to the opamCudf API to
make this possible:

• The internal representation of cycles is changed to take CUDF
  packages (the most general information) instead of strings.
• The OpamCudf.cycle_conflicts function has its type changed accordingly
  (this changes the module interface and may break library consumers).
• A new function OpamCudf.conflict_cycles is exposed to recover
  a cycle representation carrying "opam package" actions.

Remark: conflict_cycles is in the style of the current API on the
abstract conflict type, which is not very good: a conflict is
a variant/sum type in hiding (either an incompatibility in the query
or a cycle in the solution), and functions do something in one case
and silently return nothing in the other.

It would be nicer to expose a variant-sum type view of the conflict
type to users, but this should be discussed with OPAM maintainers, and
is not a requirement for our work.

[rjbou]

cc @AltGr

[AltGr]

Ok for me !
Now I am curious what exactly your are doing here... how about a chat some day?
Loosely related, and you probably know about it, but from the command-line there is a generic command (as in, not related to a specific query) to check for all possible dependency cycles:

% opam admin check --ignore-test-doc  --cycles  
[ERROR] Dependency cycles detected:
  * fileutils = 0.6.0 → oasis = 0.2.0
  * oasis (>= 0.3.0 & < 0.4.2) → ocamlmod < 0.0.7 → fileutils = 0.6.0
  * fileutils = 0.6.0 → oasis (>= 0.4.5 & < 0.4.7) → gettext
  * ocamlmod < 0.0.7 → fileutils = 0.6.0 → oasis (>= 0.4.5 & < 0.4.7)
  * fileutils = 0.6.0 → oasis = 0.4.7
  * ocamlmod < 0.0.7 → fileutils = 0.6.0 → oasis = 0.4.7
  * ocamlmod < 0.0.7 → fileutils = 0.6.0 → oasis (>= 0.4.2 & < 0.4.5 | >= 0.4.8)
  * ocamlfind-secondary = 1.8.1 → ocamlfind = 1.8.1 → graphics = 5.0.0 → dune = 2.0.0
  * ocamlfind-secondary = 1.8.1 → ocamlfind = 1.8.1 → graphics = 5.1.0 → dune = 2.0.0
  * ocamlfind-secondary = 1.8.1 → ocamlfind = 1.8.1 → graphics = 5.1.0 → dune-configurator = 1.11.4 → dune = 2.0.0
  * ocamlfind-secondary = 1.8.1 → ocamlfind = 1.8.1 → graphics = 5.1.0 → dune-configurator = 2.0.0 → dune = 2.0.0
  * ocamlfind-secondary = 1.8.1 → ocamlfind = 1.8.1 → graphics = 5.1.0 → dune-configurator = 2.0.0 → dune-private-libs = 2.0.0 → dune = 2.0.0
Summary: out of 12977 packages (2542 distinct names)
- 29 packages part of dependency cycles

[gasche]

Now I am curious what exactly your are doing here... how about a chat some day?

(I've been trying to have a physical meeting with you for unrelated matter, but scheduling appears to be tricky due to everyone's constraints.)

This is part of the Marracheck project that we already discussed. We are calling a solver to create "large sets of co-installable packages", but the solver sometimes returns solutions that contain a cycle (the example found by Armaël is that OASIS has a dependency for which some earlier versions depended on OASIS themselves). When OPAM (invoked programmatically through its API) complains about the cycle, we feed a fixed query to the solver that forbid the particular choice of packages involved in the cycle, to get a new solution that avoids it.

[Armael]

@AltGr I was not aware that there was a way to precompute all possible cycles, thank you for that.
Given this, we could simplify our strategy a bit.

As @gasche said, currently we detect cycles "dynamically": whenever the solver returns a solution that contains a cycle, we add this cycle as a conflict and re-run the query. Instead, we could pre-compute all cycles in the universe of packages and add them as conflicts beforehand.

If we do that (which seems a bit cleaner and possibly more efficient than patching and re-running queries), then we don't strictly need the present PR. Indeed, it seems we can then use OpamAdminCheck.cycle_check : universe -> package_set * formula list list to get the cycles.

I still think this PR is a nice cleanup, though :-).

[Armael]

In fact, I am now wondering: why isn't opam already doing this before calling to a solver (that is, pre-computing the cycles and adding them as conflicts)?

As far as I understand, the fact that the solver can return a solution that can contain cycles is because the encoding of the installability problem typically does not require a solution to be acyclic. And if I'm reading the code correctly, when a cyclic solution is returned by the solver, opam simply fails and prints the cycle back to the user. However, it's not really the user's fault, is it?

So it seems to me that the correct thing to do would be to pre-compute the cycles and exclude them from the universe before calling to the solver. Is the reason why that's not done currently simply that solution containing cycles happen rarely enough in practice? And maybe that pre-computing cycles would reliably slow down "opam install" queries?

Or am I missing something?

[AltGr]

So it seems to me that the correct thing to do would be to pre-compute the cycles and exclude them from the universe before calling to the solver.

This is correct. There are two reasons why it is was not done:

• it would be costly
• we have been considering this an error in the repository, that we can actually fix upstream. There is a corresponding verification in opam admin check, but it's not sufficiently enforced at the moment.

Granted, you could technically have two independent cycle-free repositories and end up with a cycle when you use both together, but I guess we can keep looking over this for the moment.

[dra27]

This is a nice change, and apologies for it's having been dropped for so long. Would you be to happy to rebase it so that it can reviewed with rather less lag for opam 2.2? If you don't have time at the moment, we'd be happy to take this on.

[kit-ty-kate]

LGTM. Good to merge once CI is green

[Armael]

Thanks for the rebase. I think this is still relevant and a useful improvement of the API, so I'm happy to see it merged.
(Looking at the current code of marracheck we've been using the "other" approach of precomputing the cycles and excluding them from the solution, but it seems that there might be some remaining issues related to cycles so this PR should be useful to have in any case.)

[kit-ty-kate]

Thanks!