Experimental Eio-port #113

patricoferris · 2022-08-07T15:11:45Z

This is a very experimental PR/branch that starts trying to port OBuilder to Eio. As part of some other work I wanted to start tackling ocaml-multicore/eio#126 so I was looking for a good candidate that made heavy use of subprocesses and redirections!

This branch is currently building with this branch of Eio: https://github.com/patricoferris/eioio/tree/processes

The trickiest part at the moment seems to be how to handle cancellation correctly. I was thinking of maybe just having a global switch that's passes in via Build.build that everything uses to spawn fibers etc. and that can be the switch that is cancelled if the user does -- any thoughts on this @talex5 ?

kit-ty-kate · 2022-08-07T21:48:40Z

lib/build_log.ml

+        try 
+          match Flow.read ch (Cstruct.sub buf 0 max_chunk_size) with
+          | 0 -> Promise.resolve set_th (Ok ())
+          | n -> dst (Cstruct.to_string buf ~off:0 ~len:n); aux ()
+        with End_of_file -> Promise.resolve set_th (Ok ())


Suggested change

try

match Flow.read ch (Cstruct.sub buf 0 max_chunk_size) with

| 0 -> Promise.resolve set_th (Ok ())

| n -> dst (Cstruct.to_string buf ~off:0 ~len:n); aux ()

with End_of_file -> Promise.resolve set_th (Ok ())

match Flow.read ch (Cstruct.sub buf 0 max_chunk_size) with

| 0 -> Promise.resolve set_th (Ok ())

| n -> dst (Cstruct.to_string buf ~off:0 ~len:n); aux ()

| exception End_of_file -> Promise.resolve set_th (Ok ())

talex5

Thanks for experimenting with this! I skimmed it and added a few comments.

The trickiest part at the moment seems to be how to handle cancellation correctly. I was thinking of maybe just having a global switch that's passes in via Build.build that everything uses to spawn fibers etc. and that can be the switch that is cancelled if the user does -- any thoughts on this @talex5 ?

Ideally, we just remove almost everything to do with cancellation. If the user wants to cancel a build they can just cancel the fiber running it and let Eio's cancellation system sort it out.

talex5 · 2022-08-08T10:26:31Z

lib/btrfs_store.ml

-let purge path =
-  Sys.readdir path |> Array.to_list |> Lwt_list.iter_s (fun item ->
+let purge t path =
+  Sys.readdir path |> Array.to_list |> Fiber.iter (fun item ->


Note: the direct translation of Lwt_list.iter_s is List.iter.

talex5 · 2022-08-08T10:26:58Z

dune

-  (libraries lwt lwt.unix fmt fmt.cli fmt.tty tar-unix obuilder cmdliner logs.fmt logs.cli))
+  (libraries eio_main fmt fmt.cli fmt.tty tar-unix obuilder cmdliner logs.fmt logs.cli))
+
+(vendored_dirs lwt_eio.0.2 eioio tar.2.0.1)


It's called eio now. You might want to rename your fork.

talex5 · 2022-08-08T10:27:58Z

lib/btrfs_store.ml

    let args = "btrfs" :: args in
    let args = if sudo && not running_as_root then "sudo" :: args else args in
