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

Dont run solver if direct dep doesnt build with dune #386

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gridbugs
Copy link
Collaborator

This is a workaround for #385. The error message displayed when a some cases. This adds a check that direct dependencies all have available versions which build with dune before running the solver.

To demonstrate the problem, here's an opam file with one package that doesn't build with dune (ansicolor) and ocaml with a version constraint:

opam-version: "2.0"
depends: [
  "ansicolor"
  "ocaml" {= "4.14.1"}
]

Prior to this change, running opam monorepo lock gives the error:

opam_monorepo.exe: [ERROR] Can't find all required versions.
Selected: base-bigarray.base base-domains.base base-nnp.base
          base-threads.base base-unix.base ocaml-config.3
          ocaml-options-vanilla.1 ocaml-base-compiler&qux ocaml-base-compiler
          ocaml-base-compiler ocaml base-domains ocaml-base-compiler
- ocaml -> ocaml.5.0.0
    ocaml-base-compiler 5.0.0 requires = 5.0.0
- ocaml-base-compiler -> ocaml-base-compiler.5.0.0
    ocaml-base-compiler|ocaml-variants|ocaml-system ocaml-base-compiler requires >= 5.0.0~ & < 5.0.1~
- qux -> (problem)
    Rejected candidates:
      qux.zdev: Requires ocaml = 4.14.1

...which is misleading because it doesn't mention anything about the fact that locking failed because ansicolor doesn't build with dune.

After this change the error is:

opam_monorepo.exe: [ERROR] Some dependencies cannot be built with dune!

opam-monorepo requires that all dependencies use dune as their build system.

These dependencies (possibly transitive) don't use dune as their build system:
- ansicolor

The dune-universe opam repository (git+https://github.com/dune-universe/opam-overlays.git) contains dune ports of some popular packages to help build more packages with dune however it appears to already be set up on this switch. Thus it is possible that no dune port exists for any of these packages.

For information on how to contribute a new dune port, see: https://github.com/dune-universe/opam-overlays

Read more at #385

This is a workaround for
tarides#385. The error message
displayed when a dependency doesn't build with dune is misleading in
some cases. This adds a check that direct dependencies all have
available versions which build with dune before running the solver.
@Leonidas-from-XIV
Copy link
Member

I am not sure this is the right approach to this problem, as if I read #385 correctly the issue is more that the opam-0install-solver doesn't show the actual error that caused the issue. We already inject the a constraint that packages are supposed to build with dune, adding another way to enforce this makes it seem more like patching around weirdness of the solver.

To me it seems more sensible to fix the solver to actually output the expected error message (and from there we can better highlight the reason in a nicer error message, but if the solver doesn't actually display the reason and we're reimplementing parts of the solver that's quite bad). This has the added advantage of making it a better solver for all the current and potential future users of the solver.

@gridbugs
Copy link
Collaborator Author

gridbugs commented May 1, 2023

I totally agree that the right thing to do is to fix this behaviour of the solver. I intended this change to be a quick and dirty workaround to improve opam-monorepo's UX while we spend more time improving the UX of the solver. I'm not sure yet whether the behavior in #385 is a bug in the solver or whether its API doesn't have a way to express that the "packages must build with dune" constraint is more important than other constraints.

The question is whether we expect to fix this behavior of the solver soon and if not what is the cost of adding this workaround in the meantime. This "fix" would have saved me a bunch of time while working on monorepo benchmarking as I would frequently generate a monorepo containing packages that didn't build with dune but the error messages didn't say which packages were causing the problem or even that the problem was related to dune at all so I resorted to binary searching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants