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

Wasm of ocaml support #10695

Closed
wants to merge 7 commits into from
Closed

Wasm of ocaml support #10695

wants to merge 7 commits into from

Conversation

vouillon
Copy link
Member

@vouillon vouillon commented Jul 4, 2024

This add the following:

  • a (js_of_ocaml (wasm_files (<files-list>)) option in library and executable stanzas to specify wasm_of_ocaml JavaScript and Wasm runtime files.
  • a (js_of_ocaml (submodes <submodes>)) option where each submode is either js or wasm in the environment and in library and executable stanzas to specify whether to generate JavaScript or Wasm code.

When compiling with wasm_of_ocaml, the output is a launcher file name.bc.wasm.js as well as a directory name.bc.wasm.asset containing the Wasm code and possibly some source maps.

When compiling to Wasm but not to JavaScript, a .bc.js file can also be produced for compatibility. It is just a copy of the
bc.wasm.js file.

@vouillon vouillon requested a review from christinerose as a code owner July 4, 2024 13:47
@vouillon vouillon mentioned this pull request Jul 4, 2024
@vouillon

This comment was marked as resolved.

@vouillon vouillon force-pushed the wasm_of_ocaml branch 2 times, most recently from 9fd5335 to c333f4d Compare July 4, 2024 14:06
@rgrinberg rgrinberg added the jsoo label Jul 4, 2024
@rgrinberg rgrinberg requested a review from hhugo July 4, 2024 21:46
@rgrinberg
Copy link
Member

I'm not sure how to add tests since they would require wasm_of_ocaml, which is a bit complicated to install at the moment...

Nix would be a good option

@vouillon

This comment was marked as resolved.

@vouillon vouillon force-pushed the wasm_of_ocaml branch 11 times, most recently from d8b957b to 025ef50 Compare July 6, 2024 16:49
@vouillon
Copy link
Member Author

vouillon commented Aug 1, 2024

@hhugo Can you have a look?

@vouillon vouillon force-pushed the wasm_of_ocaml branch 2 times, most recently from e598946 to 5905f8a Compare August 23, 2024 14:54
doc/wasmoo.rst Outdated Show resolved Hide resolved
doc/wasmoo.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

I've made some suggestions throughout. Mostly formatting stuff for consistency, but there is one area where the sentence was confusing, so I suggested an alternative. If it doesn't work for you, let me know and we'll work on a solution together.

I'm not able to make suggetsions on anything other than the changes made in this PR, as I don't have edit access.

@rgrinberg
Copy link
Member

gentle ping @hhugo

Copy link
Collaborator

@maiste maiste left a comment

Choose a reason for hiding this comment

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

I don't have the proper knowledge to review this PR. However, wouldn't it be easier to review / merge it if the PR was split in two: one with the documentation and one with the code itself?

.github/workflows/workflow.yml Outdated Show resolved Hide resolved
@vouillon vouillon force-pushed the wasm_of_ocaml branch 4 times, most recently from e81884f to 87eb637 Compare September 27, 2024 09:35
Signed-off-by: Jérôme Vouillon <[email protected]>
Signed-off-by: Jérôme Vouillon <[email protected]>
Signed-off-by: Jérôme Vouillon <[email protected]>
Signed-off-by: Jérôme Vouillon <[email protected]>
Signed-off-by: Jérôme Vouillon <[email protected]>
Comment on lines +54 to +56
- ``(js_of_ocaml (submodes <submodes>))`` controls whether to generate
JavaScript, Wasm code, or both. Each submode is either ``js`` or ``wasm``.
The default is to generate JavaScript code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having this, which controls JS vs WASM generation globally, couldn't we add a wasm constructor to the (modes) field in executable stanzas (similar to how there is a js constructor)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been unhappy about submodes but never had enough time to think about it.
I wonder if we could do something with modes of the form (<compilation-mode> <binary-kind>).
We could have (jsoo js) and (jsoo wasm), or something.

IIRC, you want to be able to compile both js and wasm without touching all dune files. We could imagine a new env field that define the semantic of the 'js' mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

This directive can also be included in executable stanzas.

I'm not completely happy with the design either. It might be better to have a separate wasm_of_ocaml field, for instance, to be able to pass different flags to the compilers.

Still, I think we need a global way to specify whether we want to generate JavaScript and/or Wasm code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we could have these in env stanzas:

(js_of_ocaml (by_default_if <blang expression>))
(wasm_of_ocaml (by_default_if <blang expression>))

and a (jsoo default) executable mode.

And the mode (jsoo js) and (jsoo wasm) could be used to generate explicitly some Wasm or JavaScript code.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that this submodes stuff doesn't seem satisfactory. Making it more similar to regular modes would be preferable.

@rgrinberg
Copy link
Member

rgrinberg commented Oct 20, 2024

Continued the PR here #11029 since I'm unable to push here

@rgrinberg rgrinberg closed this Oct 20, 2024
@rgrinberg
Copy link
Member

CI is failing @vouillon

--- a/_build/.sandbox/6975479b7ea39eedbfcff9056c4b3b15/default/test/blackbox-tests/test-cases/wasmoo/inline-tests.t/run.t
+++ b/_build/.sandbox/6975479b7ea39eedbfcff9056c4b3b15/default/test/blackbox-tests/test-cases/wasmoo/inline-tests.t/run.t.corrected
@@ -15,8 +15,17 @@ Run inline tests using Node.js
   $ dune runtest --profile release
   inline tests (Native)
   inline tests (Native)
-  inline tests (Wasm)
-  inline tests (Wasm)
+  File "wasm/dune", lines 1-6, characters 0-142:
+  1 | (library
+  2 |  (name inline_tests_wasm)
+  3 |  (js_of_ocaml (wasm_files wasm.wat) (submodes wasm))
+  4 |  (inline_tests
+  5 |   (modes js)
+  6 |   (backend fake_backend)))
+  wasm_of_ocaml: unknown option '--linkall'.
m  Usage: wasm_of_ocaml [COMMAND] …
+  Try 'wasm_of_ocaml --help' for more information.
+  [1]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants