Updated following review

doc/qemu.md

@@ -5,8 +5,15 @@ which can provide an SSH interface.
 
 # Base Images
 
-These need to be provided as boot disks.  There is a `Makefile` in the
-`qemu` directory which builds several base images:
+`obuilder` requires a base image aka a root file system, to use as
+the basis of the container which it creates.  For example the `runc`
+backend extracts the root file system from a Docker base image, while
+a FreeBSD `jail` uses a FreeBSD installation on a ZFS volume.  `qemu`
+requires virtual _hard disks_ with the operating system preinstalled.
+
+In order to use the the [QEMU](https://www.qemu.org) backend with
+`obuilder` base images need to be created. A [Makefile](../qemu/Makefile)
+is provided which builds several base images:
 
 - ubuntu-noble-amd64-ocaml-4.14.2.qcow2
 - ubuntu-noble-amd64-ocaml-5.2.0.qcow2
@@ -17,16 +24,16 @@ These need to be provided as boot disks.  There is a `Makefile` in the
 - windows-server-2022-amd64-ocaml-4.14.2.qcow2
 - windows-server-2022-amd64-ocaml-5.2.0.qcow2
 
-The base images build automatically using Cloud Init on Ubuntu,
-`autounattend.xml` on Windows and `autoinstall` on OpenBSD.
-
-Use `make ubuntu`, `make windows` or `make openbsd`.
+The base images build are build using `make ubuntu`, `make windows` or
+`make openbsd`.  The builds are unattended builds requiring no manual
+intervention. Cloud Init is used on Ubuntu, `autounattend.xml` on Windows
+and `autoinstall` on OpenBSD.
 
 # Operation
 
-A spec which reference the required base image in using the `from`
-directive, then run the whatever commands are required.  An trivial
-example is given below.
+A spec references the required base image using the `from` directive,
+then runs whatever commands are required.  An trivial to install the
+`tar` package from opam on Windows example is given below.
 
 ```
 (
@@ -38,8 +45,12 @@ example is given below.
 )
 ```
 
-A typical invocation via `obuilder build` would be as below.  Note that
-in this example, the base images would be in `/var/cache/obuilder/base-image/*.qcow2`.
+If this spec is saved in the file `test.spec` then a typical invocation
+via `obuilder build` would be as below.
+
+The base images should have been built already and moved to the
+`base-image` folder below the folder specified by `--store`.
+i.e. `/var/cache/obuilder/base-image/*.qcow2`.
 
 ```
 obuilder build --store=qemu:/var/cache/obuilder -v -f test.spec --qemu-memory 16 --qemu-cpus 8 --qemu-guest-os windows .
@@ -52,10 +63,10 @@ successfully, `result-tmp` is moved to `result`:
 ```
 (from windows-server-2022-amd64-ocaml-4.14)
 obuilder: [INFO] Base image not present; importing "windows-server-2022-amd64-ocaml-4.14"…
-obuilder: [INFO] Exec "mkdir" "-m" "755" "--" "/var/lib/docker/test/result-tmp/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101/rootfs"
-obuilder: [INFO] Exec "qemu-img" "create" "-f" "qcow2" "-b" "/var/lib/docker/test/base-image/windows-server-2022-amd64-ocaml-4.14.qcow2" "-F" "qcow2" "/var/lib/docker/test/result-tmp/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101/rootfs/image.qcow2"
-Formatting '/var/lib/docker/test/result-tmp/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101/rootfs/image.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off compression_type=zlib size=42949672960 backing_file=/var/lib/docker/test/base-image/windows-server-2022-amd64-ocaml-4.14.qcow2 backing_fmt=qcow2 lazy_refcounts=off refcount_bits=16
-obuilder: [INFO] Exec "mv" "/var/lib/docker/test/result-tmp/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101" "/var/lib/docker/test/result/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101"
+obuilder: [INFO] Exec "mkdir" "-m" "755" "--" "/var/cache/obuilder/result-tmp/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101/rootfs"
+obuilder: [INFO] Exec "qemu-img" "create" "-f" "qcow2" "-b" "/var/cache/obuilder/base-image/windows-server-2022-amd64-ocaml-4.14.qcow2" "-F" "qcow2" "/var/cache/obuilder/result-tmp/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101/rootfs/image.qcow2"
+Formatting '/var/cache/obuilder/result-tmp/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101/rootfs/image.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off compression_type=zlib size=42949672960 backing_file=/var/cache/obuilder/base-image/windows-server-2022-amd64-ocaml-4.14.qcow2 backing_fmt=qcow2 lazy_refcounts=off refcount_bits=16
+obuilder: [INFO] Exec "mv" "/var/cache/obuilder/result-tmp/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101" "/var/cache/obuilder/result/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101"
 ---> saved as “dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101”
 ```
 
@@ -73,9 +84,9 @@ by an ACPI shutdown command sent to the qemu console.
 ```
 /: (run (cache (opam-archives (target "C:\\Users\\opam\\AppData\\Local\\opam\\download-cache")))
         (shell "opam install tar"))
-obuilder: [INFO] Exec "qemu-img" "create" "-f" "qcow2" "-b" "/var/cache/obuilder/test/result/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101/rootfs/image.qcow2" "-F" "qcow2" "/var/cache/obuilder/test/result-tmp/8a897f21e54db877fc971c757ef7ffc2e1293e191dc60c3a18f24f0d3f0926f3/rootfs/image.qcow2" "40G"
-obuilder: [INFO] Exec "cp" "-pRduT" "--reflink=auto" "/var/cache/obuilder/test/cache/c-opam-archives" "/var/cache/obuilder/test/cache-tmp/0-c-opam-archives"
-obuilder: [INFO] Fork exec "qemu-system-x86_64" "-m" "16G" "-smp" "8" "-machine" "accel=kvm,type=q35" "-cpu" "host" "-nic" "user,hostfwd=tcp::56229-:22" "-display" "none" "-monitor" "stdio" "-drive" "file=/var/cache/obuilder/test/result-tmp/8a897f21e54db877fc971c757ef7ffc2e1293e191dc60c3a18f24f0d3f0926f3/rootfs/image.qcow2,format=qcow2" "-drive" "file=/var/cache/obuilder/test/cache-tmp/0-c-opam-archives/rootfs/image.qcow2,format=qcow2"
+obuilder: [INFO] Exec "qemu-img" "create" "-f" "qcow2" "-b" "/var/cache/obuilder/result/dce4336e183de81da7537728ed710f2906e9f75431694d9de80b95a9d9ff1101/rootfs/image.qcow2" "-F" "qcow2" "/var/cache/obuilder/result-tmp/8a897f21e54db877fc971c757ef7ffc2e1293e191dc60c3a18f24f0d3f0926f3/rootfs/image.qcow2" "40G"
+obuilder: [INFO] Exec "cp" "-pRduT" "--reflink=auto" "/var/cache/obuilder/cache/c-opam-archives" "/var/cache/obuilder/cache-tmp/0-c-opam-archives"
+obuilder: [INFO] Fork exec "qemu-system-x86_64" "-m" "16G" "-smp" "8" "-machine" "accel=kvm,type=q35" "-cpu" "host" "-nic" "user,hostfwd=tcp::56229-:22" "-display" "none" "-monitor" "stdio" "-drive" "file=/var/cache/obuilder/result-tmp/8a897f21e54db877fc971c757ef7ffc2e1293e191dc60c3a18f24f0d3f0926f3/rootfs/image.qcow2,format=qcow2" "-drive" "file=/var/cache/obuilder/cache-tmp/0-c-opam-archives/rootfs/image.qcow2,format=qcow2"
 obuilder: [INFO] Exec "ssh" "opam@localhost" "-p" "56229" "-o" "NoHostAuthenticationForLocalhost=yes" "exit"
 obuilder: [INFO] Exec "ssh" "opam@localhost" "-p" "56229" "-o" "NoHostAuthenticationForLocalhost=yes" "cmd" "/c" "rmdir /s /q 'C:\Users\opam\AppData\Local\opam\download-cache'"
 obuilder: [INFO] Exec "ssh" "opam@localhost" "-p" "56229" "-o" "NoHostAuthenticationForLocalhost=yes" "cmd" "/c" "mklink /j 'C:\Users\opam\AppData\Local\opam\download-cache' 'd:\'"
@@ -110,9 +121,9 @@ The following actions will be performed:
 -> installed tar.3.1.2
 Done.
 # Run eval $(opam env) to update the current shell environment
-obuilder: [INFO] Exec "cp" "-pRduT" "--reflink=auto" "/var/cache/obuilder/test/cache-tmp/0-c-opam-archives" "/var/cache/obuilder/test/cache/c-opam-archives"
-obuilder: [INFO] Exec "rm" "-r" "/var/cache/obuilder/test/cache-tmp/0-c-opam-archives"
-obuilder: [INFO] Exec "mv" "/var/cache/obuilder/test/result-tmp/8a897f21e54db877fc971c757ef7ffc2e1293e191dc60c3a18f24f0d3f0926f3" "/var/cache/obuilder/test/result/8a897f21e54db877fc971c757ef7ffc2e1293e191dc60c3a18f24f0d3f0926f3"
+obuilder: [INFO] Exec "cp" "-pRduT" "--reflink=auto" "/var/cache/obuilder/cache-tmp/0-c-opam-archives" "/var/cache/obuilder/cache/c-opam-archives"
+obuilder: [INFO] Exec "rm" "-r" "/var/cache/obuilder/cache-tmp/0-c-opam-archives"
+obuilder: [INFO] Exec "mv" "/var/cache/obuilder/result-tmp/8a897f21e54db877fc971c757ef7ffc2e1293e191dc60c3a18f24f0d3f0926f3" "/var/cache/obuilder/result/8a897f21e54db877fc971c757ef7ffc2e1293e191dc60c3a18f24f0d3f0926f3"
 ---> saved as "8a897f21e54db877fc971c757ef7ffc2e1293e191dc60c3a18f24f0d3f0926f3"
 Got: "8a897f21e54db877fc971c757ef7ffc2e1293e191dc60c3a18f24f0d3f0926f3"
 ```
@@ -174,6 +185,5 @@ let tar_in ~cancelled ?stdin ~log:_ _ config result_tmp =
   else Lwt_result.fail `Cancelled
 ```
 
-Windows ships with BSD tar in `System32` so we and that does work with an
-`ssh` pipe.
+Windows ships with BSD tar in `System32` and that does work with an `ssh` pipe.
 

lib/build.ml

@@ -35,9 +35,8 @@ module Context = struct
     secrets : (string * string) list;
   }
 
-  let v ?switch ?(env=[]) ?(user=Obuilder_spec.root) ?workdir ?shell ?(secrets=[]) ~log ~src_dir () =
+  let v ?switch ?(env=[]) ?(user=Obuilder_spec.root) ?workdir ?(secrets=[]) ~shell ~log ~src_dir () =
     let workdir = Option.value ~default:(if Sys.win32 then {|C:/|} else "/") workdir in
-    let shell = Option.value ~default:(if Sys.win32 then ["cmd"; "/S"; "/C"] else ["/usr/bin/env"; "bash"; "-c"]) shell in
     { switch; env; src_dir; user; workdir; shell; log; scope = Scope.empty; secrets }
 
   let with_binding name value t =
@@ -148,7 +147,7 @@ module Make (Raw_store : S.STORE) (Sandbox : S.SANDBOX) (Fetch : S.FETCHER) = st
       (* Fmt.pr "COPY: %a@." Sexplib.Sexp.pp_hum (sexp_of_copy_details details); *)
       let id = Sha256.to_hex (Sha256.string (Sexplib.Sexp.to_string (sexp_of_copy_details details))) in
       Store.build t.store ?switch ~base ~id ~log (fun ~cancelled ~log result_tmp ->
-          let argv = Option.value ~default:(["tar"; "-xf"; "-"]) (Sandbox.tar t.sandbox) in
+          let argv = Sandbox.tar t.sandbox in
           let config = Config.v
               ~cwd:"/"
               ~argv
@@ -293,7 +292,7 @@ module Make (Raw_store : S.STORE) (Sandbox : S.SANDBOX) (Fetch : S.FETCHER) = st
     | `Heading | `Note -> Buffer.add_string buffer (x ^ "\n")
     | `Output -> Buffer.add_string buffer x
 
-  let healthcheck ?(timeout=30.0) t =
+  let healthcheck ?(timeout=300.0) t =
     Os.with_pipe_from_child (fun ~r ~w ->
         let result = Docker.Cmd.version ~stderr:(`FD_move_safely w) () in
         let r = Lwt_io.(of_fd ~mode:input) r ~close:Lwt.return in
@@ -306,7 +305,7 @@ module Make (Raw_store : S.STORE) (Sandbox : S.SANDBOX) (Fetch : S.FETCHER) = st
     let log = log_to buffer in
     (* Get the base image first, before starting the timer. *)
     let switch = Lwt_switch.create () in
-    let context = Context.v ~switch ~log ~src_dir:"/tmp" () in
+    let context = Context.v ~shell:(Sandbox.shell t.sandbox) ~switch ~log ~src_dir:"/tmp" () in
     healthcheck_base () >>= function healthcheck_base ->
     get_base t ~log healthcheck_base >>= function
     | Error (`Msg _) as x -> Lwt.return x
@@ -544,7 +543,8 @@ module Make_Docker (Raw_store : S.STORE) = struct
   let df t =
     Store.df t.store
 
-  let shell _ = None
+  let shell t =
+    Docker_sandbox.shell t.sandbox
 
   let root t =
     Store.root t.store
@@ -557,7 +557,7 @@ module Make_Docker (Raw_store : S.STORE) = struct
     | `Heading | `Note -> Buffer.add_string buffer (x ^ "\n")
     | `Output -> Buffer.add_string buffer x
 
-  let healthcheck ?(timeout=if Sys.win32 then 300.0 else 30.0) t =
+  let healthcheck ?(timeout=if Sys.win32 then 300.0 else 300.0) t =
     Os.with_pipe_from_child (fun ~r ~w ->
         let result = Docker.Cmd.version ~stderr:(`FD_move_safely w) () in
         let r = Lwt_io.(of_fd ~mode:input) r ~close:Lwt.return in
@@ -571,7 +571,7 @@ module Make_Docker (Raw_store : S.STORE) = struct
     (* Get the base image first, before starting the timer. *)
     let switch = Lwt_switch.create () in
     let src_dir = if Sys.win32 then {|C:\TEMP|} else "/tmp" in
-    let context = Context.v ~switch ~log ~src_dir () in
+    let context = Context.v ~shell:(Docker_sandbox.shell t.sandbox) ~switch ~log ~src_dir () in
     healthcheck_base () >>= function healthcheck_base ->
     get_base t ~log healthcheck_base >>= function
     | Error (`Msg _) as x -> Lwt.return x

lib/build.mli

@@ -6,8 +6,8 @@ module Context : sig
     ?env:Config.env ->
     ?user:Obuilder_spec.user ->
     ?workdir:string ->
-    ?shell:string list ->
     ?secrets:(string * string) list ->
+    shell:string list ->
     log:S.logger ->
     src_dir:string ->
     unit -> t

lib/docker_sandbox.ml

@@ -2,6 +2,8 @@ open Lwt.Syntax
 let ( >>!= ) = Lwt_result.bind
 open Sexplib.Conv
 
+include S.Sandbox_default
+
 let ( / ) = Filename.concat
 let ( // ) dirname filename =
   if Sys.win32 then
@@ -459,13 +461,6 @@ let create (c : config) =
   let+ () = if Result.is_error volume_exists then create_tar_volume t else Lwt.return_unit in
   t
 
-let finished () =
-  Lwt.return ()
-
-let shell _ = None
-
-let tar _ = None
-
 open Cmdliner
 
 let docs = "DOCKER BACKEND"

lib/obuilder.ml

@@ -1,13 +1,13 @@
 let log_src = Log.src
 
-(** {4 Types} *)
+(** {2 Types} *)
 
 module S = S
 module Spec = Obuilder_spec
 module Context = Build.Context
 module Docker = Docker
 
-(** {7 Stores} *)
+(** {2 Stores} *)
 
 module Btrfs_store = Btrfs_store
 module Zfs_store = Zfs_store
@@ -17,20 +17,20 @@ module Store_spec = Store_spec
 module Docker_store = Docker_store
 module Qemu_store = Qemu_store
 
-(** {4 Fetchers} *)
+(** {2 Fetchers} *)
 module Zfs_clone = Zfs_clone
 module Qemu_snapshot = Qemu_snapshot
 module Docker_extract = Docker.Extract
 module Archive_extract = Archive_extract
 
-(** {3 Sandboxes} *)
+(** {2 Sandboxes} *)
 
 module Config = Config
 module Native_sandbox = Sandbox
 module Docker_sandbox = Docker_sandbox
 module Qemu_sandbox = Qemu_sandbox
 
-(** {3 Builders} *)
+(** {2 Builders} *)
 
 module type BUILDER = S.BUILDER with type context := Build.Context.t
 module Builder = Build.Make

lib/os.ml

@@ -213,12 +213,6 @@ let check_dir x =
   | _ -> Fmt.failwith "Exists, but is not a directory: %S" x
   | exception Unix.Unix_error(Unix.ENOENT, _, _) -> `Missing
 
-let check_file x =
-  match Unix.lstat x with
-  | Unix.{ st_kind = S_REG; _ } -> `Present
-  | _ -> Fmt.failwith "Exists, but is not a regular file: %S" x
-  | exception Unix.Unix_error(Unix.ENOENT, _, _) -> `Missing
-
 let ensure_dir ?(mode=0o777) path =
   match check_dir path with
   | `Present -> ()

lib/qemu_sandbox.ml

@@ -1,6 +1,8 @@
 open Lwt.Infix
 open Sexplib.Conv
 
+include S.Sandbox_default
+
 let ( / ) = Filename.concat
 
 let copy_to_log ~src ~dst =
@@ -85,7 +87,7 @@ let run ~cancelled ?stdin ~log t config result_tmp =
     | 0 -> Lwt_result.fail (`Msg "No connection")
     | n ->
       Os.exec_result ~pp (ssh @ ["exit"]) >>= function
-      | Ok _ -> Lwt_result.ok (Lwt.return ())
+      | Ok _ -> Lwt.return_ok ()
       | _ -> Lwt_unix.sleep 1. >>= fun _ -> loop (n - 1) in
   Lwt_unix.sleep 5. >>= fun _ ->
   loop t.qemu_boot_time >>= fun _ ->
@@ -152,16 +154,13 @@ let create (c : config) =
   let t = { qemu_cpus = c.cpus; qemu_memory = c.memory; qemu_guest_os = c.guest_os; qemu_guest_arch = c.guest_arch; qemu_boot_time = c.boot_time } in
   Lwt.return t
 
-let finished () =
-  Lwt.return ()
-
-let shell _ = Some []
+let shell _ = []
 
 let tar t =
   match t.qemu_guest_os with
-  | Linux -> None
-  | OpenBSD -> Some ["gtar"; "-xf"; "-"]
-  | Windows -> Some ["/cygdrive/c/Windows/System32/tar.exe"; "-xf"; "-"; "-C"; "/"]
+  | Linux -> tar t
+  | OpenBSD -> ["gtar"; "-xf"; "-"]
+  | Windows -> ["/cygdrive/c/Windows/System32/tar.exe"; "-xf"; "-"; "-C"; "/"]
 
 open Cmdliner
 

lib/qemu_sandbox.mli

@@ -1,4 +1,4 @@
-(** Sandbox builds using Docker. *)
+(** Sandbox builds using QEMU. *)
 
 include S.SANDBOX
 

lib/qemu_store.mli

@@ -3,5 +3,5 @@
 include S.STORE
 
 val create : root:string -> t Lwt.t
-(** [create ~path] creates a new overlayfs store where everything will
-    be stored under [path]. *)
+(** [create ~root] creates a new QEMU store directory where everything will
+    be stored under [root]. *)

lib/s.ml

@@ -71,6 +71,13 @@ module type STORE = sig
       so that the free space is accurate. *)
 end
 
