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

Support build with Mirage 4.5.1 and OCaml 5.2 #838

Closed
wants to merge 3 commits into from

Conversation

shym
Copy link

@shym shym commented Jun 6, 2024

This PR fixes the small details needed to build with Mirage 4.5.1 and OCaml 5.2, in part as a way to test the PR to OCaml-Solo5 that brings support for 5.2. It comes with a CI configuration to make sure that it doesn’t build just by accident locally and also as a way to document the currently required workarounds:

  • using a custom overlay repository for the lwt_ppx package until the corresponding PR is merged,
  • pinning the ocaml-solo5 package (obviously),

but I don’t know if we want to integrate that CI run (at least as is) into the repository.

This was tested briefly locally by launching the unikernel and visiting a couple of pages. Logs didn’t seem to contain anything unexpected.

Copy link
Member

@Firobe Firobe left a comment

Choose a reason for hiding this comment

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

It works on my machine ™️

CI failures are unrelated, and should go away after a rebase on #839

.github/workflows/mirage.yml Outdated Show resolved Hide resolved
.github/workflows/mirage.yml Outdated Show resolved Hide resolved
Makefile Outdated
@@ -1 +1,3 @@
# Generated by mirage.v4.5.1

-include Makefile.user
Copy link
Member

Choose a reason for hiding this comment

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

This was explicitly avoided in d25d82b

Having mirage configure override this file is a bit annoying indeed, I wonder if there is a better solution to have both workflows supported

Copy link
Author

Choose a reason for hiding this comment

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

You’re right.
I’ve instead just removed the Makefile in the GHA workflow.
I’ve also added a note to that effect in the README until a better solution is found.

@hannesm
Copy link
Member

hannesm commented Sep 11, 2024

has this been superseeded by #840?

@Firobe
Copy link
Member

Firobe commented Sep 11, 2024

Essentially yes, though it would be good to keep it open for now, as a reminder that changes made to next for this should be merged back to master if we're happy with them.

@shym
Copy link
Author

shym commented Sep 11, 2024

has this been superseeded by #840?

#840 merged that change into next. But then I guess we’ll end up pulling the exact version that went into next for the next main rather than this PR. So I close it.

@shym shym closed this Sep 11, 2024
@shym
Copy link
Author

shym commented Sep 13, 2024

Our messages were simultaneous :-)
@Firobe I let you reopen this PR if you think it’s worth it.

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.

3 participants