-    Os.exec ~stdout:`Dev_null args
+    Switch.run @@ fun sw ->
+    Os.exec ~sw ~process:t.process args


Os.exec shouldn't take a switch, because when it returns the process has already finished.

talex5 · 2022-08-08T10:28:41Z

lib/btrfs_store.ml

-let check_kernel_version () =
-  Os.pread ["uname"; "-r"] >>= fun kver ->
+let check_kernel_version process =
+  let kver = Switch.run @@ fun sw -> Os.pread ~sw ~process ["uname"; "-r"] in


Shouldn't have a switch here either.

talex5 · 2022-08-08T10:34:33Z

lib/build.ml

-               )
-             >>= fun mounts ->
+            let mounts =  
+              Fiber.map (fun { Obuilder_spec.Cache.id; target; buildkit_options = _ } ->


For a direct translation:

Suggested change

Fiber.map (fun { Obuilder_spec.Cache.id; target; buildkit_options = _ } ->

List.map (fun { Obuilder_spec.Cache.id; target; buildkit_options = _ } ->

talex5 · 2022-08-08T10:53:19Z

lib/build_log.mli

 (** [empty] is a read-only log with no content. *)

-val of_saved : string -> t Lwt.t
+val of_saved : Eio.Dir.t -> string -> t


Suggested change

val of_saved : Eio.Dir.t -> string -> t

val of_saved : Eio.Path.t -> t

talex5 · 2022-08-08T10:54:40Z

lib/db_store.ml

-    log : Build_log.t Lwt.t;
-    result : (([`Loaded | `Saved] * S.id), [`Cancelled | `Msg of string]) Lwt_result.t;
+    set_cancelled : unit Promise.u;         (* Resolve this to cancel (when [users = 0]). *)
+    log : (Build_log.t, exn) result Promise.t;


Suggested change

log : (Build_log.t, exn) result Promise.t;

log : Build_log.t Promise.or_exn;

talex5 · 2022-08-08T10:59:01Z

lib/os.ml

-  let r, w = Lwt_unix.pipe_in ~cloexec:true () in
-  let w = { raw = w; needs_close = true } in
-  Lwt.finalize
+let with_pipe_from_child ~sw fn =


Suggested change

let with_pipe_from_child ~sw fn =

let with_pipe_from_child fn =

Switch.run @@ fun sw ->

talex5 · 2022-08-08T11:01:00Z

lib/s.ml

@@ -65,13 +65,16 @@ module type SANDBOX = sig
  type t

  val run :
-    cancelled:unit Lwt.t ->
-    ?stdin:Os.unix_fd ->
+  sw:Eio__core.Switch.t ->


Shouldn't need a switch here. Everything will be finished when run returns.

talex5 · 2022-08-08T11:01:22Z

lib/s.ml

+  sw:Eio__core.Switch.t ->
+    dir:#Eio.Dir.t ->
+    process:Eio.Process.t ->
+    cancelled:unit Eio.Promise.t ->


Can probably remove this and use Eio's built-in cancellation.

MisterDA · 2023-01-09T11:57:39Z

Are there any blockers apart from applying the suggestions from the code review and rebasing? I'd definitely enjoy debugging with backtraces (hoping that the switch to OCaml 5 and Eio would provide them).

talex5 · 2023-01-13T11:24:40Z

Are there any blockers apart from applying the suggestions from the code review and rebasing? I'd definitely enjoy debugging with backtraces (hoping that the switch to OCaml 5 and Eio would provide them).

However, you might not enjoy that it will no longer work on Windows :-( See ocaml-multicore/eio#125 for remaining issues if you want to help!

patricoferris added 3 commits August 7, 2022 16:04

Initial port to Eio

0f6d5be

Cancellation part 1

6eb22d4

Enable more tests

0d1d9fa

kit-ty-kate reviewed Aug 7, 2022

View reviewed changes

talex5 reviewed Aug 8, 2022

View reviewed changes

MisterDA force-pushed the master branch 2 times, most recently from 89e84ac to ccd3f2f Compare February 3, 2023 12:39

benmandrew mentioned this pull request Mar 27, 2023

API for spawning sub-processes ocaml-multicore/eio#126

Closed

5 tasks

Upgrade to unreleased eio

b01d0b1

patricoferris mentioned this pull request Jul 17, 2023

Add eio backend for tar mirage/ocaml-tar#132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental Eio-port #113

Experimental Eio-port #113

patricoferris commented Aug 7, 2022

kit-ty-kate Aug 7, 2022

talex5 left a comment

talex5 Aug 8, 2022

talex5 Aug 8, 2022

talex5 Aug 8, 2022

talex5 Aug 8, 2022

talex5 Aug 8, 2022

talex5 Aug 8, 2022

talex5 Aug 8, 2022

talex5 Aug 8, 2022

talex5 Aug 8, 2022

talex5 Aug 8, 2022

MisterDA commented Jan 9, 2023

talex5 commented Jan 13, 2023

	Fiber.map (fun { Obuilder_spec.Cache.id; target; buildkit_options = _ } ->
	List.map (fun { Obuilder_spec.Cache.id; target; buildkit_options = _ } ->

	val of_saved : Eio.Dir.t -> string -> t
	val of_saved : Eio.Path.t -> t

	log : (Build_log.t, exn) result Promise.t;
	log : Build_log.t Promise.or_exn;

	let with_pipe_from_child ~sw fn =
	let with_pipe_from_child fn =
	Switch.run @@ fun sw ->

Experimental Eio-port #113

Are you sure you want to change the base?

Experimental Eio-port #113

Conversation

patricoferris commented Aug 7, 2022

Choose a reason for hiding this comment

talex5 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisterDA commented Jan 9, 2023

talex5 commented Jan 13, 2023