New order by hannesm · Pull Request #84 · mirage/functoria

hannesm · 2016-11-25T18:02:17Z

This is on top of #82, but also removes Cmd and Log, replacing it with Bos and Logs. ~~~For some unknown reason mirage clean does not work as expected here. I might look into that later... otherwise, comments/feedback welcome (I'm sure I missed something)~~~

It also re-introduces the build command (which calls out to bla#build in the graph ('coz @samoht is always right ;) (reverts #76)

Fixes #73 #74 #63 (removing Cmd) #54 (superseeds #82) #81

ready to go, commits can be squashed imho.

- contains opam name, ocamlfind names, version constraints, flag whether build-only dependency - users provide lists of packages - is combined into a single map (including merging of version numbers if possible) Also provide, based on this, to print an opam file (well, the good parts - name and dependencies)

- get rid of type 'a result - add vertical box in depends output - move logic of libraries and package_names to Package module - improve error messages

switch to Logs and Bos (and Fpath) instead

Drup · 2016-11-26T21:35:03Z

app/functoria_app.ml

+        (fun () -> Engine.build i jobs) ()
+    with
+    | Ok a -> a
+    | Error _ -> R.error_msg "failed to change directory for build"


Could you move the logic for directory changing in an auxiliary function ?

Drup · 2016-11-26T21:36:17Z

app/functoria_app.ml

+           Bos.OS.Dir.delete ~recurse:true (Fpath.v "_build") >>= fun () ->
+           Bos.OS.File.delete (Fpath.v "log") >>= fun () ->
+           Bos.OS.File.delete (Fpath.v "main.native.o") >>= fun () ->
+           Bos.OS.File.delete (Fpath.v "main.native")) ()


Isn't main.native associated to mirage, more than functoria ? Nothing really specify what's the main target in functoria.

was the same before

functoria/app/functoria_app.ml

Line 488 in d7a2015

Cmd.run "rm -rf log %s/main.native.o %s/main.native %s/*~" root

since this PR is already pretty big, I'd leave the main.native question to a subsequent PR.

Ok, fair enough.

Drup · 2016-11-26T21:39:21Z

app/functoria_app.ml

-    in R.ok @@ with_fmt f
+        f Format.str_formatter ;
+        let data = Format.flush_str_formatter () in
+        Bos.OS.Cmd.run_in (Bos.Cmd.v dotcmd) (Bos.OS.Cmd.in_string data)


While you're at it (and you seem fluent in Bos), could you pipe that to a temp file and call dotcmd on the tempfile, instead ? See #81.

you mean pp > tmp followed by dotcmd tmp?

Drup · 2016-11-26T21:45:21Z

lib/functoria.mli

+  method clean: Info.t -> (unit, Rresult.R.msg) Rresult.result
  (** [clean info] is the code to clean-up what has been generated
-      by {!configure}. *)
+      by {!build}. *)


Not both ? How do we clean up things generated by configure then ?

Drup · 2016-11-26T21:46:53Z

lib/functoria_misc.ml

        months.(time.Unix.tm_mon) (time.Unix.tm_year+1900)
        time.Unix.tm_hour time.Unix.tm_min time.Unix.tm_sec in
-    Format.sprintf "Generated by %s (%s)." name date
+    Format.sprintf "Generated by functoria (%s)." date


I was asked specifically not to put functoria here. The actual application (aka mirage, or the unikernel) should put its name here. Not sure how @samoht 's position has evolved on the matter. :)

I would still prefer to see mirage.VERSION here instead of functoria :-)

I now use Sys.argv, this preserves the entire command line. Not sure how functoria would be able to access the mirage version string.

Drup · 2016-11-26T21:54:05Z

Not sure why you made a new PR instead on continuing on the old one. It would have been easier to review.

Did you made sure that the log level option appears in the help ? It's not so clear to me by reading the code.

Looks good otherwise.

Drup · 2016-11-26T21:59:09Z

lib/functoria.mli

+  method build: Info.t -> (unit, Rresult.R.msg) Rresult.result
+  (** [build info] is the code to execute in order to build
+      the device. This might involve generating OCaml code,
+      running shell scripts, etc. *)


There is a clear overlap between the function of configure and build, according to the documentation. It should be much clearer what should go where and when things are executed.

hannesm · 2016-11-26T21:59:39Z

@Drup: thanks for your review. you could read only the very last commit, which is the diff on top of the previous PR. it was not necessary here, but in the mirage repository a new PR made sense (since it partially reverts previous changes, such as the Makefile pkg-config thingies)

mirage configure --help includes:

       --color=WHEN (absent=auto)
           Colorize the output. WHEN must be one of `auto', `always' or
           `never'.

       -q, --quiet
           Be quiet. Takes over -v and --verbosity.

       -v, --verbose
           Increase verbosity. Repeatable, but more than twice does not bring
           more.

       --verbosity=LEVEL (absent=warning)
           Be more or less verbose. LEVEL must be one of `quiet', `error',
           `warning', `info' or `debug'. Takes over -v.

…ction

samoht · 2016-11-26T22:16:14Z

I really like the new generation of opam files, and the cleanup with Bos + Logs is very welcome. I also like the new build command but It would be very helpful if you could comment on the new expected build flow. Do you expect the mirage driver in Mirage3 to be able to build one/all of the configured unikernels in one commend? Or something else?

hannesm · 2016-11-26T22:26:22Z

@Drup I (hopefully) fixed all your comments :) (even the main.native)

hannesm · 2016-11-26T22:33:59Z

@samoht I don't understand your question. I do specifically (atm) not work on the ability to have various configurations (xen/unix/virtio) available at the same time.

The workflow, as mentioned on mirageos-devel, will now be:
mirage configure -t <backend>; make depend; mirage build -t <backend>.

