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

New order #84

Merged
merged 15 commits into from Nov 29, 2016

Conversation

Projects
None yet
5 participants
@hannesm
Member

hannesm commented Nov 25, 2016

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.

hannesm added some commits Nov 18, 2016

Introduce `package` type
- 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)
address suggestions from @Drup:
- 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
remove cmd and logs implementation
switch to Logs and Bos (and Fpath) instead
(fun () -> Engine.build i jobs) ()
with
| Ok a -> a
| Error _ -> R.error_msg "failed to change directory for build"

This comment has been minimized.

@Drup

Drup Nov 26, 2016

Member

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

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")) ()

This comment has been minimized.

@Drup

Drup Nov 26, 2016

Member

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

This comment has been minimized.

@hannesm

hannesm Nov 26, 2016

Member

was the same before

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.

This comment has been minimized.

@Drup

Drup Nov 26, 2016

Member

Ok, fair enough.

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)

This comment has been minimized.

@Drup

Drup Nov 26, 2016

Member

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.

This comment has been minimized.

@hannesm

hannesm Nov 26, 2016

Member

you mean pp > tmp followed by dotcmd tmp?

This comment has been minimized.

@Drup

Drup Nov 26, 2016

Member

Yes.

(** [clean info] is the code to clean-up what has been generated
by {!configure}. *)
by {!build}. *)

This comment has been minimized.

@Drup

Drup Nov 26, 2016

Member

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

@@ -293,7 +83,7 @@ module Codegen = struct
days.(time.Unix.tm_wday) time.Unix.tm_mday
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

This comment has been minimized.

@Drup

Drup Nov 26, 2016

Member

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. :)

This comment has been minimized.

@samoht

samoht Nov 26, 2016

Member

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

This comment has been minimized.

@hannesm

hannesm Nov 26, 2016

Member

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

This comment has been minimized.

Member

Drup commented Nov 26, 2016

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.

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. *)

This comment has been minimized.

@Drup

Drup Nov 26, 2016

Member

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

This comment has been minimized.

Member

hannesm commented Nov 26, 2016

@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.
@samoht

This comment has been minimized.

Member

samoht commented Nov 26, 2016

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

This comment has been minimized.

Member

hannesm commented Nov 26, 2016

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

@hannesm

This comment has been minimized.

Member

hannesm commented Nov 26, 2016

@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.

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

This comment has been minimized.

@Drup

Drup Nov 26, 2016

Member

s/available/installed/

Maybe mention external tools explicitly ?

@Drup

This comment has been minimized.

Member

Drup commented Nov 26, 2016

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

@Drup

This comment has been minimized.

Member

Drup commented Nov 26, 2016

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

This comment has been minimized.

Member

hannesm commented Nov 26, 2016

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

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 comment has been minimized.

@dbuenzli

dbuenzli Nov 28, 2016

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

@mato

This comment has been minimized.

Member

mato commented Nov 28, 2016

(deleted, wrong PR and typo, sorry)

@mato

This comment has been minimized.

Member

mato commented Nov 28, 2016

@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

This comment has been minimized.

Member

hannesm commented Nov 28, 2016

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

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@hannesm hannesm deleted the hannesm:new-order branch Nov 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment