Remove custom deduplication of packages and use opam solver instead

[samoht]

Convert the constraint "if X and Y are from the same repository" into a conflict relationship in the opam metadata between X and all the version of Y which do not have the same exact version as X.

This allow to remove the adhoc post-processing of opam solution which was going very wrong in lots of case. Hopefully, the results will now be much closer to what you would expect opam to produce.

[Leonidas-from-XIV]

It's a different approach and using hashes is interesting but I don't see how it would solve the issue why the deduplication was implemented. The case that keeps failing is this:

There are OPAM packages foo, and foo-lwt from the same dev-repo which are included in the same tarball (as dune-release generates them). We assume they can be always built together.

The project to be locked depends on foo {>= 1.1} and foo-lwt {< 1.1}. Tarball foo.1.0.tbz2 contains libraries foo and foo-lwt and has the hash f00df00d. Tarball foo.1.1.tbz2 contains libraries foo and foo-lwt and has hash cafef00d. The previous solution would ignore the constraint of foo-lwt and use foo-lwt.1.1 anyway and unpack and use only the foo.1.1.tbz2 tarball.

This solution would include both tarballs in the lock file (since their dev-repos match but their hashes don't) and if fetching those worked then dune wouldn't be able to build the solution since the duniverse then contains public_library foo (1.0), public_library foo (1.1), public_library foo-lwt (1.0) and public_library foo-lwt (1.1) and it has no way to pick which to build.

[samoht]

No this PR will say there is no solution (which is the fine answer to give here) as foo.1.1 and foo-lwt.1.0 will be in conflict

[samoht]

Could you point out to a place where such constraints happen in practice? There is maybe a better way to fix those than to add (more) complexity in opam-monorepo.

[samoht]

(Also I'm not sure I understand your example: what bar has anything to do with foo?)

[Leonidas-from-XIV]

(Also I'm not sure I understand your example: what bar has anything to do with foo?)

Sorry, the example I wrote said foo and bar but then I realized that foo and foo-lwt are more real-life examples and forgot to replace bar everywhere. I've updated it, could you re-read it and let me know if it is clearer now?

But if now it will fail to solve, this is:

1. Not what OPAM would do, where foo.1.1 and foo-lwt.1.0 would be a valid and compiling solution
2. Can be achieved easier by just detecting the case of duplicates and instead of deduplicating (and printing a warning about the deduplication) just fail.

The deduplication was added to make solutions that work in OPAM work in opam-monorepo while sidestepping the issues of having the same library multiple times in the duniverse.

331 is the case you've seen that would be affected. Where h2 and hpack are from the same package (and opam-monorepo requires them to be the same version) whereas a different dependency, paf requires a a lower version of h2 whereas the user code requires a higher version of h2. So this is a more complicated example (since the lower-bound is introduced through an external package and as a software author controlling the bounds of packages you depend on is hard).

Thus introducing a conflict artificially would make it fail in the solver already, thus potentially making opam-monorepo fairly useless since a lot of compiling solutions (that work with OPAM) would be rejected.

[samoht]

I'm still not sure to understand - this patch does exactly what @reynir is suggesting in https://github.com/tarides/opam-monorepo/issues/331#issuecomment-1477772291: it makes the solver do the deduplication by always generate a consistent solution (for opam-monorepo). I'm ok to diverge from opam here slightly. We have extra constraints: all archives we download have to be co-installable, so it's better to fail cleanly with a clear error message instead of trying to patch up things randomly. Also, post-processing patching of solutions is generally a bad idea as it might ignore some real constraints.

[Leonidas-from-XIV]

But the problem is not a solver problem. If you remove the deduplication the solver will leave you with a perfectly fine solution, the issue will be that this is not a solution that dune can build. The problem has actually very little to do with opam-monorepo, on the other hand, opam-monorepo tries to sidestep it as much as possible by hopefully only fetching tarballs where the public_name fields are all unique so dune knows how to build them.

To not split the discussion too much between here and there, I've replied to @reynir's comment on the original issue: https://github.com/tarides/opam-monorepo/issues/331#issuecomment-1697491454 to explain the exact issue with h2 and hpack as specific examples.

[Leonidas-from-XIV]

I'm ok to diverge from opam here slightly. We have extra constraints: all archives we download have to be co-installable, so it's better to fail cleanly with a clear error message instead of trying to patch up things randomly.

Well that's fair, but then we trade the bug that constraints in some cases are ignored (and we try to optimistically use the newer version) with a strict failure in that case (so instead of maybe failing to build it will definitely fail to lock, thus decreasing the amount of packages that can be installed this way).

We can of course do that and I am happy to have a more reliable solution but I also think that users will not be happy if a new version of opam-monorepo can't build a duniverse anymore after an older version was able to make it work (albeit in a rather wonky way).

[samoht]

We can of course do that and I am happy to have a more reliable solution but I also think that users will not be happy if a new version of opam-monorepo can't build a duniverse anymore after an older version was able to make it work (albeit in a rather wonky way).

I am really curious if this is a real issue in practice. I'm really much more inclined to make the tool fail in an understandable way and let packages "fix" potential issues (like the temporary incompatibility of h2 and hpack which was fixed as soon as it was identified properly).

I also think we could make the conflict message more explicit in case the conflict was added because of using the same dev-repo but that could be done later on.

I think we should get that one merged and released and see how that works in practice - happy to revert if something terrible happens ;-)

[samoht]

For some reasons the CI is broken and doesn't test anything. I think it comes from #395 -- fixing it now.

[samoht]

Rebased

[Leonidas-from-XIV]

Thanks. I think we should cut a release sometime soon so users can give this a spin and provide feedback.

[samoht]

Sounds good - let me know when it's released and I'll try it on mirage stuff :p