Add experimental toolchain subcommand #10470

gridbugs · 2024-04-30T12:49:37Z

This adds a command and module for downloading, building, and installing the ocaml compiler toolchain inside the user's ~/.cache directory.

For now this works by hardcoding specific supported versions of the compiler toolchain. In the future we will probably changes this to read information about toolchain packages from the opam repo.

Here's an example of running the dune toolchain exec <VERSION> <PROG> <ARGS> command which downloads, builds and installs a toolchain version before running a command in an environment where the toolchain's bin directory is prepended to PATH.

$ dune exec bin/main.exe -- toolchain exec 5.1.1 ocaml
Will install version 5.1.1 of the compiler toolchain to
/Users/s/.cache/dune/toolchains/5.1.1/target
Downloading...
Configuring...
Building...
Installing...
Success! Compiler toolchain version 5.1.1 installed to
/Users/s/.cache/dune/toolchains/5.1.1/target.
OCaml version 5.1.1
Enter #help;; for help.

There is also a dune toolchain get <VERSION> command that just downloads, builds and installs the specified toolchain.

The subcommands are just here to allow us to test toolchain logic as we work on it. The main component of this change is the module defined in dune_pkg/toolchain.ml which defines the logic for downloading, building and installing toolchains.

rgrinberg · 2024-04-30T13:48:33Z

I think this PR is missing a description of the motivation why this command is needed.

Leonidas-from-XIV

I think this is good, but it can probably be even further simplified with some amount of Unix trickery :-)

Leonidas-from-XIV · 2024-04-30T14:00:14Z

bin/toolchain.ml

+    ; `Blocks Common.help_secs
+    ]
+  in
+  Cmd.info "toolchain" ~doc ~man


I think it would be better if this was something like x-experimental-toolchain as we don't want people to call dune toolchain get 4.14 themselves, this is only something for the cram tests.

Leonidas-from-XIV · 2024-04-30T14:15:09Z

otherlibs/stdune/src/fpath.ml

+let touch ?(perms = 0o666) path =
+  let fd = Unix.openfile path [ O_CREAT ] perms in
+  Unix.close fd
+;;


I was testing the behavior of touch and Unix.openfile and I think this only matches when touch is creating a file. touch is also often used to update the timestamps (I assume that's what its name refers to), which this doesn't do.

$ stat foo Access: 2024-04-30 16:09:47.051473084 +0200 Modify: 2024-04-30 16:09:47.051473084 +0200 Change: 2024-04-30 16:09:47.051473084 +0200 Birth: 2024-04-30 16:09:47.051473084 +0200 $ touch foo $ stat foo Access: 2024-04-30 16:11:11.278876572 +0200 Modify: 2024-04-30 16:11:11.278876572 +0200 Change: 2024-04-30 16:11:11.278876572 +0200 Birth: 2024-04-30 16:09:47.051473084 +0200

Surprisingly Unix.openfile path [ O_CREAT ] doesn't even seem to change the atime.

I suggest either renaming this into something like ensure_exists or implementing the timestamp update (as something like updating timestamps can indeed be useful for cache operations etc).

Leonidas-from-XIV · 2024-04-30T14:22:51Z

src/dune_pkg/toolchain.ml

+        [ Pp.textf "Expected %s to be a directory but it is not." (Path.to_string path) ]
+  ;;
+
+  let cache_dir () =


I think this can be simplified using Xdg.cache_dir.

src/dune_pkg/toolchain.ml

Leonidas-from-XIV · 2024-04-30T14:29:41Z

src/dune_pkg/toolchain.ml

+  run_command
+    ~dir:source_dir
+    configure_script
+    [ sprintf "--prefix=%s" (Path.to_string prefix) ]


Suggested change

[ sprintf "--prefix=%s" (Path.to_string prefix) ]

[ "--prefix"; (Path.to_string prefix) ]

I just checked OCaml's configure script also accepts --prefix <path> so we can simplify this a tiny amount.

src/dune_pkg/toolchain.ml

Leonidas-from-XIV · 2024-04-30T14:51:19Z

src/dune_pkg/toolchain.ml

+module Version = struct
+  type t =
+    | V_4_14_2
+    | V_5_1_1


I think simplifying this to type t = string would be enough, otherwise the maintenance if 4.14.3 or any other new version happens would start to explode.

Leonidas-from-XIV · 2024-04-30T14:52:39Z

bin/toolchain.ml

+      "exec"
+      ~doc:
+        "Run a command in an environment which includes a toolchain's bin directory in \
+         PATH after making sure that the specified toolchain version is intsalled."


Suggested change

PATH after making sure that the specified toolchain version is intsalled."

PATH after making sure that the specified toolchain version is installed."

bin/toolchain.ml

gridbugs · 2024-05-01T03:20:22Z

I think this PR is missing a description of the motivation why this command is needed.

I tried to motivate the commands with:

The subcommands are just here to allow us to test toolchain logic as we work on it.

Note that this PR is targeting a newly created "toolchains" branch rather than main. Once we have a working prototype of the toolchains feature we'll make a PR against main and possibly remove the toolchain subcommand altogether.

gridbugs · 2024-05-01T03:21:54Z

I'll get rid of the toolchain exec subcommand. It was just a little experiment to see if such a command would be useful in practice but it complicates the change and doesn't help us evaluate the toolchains concept.

gridbugs · 2024-05-01T08:26:37Z

I think I've applied all the feedback. Main changes are that I removed the exec subcommand and changed the make install command to pass DESTDIR so it is initially installed in a temporary directory (inside the version's toolchain directory) before copying the result into the final install directory. This lets us use the existence of the install directory to check whether the toolchain is installed rather than needing to use a separate file (this way we don't need to add a touch function).

The only slightly awkward bit is that when installing with DESTDIR the absolute path to the final install dir is instantiated inside the DESTDIR directory. Copying the target dir out of DESTDIR and into its final position requires a bit of messing around with paths and necessitated exposing Path.External.root.

This adds a command and module for downloading, building, and installing the ocaml compiler toolchain inside the user's ~/.cache directory. For now this works by hardcoding specific supported versions of the compiler toolchain. In the future we will probably changes this to read information about toolchain packages from the opam repo. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

Leonidas-from-XIV

Looks good! I've added a few minor suggestions.

Testing it it seems to have failed with the cross device link error (with the fix in #10214 I assume), might be useful to cherry-pick that one onto the toolchains branch.

Leonidas-from-XIV · 2024-05-01T13:47:09Z

src/dune_pkg/toolchain.ml

+       | Some p -> p
+       | None ->
+         User_error.raise
+           ~hints:[ Pp.text "Install \"make\" with your system package manager." ]


Nitpick: I think we have a Pp formatter for commands.

I intentionally left this with regular quotes because it's referring to the package name and not the command.

Leonidas-from-XIV · 2024-05-01T13:48:51Z

src/dune_pkg/toolchain.ml

+  type t = string
+
+  let all = [ "4.14.2"; "5.1.1" ]
+  let all_by_string = List.map all ~f:(fun t -> t, t)


This appears to be unused, maybe not necessary anymore?

It's still used to create the Version.conv value used when parsing arguments to dune x-experimental-toolchain get.

Leonidas-from-XIV · 2024-05-01T13:50:16Z

src/dune_pkg/toolchain.ml

+
+  let toolchain_dir t =
+    let d = Path.relative (Dir.toolchain_base_dir ()) (to_string t) in
+    Dir.make_and_check_dir d;


make_and_check_dir is already called by toolchain_base_dir and while it doesn't hurt, it also doesn't help.

Maybe make_and_check_dir could just get inlined into toolchain_base_dir?

Leonidas-from-XIV · 2024-05-01T13:53:31Z

src/dune_pkg/toolchain.ml

+     them into the target directory. This two stage installation means
+     that we can guarantee that if the target directory exists then it
+     contains the complete installation of the toolchain. *)
+  let tmp_install_dir t = Path.relative (toolchain_dir t) "_install"


Suggestion: instead of calling this $XDG_CACHE_DIR/dune/toolchains/_install it could contain the name of the toolchain in addition. That way the risk of clashes is minimized, if for some reason two toolchains are built at the same time, while making this only very marginally more complex.

(Reducing the risk completely would require locking and I would argue that its not worth the complexity at this moment)

The temporary directory is actually $XDG_CACHE_DIR/dune/toolchains/<version>/_install. I added a comment which hopefully clarifies this.

gridbugs · 2024-05-01T14:53:00Z

Thanks for the feedback @Leonidas-from-XIV! In the interest of simplifying my subsequent PR I'm going to merge this now as is and apply all your additional feedback in a later change.

rgrinberg · 2024-05-04T20:30:38Z

Thanks for clarifying. The implementation of the feature looks fine, but the UX needs to be different. Manually managing toolchains doesn't really work with the kind of guarantees we provide with the dune. So I'd like it if we could think of a way to hide all of this from the user. For example, it's completely fine to use this toolchain mechanism to build the compiler when executing the rules. Even if that requires working around the engine a little.

gridbugs · 2024-05-07T12:15:46Z

Manually managing toolchains doesn't really work with the kind of guarantees we provide with the dune.

The plan is to have dune automatically download, build and install the toolchain necessary to build a project while building the project. I added a command for manually managing toolchains so that the logic for managing toolchains can be exercised without needing to set up a project. Can you clarify whether you still see this as posing a problem?

rgrinberg · 2024-05-07T12:29:09Z

Without setting up a project or a workspace, how would one specify dependencies anyway?

Leonidas-from-XIV · 2024-05-08T08:58:35Z

The dependencies would need to be specified in the project indeed. The toolchain command can be used to manually install a compiler, no need for a project for that as they are in a shared location and accessible from any project that specifies a matching version of the compiler.

rgrinberg · 2024-05-08T12:42:22Z

So why not specify the "toolchain" inside the project as a dependency in the first place?

rgrinberg · 2024-05-08T12:54:50Z

The toolchain command can be used to manually install a compile

Note that this is the highly undesirable part. There's no need for manual installation or anything and installation itself shouldn't even be a concept in dune apart from the dune install command.

Leonidas-from-XIV · 2024-05-08T13:45:43Z

I think we actually agree and the issue is just one of a misunderstanding of the purpose of the CLI.

There's no need for manual installation

Yes, that's the idea, that if you have say, ocaml.4.14.2 in your lockfile, it would automatically build the compiler in the toolchain folder. The CLI command is just for developers to test the installation manually (hence I suggested calling it x-experimental-toolchain, so users don't mistake it for a command that's user-facing and has stability guarantees), like a debug console for mechanics or a hidden cheat console for game testers.

In a way this is more comparable to dune_cmd than to dune pkg.

rgrinberg · 2024-05-08T15:16:19Z

If it's just for testing, you could also just make it a separate binary. This particular detail isn't that important though.

gridbugs requested a review from Leonidas-from-XIV April 30, 2024 12:49

gridbugs force-pushed the toolchain-command branch 6 times, most recently from 6bed95a to 8dbec6e Compare April 30, 2024 13:30

Leonidas-from-XIV reviewed Apr 30, 2024

View reviewed changes

gridbugs force-pushed the toolchain-command branch from 8dbec6e to 54835c9 Compare May 1, 2024 08:21

gridbugs force-pushed the toolchain-command branch from 54835c9 to f204c49 Compare May 1, 2024 08:30

Leonidas-from-XIV approved these changes May 1, 2024

View reviewed changes

gridbugs merged commit 230e7fb into ocaml:toolchains May 1, 2024
26 of 27 checks passed

gridbugs mentioned this pull request May 2, 2024

Simplify and clarify some toolchain code #10477

Merged

gridbugs deleted the toolchain-command branch May 2, 2024 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add experimental toolchain subcommand #10470

Add experimental toolchain subcommand #10470

gridbugs commented Apr 30, 2024

rgrinberg commented Apr 30, 2024

Leonidas-from-XIV left a comment

Leonidas-from-XIV Apr 30, 2024

Leonidas-from-XIV Apr 30, 2024

Leonidas-from-XIV Apr 30, 2024

Leonidas-from-XIV Apr 30, 2024

Leonidas-from-XIV Apr 30, 2024

Leonidas-from-XIV Apr 30, 2024

gridbugs commented May 1, 2024

gridbugs commented May 1, 2024

gridbugs commented May 1, 2024

Leonidas-from-XIV left a comment

Leonidas-from-XIV May 1, 2024

gridbugs May 2, 2024

Leonidas-from-XIV May 1, 2024

gridbugs May 2, 2024

Leonidas-from-XIV May 1, 2024

Leonidas-from-XIV May 1, 2024

gridbugs May 2, 2024

gridbugs commented May 1, 2024

rgrinberg commented May 4, 2024

gridbugs commented May 7, 2024

rgrinberg commented May 7, 2024

Leonidas-from-XIV commented May 8, 2024

rgrinberg commented May 8, 2024

rgrinberg commented May 8, 2024 •

edited

Loading

Leonidas-from-XIV commented May 8, 2024

rgrinberg commented May 8, 2024 •

edited

Loading

	[ sprintf "--prefix=%s" (Path.to_string prefix) ]
	[ "--prefix"; (Path.to_string prefix) ]

	PATH after making sure that the specified toolchain version is intsalled."
	PATH after making sure that the specified toolchain version is installed."

Add experimental toolchain subcommand #10470

Add experimental toolchain subcommand #10470

Conversation

gridbugs commented Apr 30, 2024

rgrinberg commented Apr 30, 2024

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gridbugs commented May 1, 2024

gridbugs commented May 1, 2024

gridbugs commented May 1, 2024

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gridbugs commented May 1, 2024

rgrinberg commented May 4, 2024

gridbugs commented May 7, 2024

rgrinberg commented May 7, 2024

Leonidas-from-XIV commented May 8, 2024

rgrinberg commented May 8, 2024

rgrinberg commented May 8, 2024 • edited Loading

Leonidas-from-XIV commented May 8, 2024

rgrinberg commented May 8, 2024 • edited Loading

rgrinberg commented May 8, 2024 •

edited

Loading

rgrinberg commented May 8, 2024 •

edited

Loading