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

Consolidate mirage bootvar #1533

Merged
merged 1 commit into from
May 17, 2024
Merged

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented May 9, 2024

I found an even simpler example than "mirage-time" in respect to "how to use dune variants": boot parameters.

We used to have mirage-bootvar-xen, mirage-bootvar-unix, mirage-bootvar-solo5, and the parse-argv opam packages that were all involved.

Now, I created (retaining history) the single repository https://github.com/mirage/mirage-bootvar. This uses dune variants, the default is the unix implementation.

What is interesting about bootvar is that no one bothered to functorize it ;) Also, it shows how a unikernel could still decide to use "no_argv", bypassing any dune variant restrictions (I'm not sure whether this is desirable, but it is possible -- this also sheds some light that we could use configure-time keys to select some other implementation).

Anyways, this is much simpler than #1529 and you may enjoy looking into it.

EDIT: CI fails since the mirage-bootvar is not released, I'm happy to do that if no one objects about the approach taken.

Copy link
Member

@reynir reynir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me that looks great!

@hannesm hannesm force-pushed the consolidate-mirage-bootvar branch from cc63ae0 to 07b65c3 Compare May 9, 2024 19:03
@hannesm
Copy link
Member Author

hannesm commented May 9, 2024

To go a bit deeper on my motivation: I find this makes it much easier for developers to discover the code MirageOS uses. It can easily be compared what the difference between the different targets is, and API changes can be done very locally without having to think about the "release train" and in which order to release what.

Furthermore, the complexity on tooling (let's say opam) is reduced by the reduction of opam packages that are required in the universe.

Also, the maintenance of mirage is relaxed - we can replace (archive) 4 git repositories with all sorts of (partially outdated) metadata by a single one.

I guess a question is whether we should "buy in" into dune variants -- and my point of view is that it radically simplifies our tooling, plus enhances the developer experience (not shown here, since there isn't any functor removed) -- and one of the (at least for me) most useful feature of IDEs (or editor support), the "jump to definition", is finally on the horizon to work just neatly.

@hannesm hannesm mentioned this pull request May 16, 2024
14 tasks
@palainp
Copy link
Member

palainp commented May 16, 2024

Thank you, this PR looks good to me!

@samoht
Copy link
Member

samoht commented May 17, 2024

This change seems very uncontroversial and so looks great to me - could you please update the test outputs to please the CI?

@hannesm
Copy link
Member Author

hannesm commented May 17, 2024

This needs to wait for ocaml/opam-repository#25887 being merged to have the CI pass.

@hannesm hannesm force-pushed the consolidate-mirage-bootvar branch from 07b65c3 to f2067d3 Compare May 17, 2024 13:07
@hannesm
Copy link
Member Author

hannesm commented May 17, 2024

The commit a0dc4ac fixes an actual bug that was discovered due to how mirage-bootvar is put into place. I'll mention this explicitly in the CHANGES.

@hannesm hannesm force-pushed the consolidate-mirage-bootvar branch from 69ef9e7 to 541ee38 Compare May 17, 2024 13:43
@hannesm
Copy link
Member Author

hannesm commented May 17, 2024

I moved forth and back... so our Package.v ~sublibs:["x"] "foo" does add a dependency onto foo.x.
The documentation said [ "foo" ; "foo.x" ] would be the dependency.

Reality shows there are some packages that don't provide foo -- so for the mirage-bootvar case, I added support in package for ~sublibs:[""], and specified the package as: "mirage-bootvar" ~sublibs:[""; sublib] with sublib being "unix"|"solo5"|"xen" (depending on the target). This should result in CI compiling :)

this reduces the dependency cone and complexity for figuring out how the
argument vector is retrieved and parsed by MirageOS unikernels
@hannesm hannesm force-pushed the consolidate-mirage-bootvar branch from 541ee38 to 2026b03 Compare May 17, 2024 13:58
@hannesm hannesm merged commit cbf5d44 into mirage:main May 17, 2024
1 of 3 checks passed
@hannesm hannesm deleted the consolidate-mirage-bootvar branch May 17, 2024 14:37
hannesm added a commit to hannesm/opam-repository that referenced this pull request May 17, 2024
CHANGES:

- BREAKING: remove `~name` parameter from Mirage.Runtime_args.create
  (mirage/mirage#1541 @samoht, fixes mirage/mirage#1532)
- BREAKING: remove `~name` parameter from Mirage.runtime_arg, and use a
  string (instead of a format string) as third parameter (mirage/mirage#1541 @samoht)
- constrain the `start` function to `unit Lwt.t`. Previously, there was no
  restrictions, and lots of time was spent in debugging when a unikernel
  resulted in `unit Lwt.t Lwt.t` (@Julow mirage/mirage#1524)
- revise man page sections and ordering: ARGUMENTS, OPTIONAL, NETWORK OPTIONS,
  DISK OPTIONS, LOG AND MONITORING OPTIONS, OCAML RUNTIME OPTIONS. Previously,
  the ARGUMENTS and OPTIONS were put later, and were hard to find. These are
  the sections where unikernel-specific arguments are put by default
  (mirage/mirage#1531 @hannesm @reynir)
- add --net=host and --net=ocaml to reduce confusion. --net=host uses the
  TCP/IP socket stack, --net=ocaml the OCaml network stack (mirage/mirage#1525 @hannesm)
- quote Runtime_arg.call (mirage/mirage#1522 @Julow)
- documentation fixes (inline examples @Julow mirage/mirage#1523, @hannesm mirage/mirage#1537 (fixes
  mirage/mirage#1512 reported by @reynir), Runtime_args.create mirage/mirage#1541 @samoht)
- fix the build instructions of the generated opam file: since 4.5.0
  `mirage build` is no longer available, use `make "build"` (mirage/mirage#1527 @hannesm)
- add RELEASE.md, a guide on how to cut a mirage release (mirage/mirage#1519 @samoht)
- allow git 3.16 (mirage/mirage#1536 @hannesm)
- use mirage-bootvar (using dune variant) instead of parse-argv and
  mirage-bootvar-xen, mirage-bootvar-solo5, mirage-bootvar-unix
  (mirage/mirage#1533 @hannesm)
- BUGFIX: reset the lexer location before applying functors in generated code
  (mirage/mirage#1539 @samoht, fixes mirage/mirage#1520 @hannesm)
- BUGFIX: fix off-by-one locations for mirage/main.ml (mirage/mirage#1540 @samoht, fixes
  mirage/mirage#1528 @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

Successfully merging this pull request may close these issues.

None yet

4 participants