defunctorise MCLOCK, PCLOCK, TIME, RANDOM#1599
Conversation
|
I haven't looked at the details at all but the resulting |
|
I added a commit which automatically adds a unit argument to delay execution of I tried to use Feedback & reviews are welcome. I'm eager to get this merged rather sooner than later and fix thereafter potential breakage. Looking at the mirage-skeleton changes, I'm not afraid of these changes, but I'm looking forward that all our unikernels will be more readable. |
|
LGTM - happy to push any fix for breakage later on |
|
I guess irmin and cohttp are both packages I'm not familiar with, so if anyone has time and motivation to update these for the new APIs, that'd be great. Maybe @samoht? Apart from that, I'll update the mirage utility and take a look which mirage-skeleton examples are working / not working. The packages sendmail, dkim, caqti, miragevpn can be deferred to a bit later since mirage-skeleton doesn't use them (neither irmin as far as I can tell). |
|
Dear @hannesm , I've just started to play with this PR, not sure if something could/need to be done, but now I have the delayed execution message even when I try to clean the repository :) |
|
Thanks again for your work on this defunctorisation :) |
palainp
left a comment
There was a problem hiding this comment.
Thank you again for that PR. So far it's very good to me! \o/
I've not much knowledge what the best way would be to avoid this output. We could have some API to discover which Since I barely understand how functoria/mirage works - especially how |
I agree and to me that message is not a blocker for the merge&release :) |
palainp
left a comment
There was a problem hiding this comment.
Sorry, I forgot them when I increased the upper limit of ocaml-solo5.
|
\o/ CI is happy with Ocaml 4.14 and Ocaml 5.2 with unix target, but unhappy with 5.2 with solo5 targets as solo5 restricts to 5.2.1 due to compiler patches (in the meantime I've successfully compiled mirage-skeleton with the current Ocaml 5.3 and the ocaml-solo5 PR). |
|
Thanks @palainp. I asked at ocurrent/mirage-ci#50 how we can achieve such a runner, and indeed soon [tm] we should be able to get 5.3 up and running. |
|
I condensed this into 3 commits:
|
|
before merge:
later:
|
|
Enough CI, let's merge and release :) |
CHANGES: - Remove time, pclock, mclock, random from functor arguments Instead, use the linking trick in mirage-mtime, mirage-ptime, mirage-sleep libraries (mirage/mirage#1599 @hannesm, discussion in mirage/mirage#1513 and on the mailing list) This removes the default_time, default_monotonic_clock, default_posix_clock bindings, and allows "Mirage.register" to get ?sleep ?ptime ?mtime ?random passed optionally, following how ?argv is handled. This is a breaking change, and your unikernel likely needs to be updated. Have a look at mirage/mirage-skeleton#407 for how our examples changed. As example, the hello-key unikernel diff: ``` --- a/tutorial/hello-key/config.ml +++ b/tutorial/hello-key/config.ml @@ -2,5 +2,5 @@ open Mirage let packages = [ package "duration" ] -let main = main ~packages "Unikernel.Hello" (time @-> job) -let () = register "hello-key" [ main $ default_time ] +let main = main ~packages "Unikernel" job +let () = register "hello-key" [ main ] --- a/tutorial/hello-key/unikernel.ml +++ b/tutorial/hello-key/unikernel.ml @@ -5,13 +5,12 @@ let hello = let doc = Arg.info ~doc:"How to say hello." [ "hello" ] in Mirage_runtime.register_arg Arg.(value & opt string "Hello World!" doc) -module Hello (Time : Mirage_time.S) = struct - let start _time = +let start () = let rec loop = function | 0 -> Lwt.return_unit | n -> Logs.info (fun f -> f "%s" (hello ())); - Time.sleep_ns (Duration.of_sec 1) >>= fun () -> loop (n - 1) + Mirage_sleep.ns (Duration.of_sec 1) >>= fun () -> + loop (n - 1) in loop 4 -end ``` - Inject a unit argument to the start function if otherwise it would have no binding (discussed various times, including mirage/mirage#1088 mirage/mirage#873)
to be used with mirage/mirage-skeleton#407
This is the final attempt. Last call.
To play around with it, we have an entire mirage-skeleton working \o/ -- mirage/mirage-skeleton#407 is the diff
Since there's a bunch of stuff that needs changes, I prepared https://github.com/hannesm/mirage-time-defunctorised with all the overlays.
Since there are various overlays, I invite everyone to review -- maybe first the mirage-skeleton changes. Take a deep breath.
And then you can take look around in here. Am happy to see reviews, am also happy to hear "please go ahead", in which case I'll open further PRs, and cut some releases so we can jump over the breaking change while it is hot (and the code hasn't suffered from bitrot yet).
FWIW, tcpip and mirage-logs (and ca-certs-nss) have test cases where the monotonic time or posix time needs to be set to specific values -- take a look into the
mocksubdirectories of mirage-mtime/mirage-ptime repositories. So, indeed it is possible.FWIW, the mirage-logs implementation no longer needs lwt. :D
The only minor thing I'll adjust is the mirage-kv-mem / crunch and their
last_modifiedhandling -- where the semantics is different now -- but I don't think this is an issue.