Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Behavior of jobs without dependencies #1426

Closed
reynir opened this issue Jun 8, 2023 · 5 comments
Closed

Behavior of jobs without dependencies #1426

reynir opened this issue Jun 8, 2023 · 5 comments

Comments

@reynir
Copy link
Member

reynir commented Jun 8, 2023

In the following code we have a unikernel with a job that does not depend on any devices, but logs a message.

config.ml:

open Mirage

let hello = main "Unikernel.Hello" job
let () = register "hello-nodev" [ hello ]

unikernel.ml:

module Hello = struct
  (* this is imediately evaluated *)
  let start =
    Logs.info (fun f -> f "hello");
    Lwt.return_unit
end

This successfully compiles and runs, but on the unix target this does not print anything to the console. My suspicion is the log reporter is not set up before Unikernel.Hello.start is evaluated and therefore nothing is printed.

Then my idea was to suspend the computation with a dummy/null/unit device. It was not very clear, and I had to consult the source code, but I found that I could use Mirage.noop in config.ml:

open Mirage

let hello = main "Unikernel.Hello" (job @-> job)
let () = register "hello-nodev" [ hello $ noop ]

And in unikernel.ml:

module Hello (_ : sig end) = struct
  let start () =
    Logs.info (fun f -> f "hello");
    Lwt.return_unit
end 

I guess for the first code we can't do much about the evaluation order(??) to ensure the log reporter is registered first.

My question then is what is a job? The documentation says a "main task" (but we can have a list of them!?). Should we have a unit typ and unit impl? Can we chain jobs like this?

This was discovered with @PizieDust.

@palainp
Copy link
Member

palainp commented Jun 12, 2023

Hi @reynir and @PizieDust for finding this :)

I'm not sure if this is a plausible path to track this issue but looking in caml_program, calling camlDune__exe__Unikernel__entry, it seems that the start function is inline in the first version.

I then tried to un-inline it but, as it's no longer a function, the compiler doesn't let me to do that :/.

I'm now at the point where I've added a single unit to the start function when no args are provided (see main...palainp:mirage:fix-nodev). This is a breaking change as now an unikernel without functorized device will have to take a unit as argument (but I'm not sure this was used before).
@mirage/core, any thoughts on this possible fix?

@hannesm
Copy link
Member

hannesm commented Jun 12, 2023

There's #873 which looks very similar to this issue. I agree with @palainp solution to add a unit argument to start functions without any dependencies.

@reynir
Copy link
Member Author

reynir commented Jun 12, 2023

Thank you for looking into this! When I was going through mirage-skeleton and playing with mirage with @PizieDust my intuition was that zero device jobs would be a special case with a unit argument. I'm not sure if others share that intuition. On the other hand it can seem inconsistent that it's special cased.

@hannesm
Copy link
Member

hannesm commented Jun 12, 2023

Another path is to error out (at mirage configure time) if there aren't any dependencies -- then you have to manually add the noop job as dependency (which imho would be fine, and not special casing anything... the error message could even point to that thing).

hannesm added a commit to hannesm/mirage that referenced this issue Jun 19, 2023
This ensures that toplevel expressions in the unikernels are properly evaluated.

Fixes mirage#873 mirage#1426
hannesm added a commit that referenced this issue Jun 19, 2023
* Fail if there are jobs registered without any arguments.

This ensures that toplevel expressions in the unikernels are properly evaluated.

Fixes #873 #1426

* address @palainp review comments

* simplify 'Impl.app_has_no_arguments', as proposed by @reynir

* fix app_has_no_arguments again (thx @palainp @reynir)
@hannesm
Copy link
Member

hannesm commented Jun 19, 2023

fixed by #1428

@hannesm hannesm closed this as completed Jun 19, 2023
hannesm added a commit to hannesm/opam-repository that referenced this issue Jun 19, 2023
… (4.4.0)

CHANGES:

- Fail configure if jobs without arguments are present
  (fixes mirage/mirage#873 mirage/mirage#1426, mirage/mirage#1428 @hannesm)
- mirage-runtime & functoria-runtime: remove fmt dependency (mirage/mirage#1417 @hannesm)
- Fix tests on macOS (mirage/mirage#1425 @samoht)
- Adapt to happy-eyeballs 0.6.0 release (mirage/mirage#1427 @hannesm)
- Adapt to logs-syslog 0.4.0 release (mirage/mirage#1424 @hannesm)
- Adapt to docteur 0.0.6 release (mirage/mirage#1419 @dinosaure)
- Upgrade tests to cmdliner 1.2.0 (mirage/mirage#1418 @hannesm)
- Fail if jobs without arguments are registered (reported mirage/mirage#873 @kit-ty-kate
  mirage/mirage#1426 @reynir @PizieDust, fixed mirage/mirage#1428 @hannesm)
- Console is marked as deprecated (mirage/mirage#1429 @hannesm)
- Tracing has been removed, since it was not used anymore and not supported with
  solo5-xen-pvh (mirage/mirage#1430 @hannesm)
hannesm added a commit to hannesm/opam-repository that referenced this issue Jun 19, 2023
… (4.4.0)

CHANGES:

- Fail configure if jobs without arguments are present
  (fixes mirage/mirage#873 mirage/mirage#1426, mirage/mirage#1428 @hannesm)
- mirage-runtime & functoria-runtime: remove fmt dependency (mirage/mirage#1417 @hannesm)
- Fix tests on macOS (mirage/mirage#1425 @samoht)
- Adapt to happy-eyeballs 0.6.0 release (mirage/mirage#1427 @hannesm)
- Adapt to logs-syslog 0.4.0 release (mirage/mirage#1424 @hannesm)
- Adapt to docteur 0.0.6 release (mirage/mirage#1419 @dinosaure)
- Upgrade tests to cmdliner 1.2.0 (mirage/mirage#1418 @hannesm)
- Fail if jobs without arguments are registered (reported mirage/mirage#873 @kit-ty-kate
  mirage/mirage#1426 @reynir @PizieDust, fixed mirage/mirage#1428 @hannesm)
- Console is marked as deprecated (mirage/mirage#1429 @hannesm)
- Tracing has been removed, since it was not used anymore and not supported with
  solo5-xen-pvh (mirage/mirage#1430 @hannesm)
hannesm added a commit to hannesm/opam-repository that referenced this issue Jun 20, 2023
… (4.4.0)

CHANGES:

- Fail configure if jobs without arguments are present
  (fixes mirage/mirage#873 mirage/mirage#1426, mirage/mirage#1428 @hannesm)
- mirage-runtime & functoria-runtime: remove fmt dependency (mirage/mirage#1417 @hannesm)
- Fix tests on macOS (mirage/mirage#1425 @samoht)
- Adapt to happy-eyeballs 0.6.0 release (mirage/mirage#1427 @hannesm)
- Adapt to logs-syslog 0.4.0 release (mirage/mirage#1424 @hannesm)
- Adapt to docteur 0.0.6 release (mirage/mirage#1419 @dinosaure)
- Upgrade tests to cmdliner 1.2.0 (mirage/mirage#1418 @hannesm)
- Fail if jobs without arguments are registered (reported mirage/mirage#873 @kit-ty-kate
  mirage/mirage#1426 @reynir @PizieDust, fixed mirage/mirage#1428 @hannesm and mirage/mirage#1431 @reynir)
- Console is marked as deprecated (mirage/mirage#1429 @hannesm)
- Tracing has been removed, since it was not used anymore and not supported with
  solo5-xen-pvh (mirage/mirage#1430 @hannesm)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants