tarides / opam-monorepo

Assemble dune workspaces to build your project and its dependencies as a whole
ISC License
130 stars 27 forks source link

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

Open gridbugs opened 1 year ago

gridbugs commented 1 year ago

This is a workaround for https://github.com/tarides/opam-monorepo/issues/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 https://github.com/tarides/opam-monorepo/issues/385

Leonidas-from-XIV commented 1 year ago

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 commented 1 year ago

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 https://github.com/tarides/opam-monorepo/issues/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.