Mirage time defunctorised

[hannesm]

The context is #1513, and #1521. This is a first shot at the mirage-time package, to use dune variants. It builds on top of #1526 and is best read with mirage/mirage-skeleton#394.

I wrote some more text to our mailing list: https://lists.xenproject.org/archives/html/mirageos-devel/2024-05/msg00002.html

This is marked as draft, since I wouldn't want to merge this as-is, but would appreciate feedback first -- maybe the code in mirage can be further simplified: an impl that doesn't have a connect should maybe not exist in main.ml -- i.e. no need to generate anything, since there's nothing to initialize -- we can just use it in unikernel.ml -- but my functoria/mirage knowledge is not sufficient to write up such a thing. The default value for connect is eventually return () -- while I'd like to have connect being an option (but as mentioned, I'm not sure what else needs to change and what will break).

Another remark is about the "no argument check" -- I think this brief hack introduced by myself is not the right thing. We should ensure that main (and other jobs) have at least one argument -- but if we pass in one argument (of ()) for each "device" that doesn't need it, we'll add a lot of things for no good reason (and have again complexity, and it being hard to understand).

[samoht]

Why is this removal needed? Did the reason why this was added changed with that PR?

[hannesm]

From my comment above:

Another remark is about the "no argument check" -- I think this brief hack introduced by myself is not the right thing. We should ensure that main (and other jobs) have at least one argument -- but if we pass in one argument (of ()) for each "device" that doesn't need it, we'll add a lot of things for no good reason (and have again complexity, and it being hard to understand).

--> I think this "app_has_no_argument" check was a good first iteration, but what we actually want to check is that "start takes at least an argument" (to have it executed after a log reporter) -- but already with extra_deps and runtime_args we get arguments. So, indeed this removal is unrelated and should not be merged as is. It would be great to have a better check (esp. since e.g. noop is already excluded from the check).

[samoht]

I like the direction of that one too. A few comments:

• I would move the sleeper machinery in a Mirage_time.Runtime module (or something similar, but in the mirage-time package) so this is independent of mirage (if possible)
• As we need to pass the extra_dep parameter around, I still think it'd be helpful to make this type abstract and require Mirage_time.sleep_ns to take that parameter. Call it a capability or something else, but I think there is still value in having a way to know when a function needs access to the time device (and if it does, this is reflected in its type)
• What about the error case: what happens if someone uses Mirage_time.sleep_ns but forgets to add the extra_deps parameter? I guess it'll try to link with mirage-time.unix, which will fail on the solo5 target. Do we have a good way to deal with the error? Maybe passing the Mirage_time.t parameter around is a good way to help with this? Or something else?
• Related to mirage-time specifically (but also quite minor): it would be great to see if we could use variants to mock the time implementation and write unit tests for the package.

[hannesm]

Thanks for your feedback.

I will do (a) move the sleeper space to mirage-time.

With (b) - passing a value of an abstract type around, I'm undecided: what is the value thereof? What is a real use case (or requirement)? At least since mirage-time 1.0 (2016), the interface is val sleep_ns : int64 -> unit Lwt.t... In mirage-clock, since 2.0 (2018) the API is val now_d_ps : unit -> int * int64 (and val elapsed_ns : unit -> int64).

For other "devices" (such as a network stack, a network interface, block device, file system, ...) that have data attached to it, I for sure see the value. For time/clock/random, I don't.

(c) indeed, the error is a bit mystic about "unix library hidden".

(d) sure, we can have in mirage-time a ".mock" implementation, but I'm not sure what should it do? directly return? return earlier? If you've a suggestion or an actual test library, please let me know.

[hannesm]

When putting the sleeper storage into mirage-time, I ran into the dune issue again. That's fantastic since now there's a pretty small reproduction case https://github.com/hannesm/dune-variant (reported ocaml/dune#10460). Let's hope the dune developers will have some time and be able to fix the root cause of the issue.