The make depend calls out to opam, and is only necessary if you don't have all dependencies installed already.

To smooth the transition, a Makefile is generated which build target contains mirage build -t <backend> (but you don't have to have make to compile (apart from the ukvm target where ukvm-configure generates a Makefile.ukvm)).

Thus, for MirageOS unikernel developers: mirage configure ; make should be sufficient (with subsequent calls to make while changing the Caml code).

For CI systems, some instance can foreach backend ; mirage configure -t $backend, and then another set of CIs could opam pin mirage-unikernel-<name>-<backend> and mirage build -t <backend>. There is no longer a convolution between configure, opam dependency installations, and build. For incremental rebuilds (reverse dependencies), unless config.ml changes, you can use the generated opam files as dependency sources. Maybe the generated opam file should contain a build: [ mirage build -t <backend> ] to have it more streamlined.

This change, in contrast to the earlier proposal, requires that the mirage command is available at build time as well. I considered other solutions, such as generating a shell script, generating an OCaml build script, but in the end I don't want to generate any code. This way, re-using the mirage tool, ensures that it is done in a type safe manner.

Drup · 2016-11-26T22:44:27Z

lib/functoria.mli

-      the device. This might involve generating OCaml code,
-      running shell scripts, etc. *)
+      the device.  During the build phase, you can rely that all
+      {!packages} are available.  This might involve generating


s/available/installed/

Maybe mention external tools explicitly ?

Drup · 2016-11-26T22:45:47Z

@hannesm What happens if I call mirage build before doing mirage configure apart from an unclear error ?

Drup · 2016-11-26T22:48:56Z

Maybe the generated opam file should contain a build: [ mirage build -t ] to have it more streamlined.

Yes, that sounds like a good idea.

hannesm · 2016-11-26T23:37:54Z

@Drup the opam generated by mirage now emits a build rule.

dbuenzli · 2016-11-28T08:11:05Z

app/functoria_app.ml

+        let data = Format.flush_str_formatter () in
+        Bos.OS.File.tmp ~mode:0o644 "graph%s.dot" >>= fun tmp ->
+        Bos.OS.File.write tmp data >>= fun () ->
+        Bos.OS.Cmd.run Bos.Cmd.(v dotcmd % (Fpath.to_string tmp))


This can be written OS.Cmd.run Cmd.(v dotcmd % p tmp)

mato · 2016-11-28T15:58:07Z

(deleted, wrong PR and typo, sorry)

mato · 2016-11-28T16:07:47Z

@hannesm mirage build ... seems to accept all the configure-time options that mirage configure ... would for a unikernel (e.g. --dhcp ...) That doesn't seem right?

hannesm · 2016-11-28T16:30:20Z

On 28/11/2016 16:07, Martin Lucina wrote: @hannesm `mirage build ...` seems to accept all the configure-time options that `mirage configure ...` would for a unikernel (e.g. `--dhcp` ...) That doesn't seem right?

Interesting. the same is true for `mirage help clean`. not sure how to avoid this, but #72 looks very related to this.

hannesm added 4 commits November 20, 2016 20:08

remove opam handling from functoria

2a4f6f8

remove no longer needed OCamlfind command execution

fd2817e

address suggestions from @Drup:

c126c7f

- get rid of type 'a result - add vertical box in depends output - move logic of libraries and package_names to Package module - improve error messages

This was referenced Nov 25, 2016

New order mirage/mirage#703

Merged

[RFC] No opam , version constraints for packages of unikernels #82

Closed

hannesm force-pushed the new-order branch from d1afc22 to 7023d9e Compare November 26, 2016 19:07

remove cmd and logs implementation

74a3b5a

switch to Logs and Bos (and Fpath) instead

hannesm force-pushed the new-order branch from 7023d9e to 74a3b5a Compare November 26, 2016 20:00

Drup reviewed Nov 26, 2016

View reviewed changes

hannesm added 2 commits November 26, 2016 22:14

move logic for output channel + current directory into a separate fun…

9130d64

…ction

no main.native.[o] here, moved to mirage

1183424

hannesm added 3 commits November 26, 2016 22:18

use a temporary file, fixes mirage#81

e301d93

embed full command line in generated header

362e26c

clarify build vs configure

c1ff4b4

Drup reviewed Nov 26, 2016

View reviewed changes

clarify

877924c

have -v/-quiet etc in help as well

c0a649d

hannesm force-pushed the new-order branch from 0ef60ee to c0a649d Compare November 26, 2016 23:53

dbuenzli reviewed Nov 28, 2016

View reviewed changes

minor nit

770a5a4

- only delete config*, not all of build

093a06e

hannesm force-pushed the new-order branch from 9194def to 093a06e Compare November 29, 2016 01:00

exclude _build-ukvm for ocamlbuild

c2eb1db

mato merged commit 54e33b4 into mirage:master Nov 29, 2016

This was referenced Nov 29, 2016

fight bitrot in Functoria_misc #74

Closed

Fix quoting of opam arguments #54

Closed

hannesm deleted the new-order branch November 29, 2016 22:18

Conversation

hannesm commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Drup commented Nov 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannesm commented Nov 26, 2016

Uh oh!

samoht commented Nov 26, 2016

Uh oh!

hannesm commented Nov 26, 2016

Uh oh!

hannesm commented Nov 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Drup commented Nov 26, 2016

Uh oh!

Drup commented Nov 26, 2016

Uh oh!

hannesm commented Nov 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mato commented Nov 28, 2016

Uh oh!

mato commented Nov 28, 2016

Uh oh!

hannesm commented Nov 28, 2016 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hannesm commented Nov 25, 2016 •

edited

Loading

hannesm commented Nov 26, 2016 •

edited

Loading