Allow marking certain packages as "installed via OPAM"

[Leonidas-from-XIV]

This is a draft PR on the hybrid mode feature, which makes it possible to actually test things and go somewhere from there.

1. Packages to be installed via opam can't be marked as not-installable in the solver because in such case there is no solution. I explored this avenue and it doesn't lead anywhere. At least one part of the solution space eliminated ✔️
2. There's currently warnings about opam-provided, our custom variable. Not great.
3. Variables are false by default so packages marked this way will not be picked up by opam.
4. The detection of the variable is extremely inflexible now. This is just (very necessary) busywork to walk that formula and determine whether the variable is set but needs to be done before release. For testing the concept on examples this works ok.
5. It needs a command to install the opam-provided packages. That's the easiest part so I'm leaving it for last, once we have figured out the other problems just collecting packages to install should be simple.

(No changelog entry because it shouldn't be merged and also for a decent changelog entry I'd want to have the functionality in place so I know what to describe)

But what works is:

1. Mark a package as opam-provided in the opam/dune-project file, e.g. bos like in this PR
2. Run opam-monorepo lock
3. See that bos and its dependencies like fpath, rresult and friends aren't part of the lockfile anymore, so they wouldn't be pulled anymore.

[NathanReb]

So, I haven't looked at the code yet but there are a few things to clarify here.

The first is that to work well with opam, the variable should be used to mark the packages to pull, not the ones to opam install.
It should also work as it does today for opam files that don't have the variable at all, e.g. assume everything must be vendored in those cases.

The second is that we need to think carefuly about the solving step. Including the opam deps there will give a consistent result as you'll know everything can be co-installed but it then requires us to properly filter out any transitive deps that have leaked in the solution.
Having everything co-installable might also not be a requirement, for instance if you need to install a binary tool, the fact that its dependencies conflict wit the ones of your project is somewhat irrelevant.

I think a good goal here is to go for the minimum viable. The minimum is that the opam deps and duniverse can be built in the same switch, i.e. they are compatible ocaml and dune-wise. I think we should proceed as follow:

• Filter out the opam install packages from the deps formula (some packages such as ocaml and dune should be kept) of the targets packages before sending them to the solver
• Extract the dune and ocaml constraints from the deps formula of the package that have been filtered out above
• Run the solver with the filtered deps formula and the extra constraints extracted above. This gives us a solution for the duniverse, ocaml and dune.

At this point we can run the solver again with the fixed ocaml and dune from the above solution to get a solution for the opam install packages and merge it with the other solution so that everything that matters is locked there and one can just use the lockfile for all their dependency needs. The result of this merge might not always work for several reasons. We should probably stick to adding the opam install packages (not their deps) with the = constraint form the second solver run and let opam decide which version it will install for their deps. This is not a total lock but I think it's good enough for a first iteration.

I don't think there's an easy answer here so let's think in terms of workflow. What I'd like to advocate is the following:

opam switch create ./ --empty
opam monorepo lock
opam install --deps-only -t --locked ./
opam monorepo pull

Another option is to completely skip the second solver run and just re-inject the deps from the source opam files. It might make for a better first iteration by the way.

One last thing to note is that the lockfile should always contain the variables to mark the ones that opam should install or not. It's probably worth doing this fist in a separate PR by the way as this will enable the above workflow!

[Leonidas-from-XIV]

@NathanReb I'm currently resolving everything marked as opam-provided with the solver and removing it from the set of dependencies. There might be cases where it would resolve differently than with a larger set but this seems to me to be also true if I were to manually go down the formulas as well, creating another kind of solver. This seemed to be a convenient setup but if there's any concerns I am happy to change the code.

[NathanReb]

This is starting to look great!

I've done a bit of testing on the latest version and I identified a couple things:

1. It seems that the opam-provided packages and their dependencies are properly filtered out atm. I tested this on opam-monorepo itself, marking uri as opam-provided. uri has a dependency on angstrom and we don't have transitive dependencies on either of them. They were both filtered out correctly, this shows because they are not marked with the vendored variable in the lockfile. They do appear in the duniverse-dirs though which suggest they were wrongly included in the Duniverse.t. We need to make sure that's not the case. They also appear in the pin-depends field but I guess fixing this by filtering them out before building the Duniverse.t should fix both problems.
2. I then tried to add angstrom as a direct dependency, without marking it as opam-provided and the result was that it was still filtered out when I expected it would not. What should be done here is a bit less clear as we're explicitly going to state that dependency cones intersection between duniverse and opam-provided are not supported. We need to at least look into this to decide what should be done and document the filtering accordingly.

[NathanReb]

One thing I just thought about: the opam-provided packages shouldn't be required to build with dune, we need to patch the solver so it knows to do that!

[Leonidas-from-XIV]

1. I was also testing with uri. It's a good catch, fortunately it is quite easy to fix because there is code that filters out base packages and virtual packages, so adding a check there solved that issue.
2. This is indeed an issue, that we need to figure out what the right thing is in this case. At the moment, everything in the dependency tree of opam-provided packages is eliminated. This has the advantage that the transitive dependencies of opam-provided packages don't need to use dune. For them to be opam-provided they must be installed by opam anyways. Therefore pulling them into the duniverse would mean we need to duplicate them and the duplicate copy needs to be dune-buildable. Let's discuss the semantics that we wish for it.
3. That's an excellent observation that I completely missed out! I changed it so it runs the solver first for the opam-provided dependencies allowing non-dune builds and put the set of packages that are thus known to be installed by opam (by whichever means) in an exception list. This has some interaction with 2. because if we install angstrom again, the second copy has to build with dune.

