[RFC] No opam , version constraints for packages of unikernels by hannesm · Pull Request #82 · mirage/functoria

hannesm · 2016-11-19T03:37:57Z

I played around a bit... so instead of having ~packages:string list ~libraries:string list, why not have a Package.t type and use that (package : ?ocamlfind:[ `Prefix of string list | `Full of string list ] -> ?min:string -> ?max:string -> string -> t)

turns out, it is possible... but there are layering violations in mirage (expecting that during configure phase packages are already installed). the goal here is to just generate an opam file from mirage configure, and let someone else install dependencies before invoking make

see mirage/mirage#691

feedback welcome @samoht @Drup @yomimono @avsm

hannesm · 2016-11-19T04:26:15Z

opam output for mirage-console:

# mirage configure -t virtio
opam-version: "1.2"
name: console
tags: ["org:mirage" ; "org:unikernel"]
build: [ make "build" ]
depends: [
  "ocamlbuild" {build}
  "ocamlfind" {build}
  "solo5-kernel-virtio" 
  "mirage-types-lwt" 
  "mirage-types" {<"4.0.0" & >="3.0.0"}
  "mirage-solo5" 
  "mirage-runtime" {<"4.0.0" & >="3.0.0"}
  "mirage-logs" 
  "mirage-console-solo5" 
  "mirage-clock-freestanding" 
  "mirage-bootvar-solo5" 
  "lwt" 
  "functoria-runtime" 
  "duration" 
]

avsm · 2016-11-19T10:59:09Z

Woah, awesome!

samoht · 2016-11-19T11:38:54Z

That's awesome, thanks for having a look at this. I am not totally convinced by the Full / Suffix stuff but why not. Also, where do you plan to generate the opam file? At the root? In a subdir? This has a direct impact on multi-config support.

hannesm · 2016-11-19T11:45:28Z

glad that you point out the weak bits:

Full vs Suffix is a hack - I needed to find out what is used out there (what are the common patterns for ocamlfind package names, apart from when opam name and findlib name are equal. atm I'd propose two keyword arguments, ?sub:string -> ?ocamlfind:string list which are mutually exclusive to cope with the current usage
the opam file name of the unikernel should include the relevant configuration arguments (such as target, but also direct vs socket stack etc... console-virtio.opam, static_website_tls-socket-unix.opam), open for suggestions -- it could also be a hash over the config choices, console-virtio-xxxaaaannn

hannesm · 2016-11-19T15:55:20Z

this has been updated, and I think it is more or less good now... I don't know much about the pp in Functoria_app (and what it should print now, see comment), and also don't have a clue whether we need better errors (and in which way) instead of invalid_arg if the version number is not a list of numbers etc...

I addressed @samoht concerns: (changed to package : ?sub:string -> ?ocamlfind:string list -> ?min:string -> ?max:string -> string -> package); and providing a ?name:string for the opam printer -- thus it should be the concern of the caller ;)

feedback welcome!

samoht · 2016-11-19T15:59:16Z

I really like generating random names for the opam files (when it is not provided by the user). I find it quite nice in docker. And I really want to have myspitting_lama unikernel :p

hannesm · 2016-11-19T16:14:34Z

instead of random, why not deterministic (hash over all the provided configuration keys?)

hannesm · 2016-11-19T16:28:06Z

I updated the mirage bits mirage/mirage#691, and mirage/mirage-skeleton#198

will not compile (unless using all these branches and rewriting the CI scripts slightly)

hannesm · 2016-11-21T00:30:14Z

is better now, still questions are open: what should Functoria_app.pp_dump_info do? how should error handling be done (throwing from Functoria.package (and merge, etc.) seems a bit rough)?

hannesm · 2016-11-21T12:56:43Z

@Drup I'm interested in your opinion here (and maybe you have answers to the 2 questions I raised in my previous comment).

Drup

I like that work a lot, well done. I reviewed the changes and made various comments.

I'm not extremely convinced by the usage of a String.Map for packages, but the implementation make sense, you probably want to add a quick comment in the ml file as to why it's done that way (to make merging of packages easy).

As for your question about invalid_arg ... I think it's ok. It's really the right thing to raise here, and no need to sugar coat it with fancy cmdliner error handling.

Drup · 2016-11-21T04:50:19Z

lib/functoria.mli

+    additional sublibraries (e.g. [~sublibs:["mirage"] "foo"] will result in the
+    findlib names [ ["foo"; "foo.mirage"] ].  In case the findlib name is
+    disjoint (or empty), use [~ocamlfind].  Version constraints are given as
+    [min] (inclusive) and [max] (exclusive). *)


Please specify that using ocamlfind with sublib is an error.

Drup · 2016-11-21T04:50:48Z

lib/functoria.mli

@@ -120,8 +150,6 @@ val foreign:
    {ul
    {- If [packages] is set, then the given OPAM packages are


"given packages", not only OPAM packages anymore.

Drup · 2016-11-21T04:54:35Z

app/functoria_command_line.mli

-  no_opam: bool;
-  no_depext: bool;
-  no_opam_version: bool
+  result: 'a


Are we removing this record ? Keeping it make adding some argument back convenient, but ...

Drup · 2016-11-21T08:17:12Z

lib/functoria.ml

+    let name = match name with None -> t.name | Some x -> x in
+    Fmt.pf ppf "opam-version: \"1.2\"@." ;
+    Fmt.pf ppf "name: \"%s\"@." name ;
+    Fmt.pf ppf "depends: [ @[<2>%a@]@ ]@."


Maybe use a vertical box here ?

Drup · 2016-11-21T14:36:15Z

lib/functoria.ml

+        | None -> String.Map.add n p m
+        | Some p' -> match Package.merge p.opam p p' with
+          | Some p -> String.Map.add n p m
+          | None -> invalid_arg "bad constraints")


We should provide a better error message (and in particular, which opam package triggers the conflict).

Drup · 2016-11-21T14:52:02Z

lib/functoria.ml

+  let package_names t =
+    List.map fst
+      (List.filter (fun (_, p) -> p.build = false)
+         (String.Map.bindings t.packages))


I feel like you should move that stuff to the Package module, and keep only Info.package.

no. The Info record keeps track of a compilation unit, which contain a map of packages. The package module does only care about a single package.

Meh, Package is the module that deals with packages. You want something from package list to the list of ocamlfind libraries (resp. opam packages), I feel like it would belong there.

Drup · 2016-11-21T14:56:31Z

app/functoria_app.ml

     @[<v 2>packages = %a@]@ ;@ @[<v 2>libraries = %a@]@ }"
    module_name (Info.name i)
-    pp_packages (Info.packages i)
+    pp_packages (Info.package_names i)


pp_dump_info is used to expose the content of Info to the underlying unikernel, in order to do things like an unikernel that lists the packages and the version it uses. It dumps an ml files with placeholder like %{functoria:version}% and then call opam config subst to substitute them.

That probably won't really work well anymore, so we need to move opam config subst to the "make depend" target.
Since you have a bit more information now, we could enrich the datastructure in functoria-runtime with packages and dump that.

- 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

hannesm · 2016-11-21T17:08:37Z

this is now pretty-printing a bit prettier (comment on the pr with magic open boxes etc. if you'd like to improve):

$ less mirage-unikernel-console-virtio.opam
# Generated by console (Mon, 21 Nov 2016 17:03:06 GMT).
opam-version: "1.2"
name: "mirage-unikernel-console-virtio"
depends: [ "duration" 
           "functoria-runtime" 
           "lwt" 
           "mirage-bootvar-solo5" 
           "mirage-clock-freestanding" 
           "mirage-console-solo5" 
           "mirage-logs" 
           "mirage-runtime" {>="3.0.0"}
           "mirage-solo5" 
           "mirage-types" {>="3.0.0"}
           "mirage-types-lwt" 
           "ocamlbuild" {build}
           "ocamlfind" {build}
           "solo5-kernel-virtio" 
]

$ less mirage-unikernel-console-xen.opam 
# Generated by console (Mon, 21 Nov 2016 17:06:27 GMT).
opam-version: "1.2"
name: "mirage-unikernel-console-xen"
depends: [ "duration" 
           "functoria-runtime" 
           "lwt" 
           "mirage-bootvar-xen" 
           "mirage-clock-freestanding" 
           "mirage-console-xen" 
           "mirage-logs" 
           "mirage-runtime" {>="3.0.0"}
           "mirage-types" {>="3.0.0"}
           "mirage-types-lwt" 
           "mirage-xen" 
           "ocamlbuild" {build}
           "ocamlfind" {build}
]

$ less mirage-unikernel-console-unix.opam 
# Generated by console (Mon, 21 Nov 2016 17:06:50 GMT).
opam-version: "1.2"
name: "mirage-unikernel-console-unix"
depends: [ "duration" 
           "functoria-runtime" 
           "lwt" 
           "mirage-clock-unix" 
           "mirage-console-unix" 
           "mirage-logs" 
           "mirage-runtime" {>="3.0.0"}
           "mirage-types" {>="3.0.0"}
           "mirage-types-lwt" 
           "mirage-unix" 
           "ocamlbuild" {build}
           "ocamlfind" {build}
]

thing left is the info stuff drup explained above... this will need more thinking and integration into the build phase (but i guess it is ok to have it partially broken for some time)...

Drup · 2016-11-22T03:53:03Z

I'm pretty happy with the last batch of changes. We just need to fix pp_dump_info/app_info.

hannesm · 2016-11-26T02:45:16Z

superseeded by #84

hannesm mentioned this pull request Nov 19, 2016

[RFC] No opam, constrain package versions in unikernels mirage/mirage#691

Closed

yomimono mentioned this pull request Nov 19, 2016

pending items for consideration for Mirage 3.0 mirage/mirage#571

Closed

9 tasks

hannesm force-pushed the no-opam branch from 3618c54 to b11229a Compare November 19, 2016 15:50

hannesm changed the title ~~[WIP] No opam , version constraints for packages of unikernels~~ [RFC] No opam , version constraints for packages of unikernels Nov 19, 2016

hannesm force-pushed the no-opam branch from e41085c to 07d29e9 Compare November 20, 2016 17:59

hannesm added 2 commits November 20, 2016 20:08

remove opam handling from functoria

2a4f6f8

remove no longer needed OCamlfind command execution

fd2817e

hannesm force-pushed the no-opam branch 2 times, most recently from eaa7a74 to 07d974a Compare November 20, 2016 23:36

hannesm mentioned this pull request Nov 21, 2016

Add package version constraints mirage/mirage#392

Closed

Drup reviewed Nov 21, 2016

View reviewed changes

hannesm force-pushed the no-opam branch from 3b2e443 to 0406036 Compare November 21, 2016 16:53

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

hannesm force-pushed the no-opam branch from 0406036 to c126c7f Compare November 21, 2016 17:04

hannesm mentioned this pull request Nov 25, 2016

New order #84

Merged

hannesm closed this Nov 26, 2016

hannesm deleted the no-opam branch December 10, 2016 15:07

		@@ -120,8 +150,6 @@ val foreign:
		{ul
		{- If [packages] is set, then the given OPAM packages are

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

avsm commented Nov 19, 2016

Uh oh!

samoht commented Nov 19, 2016

Uh oh!

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

Uh oh!

Uh oh!

hannesm commented Nov 19, 2016

Uh oh!

samoht commented Nov 19, 2016

Uh oh!

hannesm commented Nov 19, 2016

Uh oh!

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

Uh oh!

Uh oh!

hannesm commented Nov 21, 2016

Uh oh!

hannesm commented Nov 21, 2016

Uh oh!

Drup left a comment

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 Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Drup commented Nov 22, 2016

Uh oh!

hannesm commented Nov 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hannesm commented Nov 19, 2016 •

edited

Loading

hannesm commented Nov 19, 2016 •

edited

Loading

hannesm commented Nov 19, 2016 •

edited

Loading

hannesm commented Nov 19, 2016 •

edited

Loading

Drup Nov 21, 2016 •

edited

Loading

hannesm commented Nov 21, 2016 •

edited

Loading