WIP: Fix opam-repo commit selection for lint-fmt

[NathanReb]

Fixes #398

This is still work in progress but a bit of feedback on this would be greatly appreciated!

CC@emillon who was my pair programming partner on this and who wrote the nice summary below:

Ways to improve this / future work

Making it correct

At the moment we just pick the first solution and hope that it will be compatible with the worker that is selected for the lint-fmt job (first in the list of solutions for the main build), but that is not guaranteed.

We can return the whole selection (where we return just a commit), and use that instead of the first compatible worker. That scheme is also used for other kinds of lint builds (opam and doc) so they would need to be adjusted. This seems to be the best solution but requires more work.

Some older versions of ocamlformat could have a different behavior depending on the version of ocaml they have been compiled with. This is not the case with recent versions of ocaml and ocamlformat, but if we want to support that we would need to select the worker with most recent compiler (from the list of compatible ones).

The candidate implementation calls the solver for all platforms and only returns the first one. This is slow (see below) but in the case we keep the "use first worker for lint builds" strategy, it is possible just call the solver once by first. No guarantee that it always works.

Making it faster

If the solution needs to call the solver on all platform and just pick one, it is possible to change the solver API so that rather than querying for solves on all platforms, waiting for all the results and using List.hd, we instead pass a flag asking for early return of the first solution (akin to usingLIMIT 1 in SQL rather than List.hd).

Finally, if the function that maps ocamlformat+dune version toopam-repository commit is too expensive to compute (it's at least an extra solver call in the analysis phase, possibly more), we can cache it separately by altering the pipeline a bit:

    opam-repo -----------------> analyse-fmt -> lint-fmt
              -,             ,->
               |             |
               '-->         -'
         repo ----> analyse ---> build

In this new pipeline, analyse returns (for the formatting part) only the parsed ocamlformat and dune constraints instead of the opam-repository commit to use. And a new node (analyse-fmt) matches these constraints against the current state of opam-repository. This part is computed and cached separately.

Code TODOs

• Fix the test suite
• there's a spec line that is specific to ocaml < 4.08 in formatting jobs. We could probably drop this.
• more generally it looks like this part is about installing packages from opam after resetting opam-repository to a known version. there is already code to do that
• deduplicate the opam file generation function

[NathanReb]

This happened again: https://ci.ocamllabs.io/github/ocamllabs/opam-monorepo/commit/565c2a8742680a0d571d7a305e3c6f7d8b5fd8cc/variant/(lint-fmt)

This needs to be refreshed and reviewed so we can move on with this.

CC @maiste @tmcgilchrist. We had a discussion recently about having to declare the ocamlformat dependency in the metadata. If we decide to go down that road that fix might not be needed anymore so please let me know!

[maiste]

Hi @NathanReb,
I agree with you! IMHO, having the ocamlformat version within the opam file metadata is still the best thing to do. However, it seems reasonable to allow users not to do it as some people don't like to add the dev dependencies within their opam files.

Another thing, maybe it would be worth going more profound in the code structure to find some .ocamlformat as it could be at the bottom of the tree structure. WDYT?

[dra27]

Fly-by, but opam 2.2 adds a tools predicate (dev has other meanings, and isn’t appropriate for use for this IIRC) but the we may be able to get more opinionated about that with 2.2

[dra27]

(The opam feature is ocaml/opam#5016)

[maiste]

This is a helpful feature to have in opam and we can add it to the lint-fmt stage when it will be available.
However, as opam.2.2 isn't released yet, we still need to find a way to solve the problem for older versions 😕

[dra27]

Not necessarily - the predicate is compatible with older versions (predicates not recognised by older opams will just be assumed to be false)... the useful part here is that you'd be specifying the required version in your opam file for the benefit of opam 2.2, but ocaml-ci still gets to claim zero configuration because it can piggy-back on information which is already required to be there.

[NathanReb]

That sounds like the better option to me as well, this information does belong in the opam file as that's what you'll ultimately use to install those tools. Additionally that could provide useful information on dev tools usage in opam!

At the moment you basically have to read .ocamlformat's content and install ocamlformat to the right version by hand or add this to some Makefile target and maintain the version in two places.

@maiste do you have any specific plans to move forward on this?

[NathanReb]

@dra27 I was recently surprised to get errors for undefined variables though. We had to use ?var_name instead which will default to false when var_name is not defined.

[NathanReb]

Another thing, maybe it would be worth going more profound in the code structure to find some .ocamlformat as it could be at the bottom of the tree structure. WDYT?

I personally always have mines at the root and I have never worked on a project that was hiding it somewhere else so I don't feel a particular need for that specific feature but you might no more than I do about this. Ultimately it's not my decision to make.

I do care about a fix for the opam-repo selection bug though as it has prevented me from upgrading ocamlformat several times now! I'd be more than willing to add it to my opam and test the suggested feature once someone's started working on it if that can help!

[maiste]

I plan to work on it next week as I still have some work to finish this week on my TODO list.

[maiste]

As the linting situation has been merged in #472 (which was a rebase and an upgrade from this one), I close this draft PR. Feel free to reopen in any case.