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

Update to latest mirage #364

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Update to latest mirage #364

merged 1 commit into from
Jun 30, 2023

Conversation

samoht
Copy link
Member

@samoht samoht commented Jun 24, 2023

To be tested with mirage/mirage#1442

@hannesm
Copy link
Member

hannesm commented Jun 25, 2023

that looks nice, but would you mind to separate the ocamlformat changes (whitespaces, new lines) from the actual changes?

@samoht
Copy link
Member Author

samoht commented Jun 26, 2023

Rebased on top of #365 - see 40231d0 for the related changes.

@samoht samoht force-pushed the mirage-keys branch 2 times, most recently from 735bdf8 to 1dd2ded Compare June 26, 2023 10:29
@samoht
Copy link
Member Author

samoht commented Jun 26, 2023

Rebased on top of master.

@samoht samoht merged commit 726ac18 into mirage:main Jun 30, 2023
@samoht samoht deleted the mirage-keys branch June 30, 2023 06:53
@hannesm
Copy link
Member

hannesm commented Jul 3, 2023

Unfortunately this broke mirage-skeleton on main branch with the latest released mirage tool.

It seems like we're still lacking documentation thereof (and I've no clue how the CI system is configured / working), but I'd really like to have:

  • main branch of mirage-skeleton works with latest opam-released mirage
  • dev branch of mirage-skeleton works with main branch of mirage

this allows people passing by to just use mirage & mirage-skeleton as documented. It would be great to have mirage/mirage#1381 / #338 in place very soon to get nice error messages to the users instead of confusing them with errors such as:

(hello-key unikernel)
776.935s:# File "unikernel.ml", line 7, characters 2-20:
776.935s:# 7 |   Mirage_runtime.key key
776.935s:#       ^^^^^^^^^^^^^^^^^^
776.935s:# Error: Unbound value Mirage_runtime.key
(static-website-tls)
581.409s:+ mirage configure -t hvt --http 80 --https 443 '--allocation-policy=best-fit' --context ./mirage/context --no-extra-repo
582.639s:mirage: unknown option '--http', did you mean '--help'?
582.639s:        unknown option '--https'.
582.639s:Usage: mirage configure [OPTION]…
582.639s:Try 'mirage configure --help' or 'mirage --help' for more information.

what do you think?

@hannesm
Copy link
Member

hannesm commented Jul 3, 2023

^^ @samoht. In a nice world, the mirage-CI would actually enforce this -- so testing pushes/PRs to the main branch with the released mirage, while testing pushes/PRs to the dev branch with mirage pinned to its main branch. and also, testing PRs to mirage with the dev branch of mirage-skeleton (+to test with PR comment thingy).

@samoht
Copy link
Member Author

samoht commented Jul 3, 2023

woops, I should have opened this PR against dev. Let me try to fix this...

And yes your scheme makes sense (and corresponds to my understanding of our current worfklows) but the current CI doesn't test this: https://github.com/ocurrent/mirage-ci/blob/main/src/pipelines/PR.ml#L585-L589. Let me try to fix this (bis).

@samoht
Copy link
Member Author

samoht commented Jul 3, 2023

I've reverted this PR: #375 and merged it to unbreak the compilation of mirage-skeleton main's branch with the latest released version of mirage.

@hannesm
Copy link
Member

hannesm commented Jul 4, 2023

see ocurrent/mirage-ci#37 which may do the thing we're interested in!?

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.

2 participants