dune icon indicating copy to clipboard operation
dune copied to clipboard

Add toolchains feature

Open gridbugs opened this issue 1 year ago • 4 comments

Toolchains refers to a feature where dune downloads and builds ocaml compiler packages using an alternative mechanism to regular dune package management. Toolchains are installed into $XDG_CACHE_DIR/dune/toolchains/<name.version>. Both the ocaml-base-compiler and ocaml-variants compilers are supported. In the case of the latter, multiple different configurations may be installed concurrently, by concatenating the names of enabled options to the end of the name of the directory containing the toolchain.

The toolchains feature is a temporary workaround for the fact that the compiler packages are not currently relocatable. That is, the artifacts from the compiler packages built inside dune's build sandbox cannot be used after moving them into the dune cache or their eventual location within the _build directory. Work is currently being done to make the compiler relocatable, at which point dune will be able to build compiler packages in its sandbox and the toolchains feature can be deleted.

gridbugs avatar Jun 11 '24 11:06 gridbugs

I added a few more doc comments to the Toolchain module and revised some of the existing comments. I also renamed Compiler.uninit to Compiler.t and renamed Compiler.t to Compiler.installed as I find that clearer. Now if an interface accepts a Compiler.installed it's clear from the type name that it expects to be passed an installed compiler.

There are a few more unaddressed comments on this PR that I will address tomorrow.

gridbugs avatar Jun 25 '24 14:06 gridbugs

Thanks for the review @rgrinberg!

gridbugs avatar Jun 26 '24 15:06 gridbugs

Looking over the integration with Pkg_rules again, I pointed out a few flaws I didn't notice earlier. I know you've dismissed this earlier, but I can't help but think your job could have been made much easier by making a toolchain a pseudo package that just installs the toolchain and creates an install cookie file. Otherwise I'm not sure how we can even maintain the same semantics we've had before.

rgrinberg avatar Jun 27 '24 07:06 rgrinberg

The review is a bit scattered, so let me just summarize the state of affairs so that the priorities are clear.

  • The interaction with opam repo when building needs to go away. This is critical.
  • The remaining bugs can we can make a note of and move on. If we're lucky, the users will never run into them.
  • The design questions can be stopped for now. Let's just concentrate on keeping things in one place in dune_rules, or moving the code to Dune_pkg otherwise.

rgrinberg avatar Jun 27 '24 08:06 rgrinberg

Looking over the integration with Pkg_rules again, I pointed out a few flaws I didn't notice earlier. I know you've dismissed this earlier, but I can't help but think your job could have been made much easier by making a toolchain a pseudo package that just installs the toolchain and creates an install cookie file. Otherwise I'm not sure how we can even maintain the same semantics we've had before.

Can you elaborate a bit on this. Are you proposing to represent a toolchain with a regular package (that gets a lockfile like any other package), where dune would recognize the package as representing a toolchain, and install the compiler into the toolchain directory when building the package? That way we could use the existing machinery in pkg_rules for updating paths.

gridbugs avatar Jul 05 '24 02:07 gridbugs

@rgrinberg I've started experimenting with a more conservative implementation of toolchains based on your suggestion. The only difference is in pkg_rules, where the toolchain is installed as if it was the install command of compiler packages. It's not completely working yet as I'm not providing the new Ocaml_toolchain.t. Can you take a look at https://github.com/ocaml/dune/pull/10698/files#diff-ed8949e9f1cea7cfb9eee6f20d456401dc3755949ed4d24268d1c861f24d69b7 and let me know if I'm on the right track please.

gridbugs avatar Jul 05 '24 08:07 gridbugs

Can you elaborate a bit on this. Are you proposing to represent a toolchain with a regular package (that gets a lockfile like any other package), where dune would recognize the package as representing a toolchain, and install the compiler into the toolchain directory when building the package? That way we could use the existing machinery in pkg_rules for updating paths.

Basically, there's nothing special between a toolchain and any regular package apart from how we build it. A regular package is installed by running an action that builds the package in _build. A non relocatable package on the other hand, builds it in a global directory. The package rules could work equally well with both types of packages as long as the relevant types are updated to reflect that. In particular, Paths.t, Install_cookie.t, Pkg.t are of interest. Some of these types will need to be updated to allow external paths by generalizing Path.Build.t to Path.t. Others, you might need to introduce explicit variants to special case things.

Can you take a look at #10698 (files) and let me know if I'm on the right track please.

Yeah, that seems to be on the right track:

Fetching a non relocatable package should work in the same way. Or at least I don't see why should it be any different. So that work seems unnecessary.

The meat of the work is implementing a custom build/install action that is going to reuse your custom building/caching logic instead of reusing dune's. A few things to keep in mind:

  1. For modifying the build/install actions, instead of adding an "is_compiler" check. Which as I mentioned earlier, is an imprecise way of naming a non-relocatable package, you could expand Build_command.t (the install command will need similar treatment) to:
  type t =
    | Action of Action.t
    | Dune
    | Non_relocatable of Action.t (* you might want to pick something simpler than Action.t as the argument *)

All that should remain is implementing a custom action to run your build/install commands using your existing code

  1. Note that you will still need to produce the install cookie file in _build, but this cookie file should point to your external paths.

  2. You will also need to copy the source paths to your global directory manually as well. As dune will not understand that you need them outside the build directory.

  3. Finally, if your action encounters a compiler that has already been built for the configuration that you're looking for, it should do no work and just produce a cookie pointing to the existing install. A good way to do this is to maintain a cache of non relocatable packages keyed by md5 (source_checksum, action_digest) in your state directory. But something simpler will work too if don't care about caching reliably.

  4. You should disable sandboxing for the build/install rule. Those don't make sense for non-relocatable packages. When another package depends on a non-relocatable package, we exploit the fact that external paths Path.External.t never get copied to the sandbox even if they are dependencies. Otherwise, we'd need to disable sandboxing everywhere.

To do all this, I suggest you start by defining the simplest non relocatable package:

(version 0.0.1)
(relocatable false)
;; when you depend on this package and print `%{lib}/foo`, you should see a path pointing to your global dir
(install (run bash -i "mkdir -p %{lib}; echo $PWD > %{lib}/foo"))

If you can make it work for the above, you should be able to make it work for the complier.

rgrinberg avatar Jul 05 '24 23:07 rgrinberg

Hey thanks for the detailed suggestions.

I want to share a bit of context from inside the build system team. Previously we had a long-lived branch that includes the toolchains feature, and the goal of upstreaming this feature was so we no longer have to keep it up to date with main by periodically merging main into our long-lived branch. It's a temporary (🤞) workaround to the compiler's relocation issue, and we just needed this feature to unblock some developer interviews evaluating other more user-visible features, so we prioritized implementing it quickly.

We (the build system team) don't want to spend much more time upstreaming this feature. We're going to start using the long-lived toolchains branch again internally (including changes from this review) for our new feature work rather than focusing on upstreaming toolchains. I don't want anyone on this review to feel like it was a waste of time. I'm very grateful for all the feedback and improvements made to the branch!

I'm still interested in seeing if we can upstream a version of this feature, though given it's a temporary (🤞) workaround and I'm planning to spend most of my time on new feature work, I can't justify implementing the solution you're proposing @rgrinberg. My plan is to instead:

  • remove the interaction with the opam repo from toolchains and by using the source urls and hashes from the compiler package lockfile rather than looking them up in the opam repo
  • install the compiler into the toolchains dir as if doing so was a regular package install command (ie. just from the install_command function in pkg_rules) rather than ensuring that it's installed in other parts of pkg_rules
  • add the compiler bin directory to PATH that same way as we update PATH for regular packages

The general idea would be to make this change more conservative by reducing its footprint on the pkg_rules module. Of course, all of this functionality will be disabled with a feature flag anyway, but making it more conservative will still help make sure we didn't subtly break something and also keep the contract between pkg_rules and the rest of dune from getting too complex.

gridbugs avatar Jul 08 '24 02:07 gridbugs

Also to clarify, implementing this feature correctly is something I want to do eventually but if there's a way to quickly upstream something that solves the problem now I'd like to do that first.

gridbugs avatar Jul 08 '24 08:07 gridbugs

Sounds fine with me. If you wait long enough, maybe relocation will arrive and you will never need to implement this feature properly anyway. I'd say if this feature maintains the "illusion" that the compiler is first class. To maintain the illusion, I think this requires:

  1. No interaction with opam repo during build/install time
  2. Allowing users to pin the compiler

I think this is good enough for alphas and betas.

rgrinberg avatar Jul 09 '24 21:07 rgrinberg

Thanks for the feedback everyone. Closing this in favour of a new PR that re-implements this functionality based on the feedback. https://github.com/ocaml/dune/pull/10719.

gridbugs avatar Jul 12 '24 08:07 gridbugs