Skip to content

Use virtual_modules in Dune 1.7#59

Closed
avsm wants to merge 3 commits intomirage:masterfrom
avsm:vmodules
Closed

Use virtual_modules in Dune 1.7#59
avsm wants to merge 3 commits intomirage:masterfrom
avsm:vmodules

Conversation

@avsm
Copy link
Copy Markdown
Member

@avsm avsm commented Jun 26, 2019

This allows use of the interface file separately from the implementations. We now provide separate implementations that use dune variants so that they can be found via a variants tag search:

  • unix (Unix or Windows)
  • js (JavaScript)
  • xen (Xen)

This cleans up the utop toplevel support as well to work by default now. (@avsm, fixes #50)
Also use implicit transitive dependencies, which will become the default in dune 2.0

I am noticing some local problems with opam installations though:

#=== ERROR while compiling io-page-unix.2.2.0 =================================#
# context     2.0.4 | macos/x86_64 | ocaml-base-compiler.4.07.1 | pinned(git+file:///Users/avsm/src/git/mirage/io-page#vmodules#a58f53d0)
# path        ~/.opam/ocaml-base-compiler.4.07.1/.opam-switch/build/io-page-unix.2.2.0
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p io-page-unix -j 27
# exit-code   1
# env-file    ~/.opam/log/io-page-unix-37329-03b1a4.env
# output-file ~/.opam/log/io-page-unix-37329-03b1a4.out
### output ###
# File "lib/unix/dune", line 7, characters 14-21:
# 7 |   (implements io-page)
#                   ^^^^^^^
# Error: Library implementation io-page for variant "unix" implements a library outside the project. This is forbidden.

which is why I am marking this as a draft PR to work through the CI first.

@avsm
Use virtual_modules in Dune 1.7
a58f53d 
This allows use of the interface file separately from the
implementations.

We now provide separate implementations that use dune variants
so that they can be found via a variants tag search:

- `unix` (Unix or Windows)
- `js` (JavaScript)
- `xen` (Xen)

This cleans up the utop toplevel support as well to work by
default now. (@avsm, fixes mirage#50)

Use implicit transitive dependencies, which will become the
default in dune 2.0
@avsm avsm mentioned this pull request Jun 26, 2019
Closed
This was referenced Jun 26, 2019
Closed
Closed
djs55
djs55 reviewed Jun 27, 2019
View reviewed changes
CHANGES.md
- `xen` (Xen)

This cleans up the utop toplevel support as well to work by
default now. (@avsm, fixes #50)
Copy link
Copy Markdown
Member

@djs55 djs55 Jun 27, 2019

Choose a reason for hiding this comment

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

awesome!

@dinosaure
Copy link
Copy Markdown
Member

dinosaure commented Jun 27, 2019

Should related to mirage/irmin#338

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Jun 27, 2019

I still don't fully understand variants, but in the above list, what is happening with freestanding? or is this a non-issue? AFAIR the C stubs are provided by mirage-solo5 atm, but what does dune do when asked to use a variant that does not exist for a certain package? take the "default" one (is there a default?)?

and what is the extensibility story - do all variants now need to end up in this io-page repository, or are they free to be defined elsewhere? (how will people/programs know about the existance elsewhere, what happens if there are multiple implementations of a single package for the same variant?)

thanks for working on this! and sorry for asking questions, but I could not discover where/whether they've been answered previously (please point me to a comment if this is the case)

@avsm avsm mentioned this pull request Jul 10, 2019
Closed
@samoht
Copy link
Copy Markdown
Member

samoht commented Aug 3, 2019 •
edited
Loading

Do we really need variants when we could just rely on dune doing the correct cross-compilation of C bindings? e.g. in #60 I've simply removed the unix and xen subpackages and everything seems much cleaner/simpler :-)

@samoht samoht closed this Aug 6, 2019
@samoht
Copy link
Copy Markdown
Member

samoht commented Aug 6, 2019

Closing in favour of #60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@djs55 djs55 djs55 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Doesn't work from utop : function `mirage_alloc_pages' is not available

5 participants

@avsm @dinosaure @hannesm @samoht @djs55