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

travis: add a POST_INSTALL_HOOK that links a unikernel #55

Merged
merged 3 commits into from May 16, 2019

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented May 16, 2019

as requested in #24. From #53

So, if you don't mind, I'd like to ask that you manually test this with all the supported compiler compiler versions (as listed in .travis.yml). No need to test with different Solo5 bindings, one target is fine.

actually, I do mind ;) -- that's why 4c1c42f includes a (basic) solution here where travis after compiling and installing ocaml-freestanding executes a shell script which installs mirage in order to compile and link a minimal unikernel.

travis run differences (IMHO acceptable for the added benefit):

A failing run (I removed gettimeofday in 3c25294, required by OCaml runtime) can be seen at https://travis-ci.org/mirage/ocaml-freestanding/jobs/533099435 -- i.e. the travis run fails, and you can see the output at the end.

The second commit here, 990de6b, is an artifact of a compiler issue we encountered in mirage-kv 2.0.0 (see mirage/mirage-kv@3df5b30), which is not yet properly propagated through mirage opam packages (see mirage/mirage#988). Thus it just reflects reality, and removes the 4.04.2 bits and pieces from ocaml-freestanding.

@hannesm
Copy link
Member Author

hannesm commented May 16, 2019

I've two three more thoughts on this:

  • A more lightweight (i.e. less dependencies) approach would be to run the link command mirage does manually in the test here, but I don't think it is worth it (given that it requires duplicating parts of mirage logic for this, and ocaml-freestanding's only user is mirage)
  • Since ocaml-freestanding has two C libraries as users (namely OCaml runtime and gmp), should a second unikernel which depends on nocrypto (or zarith-freestanding) be added (or the current one modified to include such a dependency)?
  • EDIT: there's a choice between adding MODE=.. in .travis.yml and MODE=$(echo $EXTRA_DEPS | cut -d '-' -f 3) -- I prefer the former, being more explicit. but we can as well use 60acb92

@mato
Copy link
Contributor

mato commented May 16, 2019

@hannesm Thank you very much for this, it's a big improvement in ensuring that ocaml-freestanding does not break in unexpected ways.

Thus it just reflects reality, and removes the 4.04.2 bits and pieces from ocaml-freestanding.

I hadn't realised that Mirage no longer supports OCaml 4.04.2, thanks for pointing this out. Nit: In the change to README.md, you may as well specify 4.07.1 as the upper bound since we do test with that.

A more lightweight (i.e. less dependencies) approach would be to run the link command mirage does manually

That was the approach I tried yesterday, but it does indeed result in a lot of duplication. Your solution using Travis POST_INSTALL_HOOK is much better.

Since ocaml-freestanding has two C libraries as users (namely OCaml runtime and gmp), should a second unikernel which depends on nocrypto (or zarith-freestanding) be added [...]

This would slow down the Travis builds significantly, so I don't think it's worth it right now.

Regarding manually specifying MODE

I also prefer the manual approach, but can we call it MIRAGE_TEST_MODE to make it more explicit?

I'm happy with merging this after addressing the minor comments (README.md and naming of MODE), followed by #53.

@hannesm
Copy link
Member Author

hannesm commented May 16, 2019

@mato done in ca675eb, I'd suggest to wait for travis and then squash-merge

@mato mato merged commit 9f2e359 into mirage:master May 16, 2019
@hannesm hannesm deleted the link-test branch May 16, 2019 11:06
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

2 participants