[Leonidas-from-XIV]

I've added a number of tests:

• A very simple one to show vendoring in action
• A more complicated one with transitive dependencies
• A variation of the previous one, but with the package to be opam-provided flipped, so demonstrate the implication

[NathanReb]

It's good to finally have tests for this!

I think another good thing to test is the dependency collision thing, e.g. both the target package and an opam provided package depend on a same third one, whatever behaviour we choose there.

I have a few comments on the code but it's looking good!

[Leonidas-from-XIV]

@TheLortex Can you take another look? There was indeed an error and I ended up just adding debug/info prints in relevant places to be able to see why a package is classified as either (so --verbosity=debug might show some interesting insights why things are wrong).

[NathanReb]

Looks good! This is long overdue!

[TheLortex]

I'm still having issues using the feature. See this opam file:

opam-version: "2.0"
name: "noop"
maintainer: "dummy"
authors: "dummy"
homepage: "dummy"
bug-reports: "dummy"
dev-repo: "git://dummy"
synopsis: "Unikernel noop - monorepo dependencies"

depends: [
  "ocaml-freestanding" { build & >= "0.7.0" }
]
x-opam-monorepo-opam-provided: [ "ocaml-freestanding" ]

Opam-monorepo crashes with the following error:

opam-monorepo: internal error, uncaught exception:
               (Failure "duplicate entry ocaml")
               Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33
               Called from OpamStd.Set.Make.fold.(fun) in file "src/core/opamStd.ml", line 190, characters 15-23
               Called from Stdlib__Set.Make.iter in file "set.ml", line 378, characters 35-38
               Called from Stdlib__Set.Make.iter in file "set.ml", line 378, characters 25-33
               Called from OpamStd.Set.Make.fold in file "src/core/opamStd.ml", line 189, characters 6-62
               Called from Duniverse_lib__Opam_solve.Make_solver.build_context in file "lib/opam_solve.ml", line 285, characters 6-69
               Called from Duniverse_lib__Opam_solve.Make_solver.calculate in file "lib/opam_solve.ml", line 505, characters 12-187
               Called from Duniverse_lib__Opam_solve.calculate in file "lib/opam_solve.ml" (inlined), line 642, characters 2-134
               Called from Duniverse_cli__Lock.calculate_opam.(fun) in file "cli/lock.ml", line 302, characters 14-192
               Called from OpamSwitchState.with_.(fun) in file "src/state/opamSwitchState.ml", line 1211, characters 14-18
               Re-raised at OpamStd.Exn.finalise in file "src/core/opamStd.ml", line 1372, characters 4-38
               Called from OpamStd.Exn.finally in file "src/core/opamStd.ml", line 1375, characters 10-14
               Re-raised at OpamStd.Exn.finalise in file "src/core/opamStd.ml", line 1372, characters 4-38
               Called from OpamGlobalState.with_ in file "src/state/opamGlobalState.ml", line 186, characters 14-18
               Re-raised at OpamStd.Exn.finalise in file "src/core/opamStd.ml", line 1372, characters 4-38
               Called from Duniverse_cli__Lock.run in file "cli/lock.ml", line 449, characters 4-143
               Called from Cmdliner_term.app.(fun) in file "src/cmdliner_term.ml", line 24, characters 19-24
               Called from Cmdliner_term.term_result.(fun) in file "src/cmdliner_term.ml", line 40, characters 25-32
               Called from Cmdliner_eval.run_parser in file "src/cmdliner_eval.ml", line 34, characters 37-44

[NathanReb]

Ah yeah I think I see where it's coming from, should be easy to fix!

@Leonidas-from-XIV probably the constraints that aren't dedup in the second run!

[Leonidas-from-XIV]

@TheLortex Thanks for the testing, I appreciate it. I tried with your example first and it didn't work because the solver picked 4.14 which is not compatible with ocaml-freestanding, so I realized you're using --ocaml-version to lock it.

We did indeed have an issue there, because in certain cases we inject the compiler constraint from the command line twice, but the code was written in a way where this would crash. I made sure for it to work as long as the constraint is unchanged.

@NathanReb I added a small regression test into the cram test (that fails with the exact error @TheLortex describes) so this should not happen in the future. It is a bit more code than absolutely necessary because I wanted to make sure the constraint is equal to the constraint that already exists - otherwise there would be some tricky solver problem that we should not just ignore.

[hannesm]

...impatiently waiting for this to be merged and released... thanks for all your hard work on it :)

[NathanReb]

@hannesm it should get in today. We will also focus on the 0.3 release in the next few days so this should all soon be available!

[hannesm]

Thanks again, for the work on this, and for merging it. I'm wondering @NathanReb 8 days ago wrote "release in the next few days", is there still a release planned, or have there been show stoppers encountered? Thanks for you time.

[Leonidas-from-XIV]

@hannesm Yes, we are planning a 0.3.0 release soon. There is a milestone with the missing things for 0.3.0, the only potential show stopper is about the opam vendor variables that needs to be investigated beforehand.