-
Notifications
You must be signed in to change notification settings - Fork 53
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
Dune #158
base: master
Are you sure you want to change the base?
Conversation
Awesome! I'll take a look at the Mirage end later this afternoon, and review the dune changes as well. |
@pqwy AFAICT the current "mirage+dune cross" story is (as shown in https://github.com/inhabitedtype/bigstringaf - this will hopefully be less horrible soon)
|
nocrypto.opam
Outdated
|
||
depends: [ | ||
"ocaml" {>= "4.03.0"} | ||
"dune" {build} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"dune" {build} | |
"dune" {build & >="1.7"} |
to match the dune version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, but I'm not sure what is the minimal dune language version that this needs?
"ocplib-endian" | ||
"zarith" | ||
("mirage-no-xen" | ("mirage-xen" & "zarith-xen")) | ||
("mirage-no-solo5" | ("mirage-solo5" & "zarith-freestanding")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these still needed since the mirage packages are removed from this repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mirage package, nocrypto.mirage
, is still present. It's just that it's meaningless because the stubs are not cross-compiled.
Hence the PR: the state of this branch is still incoherent.
@hannes I'm looking at the new cross-compilation story for this, so that we can keep the mirage-specific build pieces out of the mainline nocrypto repository and only use it to generate nocrypto-freestanding, nocrypto-xen, etc. I'm just working on a tree to demonstrate this (and reply to your mirageos-devel email), so I'll update as soon as I have that (hopefully tomorrow). This PR looks good to me to merge and I can send any tweaks in a followup one. |
@avsm I cannot merge this as it is, because it immediately breaks mirage compatibility. In general, I would strongly prefer a DRY solution, of the sort that I lost giving up ocamlbuild. You suggestion is to remove mirage-related bits, and re-add them in a separate repository, correct? If this is the case, that repo needs to go up before this is merged, so that I can review the exact approach before proceeding. |
let fs = match Sys.getenv evar with | ||
"true" -> flags | ||
| "false" -> [] | ||
| _ -> auto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this should be an error (or at least a warning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... right, if you set it but not to true
or false
. I was hoping I'd snatch a parser for configuration things from somewhere else, before I start writing that in the discovery tool.
I did try to emit text to confirm override, but dune seems to swallow my stderr
.
@@ -0,0 +1,3 @@ | |||
(lang dune 1.7) | |||
(name nocrypto) | |||
(version %%VERSION_NUM%%) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this is necessary
(version %%VERSION_NUM%%) | |
(version %%VERSION_NUM%%) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, it's not strictly necessary. But it adds version to META
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
META files are automatically generated with the correct version. For example, ̀cstructdoesn't have
(version)` here, and you can check that the installed META file has the correct version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not what I'm seeing while playing with a pinned package. I won't pretend to understand why.
|
||
let some x = Some x | ||
|
||
[@@@ocaml.warning "-3"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's for Lwt_sequence
it's now possible to use Lwt_dllist
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or more precisely, Lwt_main.Enter_iter_hooks
. This is not the changeset for that.
Regarding mirage compatibility, I've done some experiments and it's possible to separate the nocrypto core and mirage packages in several repositories. I've done the modifications here:
What changed in nocrypto is:
The problem, for now, is that it's a 'file-by-file' procedure, meaning that we can't export and re-import a whole directory, so the work remains tedious. |
I'll be honest -- this falls a little short of looking impeccable. It seems that there is a choice between exporting the build internals of a library to Mirage, versus exporting the build internals of Mirage to a library. Is it not possible to simply add build flags for particular tools to an invocation of Because the only problem here is a limited form of cross-compilation: the C compiler needs to be given certain flags, the results need to be stored somewhere, and this somewhere needs to play nicely with the final step of linking the unikernel. |
I definitely agree with you, this was to show that it's possible but definitely not good looking.
It's possible using different build contexts with different flags set, but I'm not sure the tooling is ready for that yet. |
If I'm not mistaken, as long as Assuming
is there anything that makes this scheme infeasible? It would answer Adam Steen's question on the list from yesterday and make builds generally more controllable. |
So it looks like we're going with what you suggested, and therefore it should be good for
One more thing I noticed is that |
I think @hannesm generally wants to reduce his dependency on ppx, so removing sexp derivers from tls is likely the way to go |
Well the problem is that |
Once this goes in, there is no cross-compilation for mirage. What is the current story? @avsm