+module Sandbox_default =
+  struct
+    let tar _ = ["tar"; "-xf"; "-"]
+    let shell _ = if Sys.win32 then ["cmd"; "/S"; "/C"] else ["/usr/bin/env"; "bash"; "-c"]
+    let finished () = Lwt.return ()
+  end
+
 module type SANDBOX = sig
   type t
 
@@ -89,11 +96,11 @@ module type SANDBOX = sig
       @param log Used for child's stdout and stderr.
   *)
 
-  val shell : t -> string list option
-  (** [shell] optional value to be used as the default shell. *)
+  val shell : t -> string list
+  (** [shell t] Command line for the default shell. *)
 
-  val tar : t -> string list option
-  (** [tar] tar command for this sandbox. *)
+  val tar : t -> string list
+  (** [tar t] Command line to invoke tar for this sandbox. *)
 
   val finished : unit -> unit Lwt.t
 end
@@ -134,7 +141,7 @@ module type BUILDER = sig
   val df : t -> float Lwt.t
   (** [df t] returns the percentage of free space in the store. *)
 
-  val shell : t -> string list option
+  val shell : t -> string list
   (** [shell] optional value to be used as the default shell. *)
 
   val cache_stats : t -> int * int

lib/sandbox.jail.ml

@@ -1,6 +1,8 @@
 open Lwt.Infix
 open Sexplib.Conv
 
+include S.Sandbox_default
+
 let ( / ) = Filename.concat
 
 type t = {
@@ -165,13 +167,6 @@ let create ~state_dir:_ _c =
     jail_name_prefix = "obuilder_" ^ (Int.to_string (Unix.getpid ()));
   }
 
-let finished () =
-  Lwt.return ()
-
-let shell _ = None
-
-let tar _ = None
-
 open Cmdliner
 
 let cmdliner : config Term.t =

lib/sandbox.macos.ml

@@ -1,6 +1,8 @@
 open Lwt.Infix
 open Cmdliner
 
+include S.Sandbox_default
+
 type t = {
   uid: int;
   gid: int;
@@ -114,10 +116,6 @@ let finished () =
   Os.sudo [ "zfs"; "mount"; "obuilder/result" ] >>= fun () ->
   Lwt.return ()
 
-let shell _ = None
-
-let tar _ = None
-
 let uid =
   Arg.required @@
   Arg.opt Arg.(some int) None @@