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

Variants #48

Closed
wants to merge 9 commits into from
Closed

Variants #48

wants to merge 9 commits into from

Conversation

dinosaure
Copy link
Member

A proposal about #46

@dinosaure dinosaure requested review from samoht and hannesm April 7, 2020 10:50
des_generic
entropy_cpu_stubs)
(c_flags (:standard) (:include cflags.sexp)))
(modules mirage_crypto ccm cipher_block cipher_stream hash uncommon)
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, if I don't explicitly all modules into this field (even if some appear into private_modules), I got a compilation error. Not sure about implication of it.

@hannesm
Copy link
Member

hannesm commented Apr 7, 2020

I don't quite understand why a native.mli is needed now. before this PR, the cross-compilation created an (empty!) mirage_crypto_freestanding.ml (and compilation resulted in cma/cmi/cmt/cmx/cmxa/cmxs thereof). can't we similarly use an empty mli, generated on-the-fly by dune somehow, and declare this as variant (implementation only matters in terms of C code anyways)?

@dinosaure
Copy link
Member Author

I don't quite understand why a native.mli is needed now.

This is needed for variants/virtual library where a common (needed) interfaces can hide several implementations (unix, xen and freestanding). So, native.mli is required to be used by others modules (such as uncommon.ml or hash.ml) even if the implementation of it does not exist.

the cross-compilation created an (empty!) mirage_crypto_freestanding.ml (and compilation resulted in cma/cmi/cmt/cmx/cmxa/cmxs thereof)

mirage_crypto_freestanding was empty because we just wanted to produce a libmirage_crypto_freestanding_stubs.a. Note that we don't want to link or use it but just generate the needed artifact for a Solo5 unikernel.

Then, the current mirage tool will take care about the link and will use libmirage_crypto_freestanding.a or libmirage_crypto_xen.a according the target - again, without an introspection of needed static C library notified into *.cmxa (even if they are empty). The current status of mirage-crypto generates a mirage_crypto.cmxa with -lmirage_crypto_stubs which corresponds to a standalone compilation (incompatible with Xen and Solo5) - again, this wrong behavior appears only with -output-complete-obj.

The idea is to use a sub-library mirage_crypto_native which does not refer to any implementations (unix, xen or freestanding) and let dune (with variants) to choose one of them at the end.

use an empty mli, generated on-the-fly by dune somehow, and declare this as variant (implementation only matters in terms of C code anyways)?

The problem is that native does not come with any implementations (mirage_crypto_native does have any *.cmx). So, we need something (a *.mli) to say to others that this module should provides these functions. By this way, they are available but does not exists yet - you need a link to mirage-crypto.native.{unix,freestanding,xen}.

@hannesm hannesm mentioned this pull request Apr 13, 2020
@hannesm
Copy link
Member

hannesm commented Apr 30, 2020

should this be part of the next release? (sorry, I lost track how mirage4 plans to handle C resources). if it is the case, would you mind to rebase this on top of master and look into the CI failures (the windows runner has some issues).

@hannesm
Copy link
Member

hannesm commented Jun 17, 2020

dune removed their variants, the MirageOS 4 story is laid out slightly different now. closing this PR.

@hannesm hannesm closed this Jun 17, 2020
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