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

[interpreter] Add dune support to build the #1408

Closed
wants to merge 4 commits into from

Conversation

zapashcanon
Copy link
Contributor

@zapashcanon zapashcanon commented Jan 11, 2022

Hi,

I wasn't able to compile the interpreter anymore with the provided Makefile :

$ make
echo >_tags "true: bin_annot"
echo >>_tags "true: debug"
echo >>_tags "<{util,syntax,binary,text,valid,runtime,exec,script,host,main,tests}/*.cmx>: for-pack(Wasm)"
ocamlbuild -lexflags -ml -cflags '-w +a-4-27-42-44-45 -warn-error +a-3' -I util -I syntax -I binary -I text -I valid -I runtime -I exec -I script -I host -I main -I tests -libs bigarray -quiet main.native
+ /home/zapashcanon/.config/opam/4.13.0/bin/ocamlc.opt -c -w +a-4-27-42-44-45 -warn-error +a-3 -g -bin-annot -I exec -I text -I script -I binary -I host -I util -I runtime -I valid -I syntax -I tests -I main -o exec/ixx.cmo exec/ixx.ml
File "exec/ixx.ml", line 1:
Error (warning 70 [missing-mli]): Cannot find interface file.
Command exited with code 2.
make: *** [Makefile:68 : main.native] Erreur 10
rm _tags

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 many dune files, one for each library. In the dune-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 the dune files. I had to split some directories into many libraries to avoid dependency cycles. Another solution would have been to use a single dune 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 renamed foo into _foo, but sometimes, for records, I had to use _ or added a let ... 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.

@rossberg
Copy link
Member

Thanks, this looks nice! Personally, I'd like to support dune. However, there are two hurdles we'd need to address:

  • If we support dune in this repo, then we must ensure that it does not bit-rot and always builds successfully. In practice, that requires pre-commit checks verifying the build, which in turn only makes sense if everybody includes dune in their workflow. Consequently, we need to change Makefiles etc to switch from ocamlbuild to dune.

  • Dune introduces a new dependency. That may be less of an issue since ocamlbuild has been removed from the core distribution as well, but we'd still have to make sure that we adjust dependencies everywhere and that all workflows (e.g., github actions, Windows make.bat, etc) remain operational.

So there's some effort involved. Would you be interested in going through that?

@zapashcanon
Copy link
Contributor Author

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 ?

@zapashcanon
Copy link
Contributor Author

zapashcanon commented Jan 20, 2022

@rossberg I started to look into it. I have a few questions already.

  • Can I switch from Bucklescript to js_of_ocaml ? The latter is the only one I know (and the more used I believe) and it would allow me to easily let dune take care of building the js library.
  • In the winmake.bat script, can I assume that dune is available ? If I can't, would it be OK to just download the dune sources and bootstrap it to easily build the interpreter ? I first tried something with dune rules to generate the file as you were doing before but it was getting really complicated (I'll ask if there's an easiest way to do it with dune but I'm afraid there isn't).
  • Can I rewrite the test/core/run.py file in OCaml (or pure dune if that's not too hard) ? It'll make the Makefile much nicer.
  • Can I rename interpreter/main/main.ml to interpreter/main/wasm.ml (and the produced executable too) ? It's simply to avoid having a different name and public_name.

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 copy_files but I guess we can live with it.

EDIT: meh, it seems you'll have to approve the workflow each time as long as I havn't contributed to the repo...

@rossberg
Copy link
Member

  • Can I switch from Bucklescript to js_of_ocaml ? The latter is the only one I know (and the more used I believe) and it would allow me to easily let dune take care of building the js library.

Sounds fine to me.

  • In the winmake.bat script, can I assume that dune is available ? If I can't, would it be OK to just download the dune sources and bootstrap it to easily build the interpreter ? I first tried something with dune rules to generate the file as you were doing before but it was getting really complicated (I'll ask if there's an easiest way to do it with dune but I'm afraid there isn't).

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.

  • Can I rewrite the test/core/run.py file in OCaml (or pure dune if that's not too hard) ? It'll make the Makefile much nicer.

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?

  • Can I rename interpreter/main/main.ml to interpreter/main/wasm.ml (and the produced executable too) ? It's simply to avoid having a different name and public_name.

There already is the library module Wasm produced via pack. There also is a wasm.ml in jslib. So that would rather confusing.

@zapashcanon
Copy link
Contributor Author

zapashcanon commented Jan 21, 2022

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.

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.

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?

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.

There already is the library module Wasm produced via pack. There also is a wasm.ml in jslib. So that would rather confusing.

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 interpreter/main/dune:

(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 !

@zapashcanon
Copy link
Contributor Author

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.

@concatime
Copy link

I had the same issue:

Error (warning 70 [missing-mli]): Cannot find interface file.

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

@rossberg
Copy link
Member

rossberg commented Aug 4, 2022

@zapashcanon, what is the current status of this PR?

@rossberg rossberg changed the title add dune support to build the interpreter [interpreter] Add dune support to build the Aug 4, 2022
@rossberg
Copy link
Member

@zapashcanon, do you still intend to rebase and land this PR, or shall I close it?

@zapashcanon
Copy link
Contributor Author

@rossberg, sorry for the delay. #1459 was merged and it's what I initially wanted to add. I'm not really interested into getting the windows support (it was requested to accept the switch to dune but apparently that changed). I'm closing.

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