-
Notifications
You must be signed in to change notification settings - Fork 401
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 #8278
Wasm_of_ocaml support #8278
Conversation
src/dune_rules/jsoo/js_of_ocaml.ml
Outdated
}) | ||
else | ||
fields | ||
(let+ flags = Flags.decode | ||
and+ javascript_files = | ||
field "javascript_files" (repeat string) ~default:[] | ||
in | ||
{ flags; javascript_files }) | ||
and+ wasm_files = field "wasm_files" (repeat string) ~default:[] in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to enable this only for newer dune lang versions. see how it's done above if syntax_version < (3, 0) then
src/dune_rules/jsoo/jsoo_rules.ml
Outdated
@@ -137,6 +137,8 @@ end | |||
|
|||
let install_jsoo_hint = "opam install js_of_ocaml-compiler" | |||
|
|||
let install_wasoo_hint = "opam install wasm_of_ocaml-compiler" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has this been released on opam already ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. I think we should wait for thinks (wasm_of_ocaml
, the WebAssembly Garbage Collection proposal, and related tools) to stabilize a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hint is a bit misleading then. Should it say something like
opam pin add wasm_of_ocaml-compiler https://github.com/ocaml-wasm/wasm_of_ocaml.git
@rgrinberg, any opinion ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no good hint to give, then I would just not give any. You can always add a hint in the future when the situation is more clear.
src/dune_rules/jsoo/jsoo_rules.ml
Outdated
((match ctarget with | ||
| JS -> [] | ||
| Wasm -> wasm_runtime_files libs) | ||
@ jsoo_runtime_files libs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the wasm target depend on the jsoo_runtime_files ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JavaScript runtime files are parsed to get purity information about primitives. We want also to include JavaScript files with no annotation, like lib/virtualdom.compiled.js
in package virtual_dom
. And I have just added a way to link JavaScript code referenced by the Wasm runtime code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hhugo can we run the wasm compilation with sandboxing? Or are there still issues with source maps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vouillon, do you plan to support sourcemap for the wasm target ? What's the content of the js file when target is wasm ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to support them eventually. Probably not a high priority, though.
What are the issues with source maps?
The contents of the js file is currently a mix of js code generated by the js_of_ocaml linker and of javascript code directly copied from a file. I should certainly improve on this, and then one could add a sourcemap to this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with source map is that it might need to access sources to embed them. We do not currently track this kind of dependencies. Preventing us from using sandboxing.
src/dune_rules/jsoo/jsoo_rules.ml
Outdated
exe_rule ~ctarget cc ~javascript_files ~wasm_files:[] ~src ~target ~flags | ||
>>= Super_context.add_rule sctx ~loc ~dir ~mode | ||
| _, Wasm -> | ||
exe_rule ~ctarget cc ~javascript_files ~wasm_files ~src ~target ~flags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be an error if Separate_compilation, Wasm
? instead of silently ignoring cmode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that you can switch to producing Wasm code just by setting the target, without having to specify a compilation mode as well.
Do you think it would be better to change Super_context.js_of_ocaml_compilation_mode
to not default to Separate_compilation
when the target is wasm
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered making wasm part of compilation_mode
?
I'm also happy to keep this as is and revisit later if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is what I did at first. But this is orthogonal. I plan to implement separate compilation in the middle term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave it as is then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment in the code saying that separate compilation is coming later.
a9a82a2
to
7b05a53
Compare
src/dune_rules/jsoo/js_of_ocaml.ml
Outdated
@@ -161,24 +184,31 @@ module Env = struct | |||
fields | |||
@@ let+ compilation_mode = | |||
field_o "compilation_mode" Compilation_mode.decode | |||
and+ target = field_o "target" Target.decode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be guarded based on dune lang version as well
src/dune_rules/super_context.mli
Outdated
@@ -38,6 +38,8 @@ val js_of_ocaml_runtest_alias : t -> dir:Path.Build.t -> Alias.Name.t Memo.t | |||
val js_of_ocaml_compilation_mode : | |||
t -> dir:Path.Build.t -> Js_of_ocaml.Compilation_mode.t Memo.t | |||
|
|||
val js_of_ocaml_target : t -> dir:Path.Build.t -> Js_of_ocaml.Target.t Memo.t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you inline this function in the one place where it's being used?
In general the code looks really good to me. Awaiting for approval from Hugo. Also don't forget to:
|
The dune lang constraint will need to be updated to 3.11.0 (dune.3.10) as been released alrady) |
@hhugo do you want this to be ready for 3.11? |
I think this can wait. I'm wondering whether this should work like |
otherlibs/dune-rpc/private/types.ml
Outdated
@@ -35,7 +35,7 @@ end | |||
module Version = struct | |||
type t = int * int | |||
|
|||
let latest = (3, 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you rebase you will find we are on 3.13 now.
… Wasm. Signed-off-by: Jérôme Vouillon <jerome.vouillon@gmail.com>
Signed-off-by: Jérôme Vouillon <jerome.vouillon@gmail.com>
Signed-off-by: Jérôme Vouillon <jerome.vouillon@gmail.com>
| Some targets -> List.length (Js_of_ocaml.Target.Set.to_list targets) > 1 | ||
| None -> false | ||
in | ||
if multiple_targets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand the logic here, why do we have Js_of_ocaml.Ext.exe when targets is wasm only ?
This kind of pattern seems fragile/ error prone if with ever add a new target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to be able to simply switch between producing a JavaScript file or a Wasm binary by using a dune profile. Since we are using the JavaScript file names in dune files or in HTML files, it seems natural to use the same JavaScript file name in both cases.
Since Wasm is not widely available, one also needs to be able to generate both a Javascript file and a Wasm binary, so that a web server can send one or the other based on the browser. In this case, we need two different file names.
Do you see a better way to deal with both of these use cases?
@@ -91,6 +93,7 @@ module In_buildable = struct | |||
type t = | |||
{ flags : Ordered_set_lang.Unexpanded.t Flags.t | |||
; javascript_files : string list | |||
; wasm_files : string list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to have a comment explaining why wasm_of_ocaml reuses the (js_of_ocaml ..)
field instead of being its own thing
I found out that adding |
Superseded by #10695 |
No description provided.