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

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

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@hannesm
Member

hannesm commented Nov 19, 2016

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

This comment has been minimized.

Member

hannesm commented Nov 19, 2016

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

This comment has been minimized.

Member

avsm commented Nov 19, 2016

Woah, awesome!

@yomimono yomimono referenced this pull request Nov 19, 2016

Closed

pending items for consideration for Mirage 3.0 #571

9 of 9 tasks complete
@samoht

This comment has been minimized.

Member

samoht commented Nov 19, 2016

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

This comment has been minimized.

Member

hannesm commented Nov 19, 2016

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

This comment has been minimized.

Member

hannesm commented Nov 19, 2016

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

This comment has been minimized.

Member

samoht commented Nov 19, 2016

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

This comment has been minimized.

Member

hannesm commented Nov 19, 2016

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

@hannesm

This comment has been minimized.

Member

hannesm commented Nov 19, 2016

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 hannesm changed the title from [WIP] No opam , version constraints for packages of unikernels to [RFC] No opam , version constraints for packages of unikernels Nov 19, 2016

@hannesm

This comment has been minimized.

Member

hannesm commented Nov 21, 2016

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

This comment has been minimized.

Member

hannesm commented Nov 21, 2016

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

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

This comment has been minimized.

@Drup

Drup Nov 21, 2016

Member

Please specify that using ocamlfind with sublib is an error.

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

This comment has been minimized.

@Drup

Drup Nov 21, 2016

Member

"given packages", not only OPAM packages anymore.

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

This comment has been minimized.

@Drup

Drup Nov 21, 2016

Member

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

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

This comment has been minimized.

@Drup

Drup Nov 21, 2016

Member

Maybe use a vertical box here ?

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

This comment has been minimized.

@Drup

Drup Nov 21, 2016

Member

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

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

This comment has been minimized.

@Drup

Drup Nov 21, 2016

Member

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

This comment has been minimized.

@hannesm

hannesm Nov 21, 2016

Member

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.

This comment has been minimized.

@Drup

Drup Nov 21, 2016

Member

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.

@@ -113,7 +114,7 @@ let pp_dump_info module_name fmt i =
"%s.{@ name = %S;@ \
@[<v 2>packages = %a@]@ ;@ @[<v 2>libraries = %a@]@ }"
module_name (Info.name i)
pp_packages (Info.packages i)
pp_packages (Info.package_names i)

This comment has been minimized.

@Drup

Drup Nov 21, 2016

Member

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.

hannesm added some commits Nov 19, 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
@hannesm

This comment has been minimized.

Member

hannesm commented Nov 21, 2016

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

This comment has been minimized.

Member

Drup commented Nov 22, 2016

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

@hannesm hannesm referenced this pull request Nov 25, 2016

Merged

New order #84

@hannesm

This comment has been minimized.

Member

hannesm commented Nov 26, 2016

superseeded by #84

@hannesm hannesm closed this Nov 26, 2016

@hannesm hannesm deleted the hannesm:no-opam branch Dec 10, 2016

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