Experimental Eio-port by patricoferris · Pull Request #113 · ocurrent/obuilder

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 = 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

  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

	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 ->

Conversation

patricoferris commented Aug 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talex5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MisterDA commented Jan 9, 2023

Uh oh!

talex5 commented Jan 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants