-
Notifications
You must be signed in to change notification settings - Fork 454
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
[interpreter] Add dune support to build the #1408
Conversation
Thanks, this looks nice! Personally, I'd like to support dune. However, there are two hurdles we'd need to address:
So there's some effort involved. Would you be interested in going through that? |
Yes sure, I'm interested. I'll update the GitHub actions, Makefiles and other things to use dune. Could you allow the workflow to run so I can test my changes ? |
@rossberg I started to look into it. I have a few questions already.
I decided to merge all the different libraries I had initially as it's what seems to be expected elsewhere. That leads to some ugly EDIT: meh, it seems you'll have to approve the workflow each time as long as I havn't contributed to the repo... |
Sounds fine to me.
You mean to (re)generate the batch file or to run it? For generation, you can of course assume dune. But the resulting batch script ought to run in a bare Windows shell, without any assumption about non-standard tools (or Internet), other than ocamlc being installed. That's the point of this file.
Not sure that would be a good idea, since the test suite per se does not currently depend on OCaml and it's better to keep dependencies minimal. Isn't that an orthogonal question anyway?
There already is the library module |
I meant to run it. OK so no Internet mean no dune bootstrap. I'll try to find an easy way to generate it from dune then.
I did not notice the testsuite was made to be usable from somewhere else. I'll leave the python script then. It was just to remove a bunch of lines from the Makefile and is not needed.
I didn't see the one in jslib... Indeed that would be a little bit confusing. But it's also confusing not to rename it because you end up with the following (executable
(public_name wasm)
(name main)
(modules main)
(modes byte exe)
(libraries wasm)) But, I'll leave it as is. Thanks for enabling the workflow again ! |
dune, update GitHub actions
I have to wait before a new dune release is published as there was a bug in dune - it was already fixed when I found it. |
I had the same issue:
Until this MR get merged, the fix is to not use OCaml 4.13 (aka. downgrade to 4.12) as shown in ocaml/opam-repository@2d33cc1#diff-351dcabc190f8ca39b4f7cb6fa99ed765917c41f1c19e69c3f19ac02278e7fc6 |
@zapashcanon, what is the current status of this PR? |
@zapashcanon, do you still intend to rebase and land this PR, or shall I close it? |
Hi,
I wasn't able to compile the interpreter anymore with the provided Makefile :
I didn't want to get into the troubles of hand written Makefiles so I simply added suppor for dune which nowadays is the standard build system for OCaml.
This operation is in the first commit. It consists of addind a
dune-project
file and manydune
files, one for eachlibrary
. In thedune-project
I disabled transitive dependencies as it avoids some troubles, this forces to explicit all dependencies of a library in the dune file, making it more verbose. Regarding thedune
files. I had to split some directories into many libraries to avoid dependency cycles. Another solution would have been to use a singledune
file at the root and include all the sub directories but that's ugly so I decided to go the other way.The second commit is just to please dune as it enables more warnings by default and treats them as errors. It's mainly (only ?) some
unused variable foo
warnings, when possible I just renamedfoo
into_foo
, but sometimes, for records, I had to use_
or added alet ... in
when a single fields of the records was matched.I also couldn't use the
ocamllex
stanza directly (because there's no way to pass the-ml
flag) so I used a custom rule.You can now just
dune build @all
in the repository and get a working interpreter.Note that all of that have no impact on the current Makefile system.
Later, if there's interested and this is merged, I can also add support for tests, doc generation, code coverage with dune.