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

Get wasm_of_ocaml up-to-date with js_of_ocaml #47

Open
20 of 22 tasks
vouillon opened this issue Jul 3, 2024 · 12 comments
Open
20 of 22 tasks

Get wasm_of_ocaml up-to-date with js_of_ocaml #47

vouillon opened this issue Jul 3, 2024 · 12 comments
Assignees

Comments

@vouillon
Copy link
Collaborator

vouillon commented Jul 3, 2024

We have a branch jsoo-tip which contains the changes we make to the Js_of_ocaml code in compiler/lib, and a branch jsoo-merge which contains some changes to the compiler/lib/wasm code to adapt to API changes in Js_of_ocaml.

@OlivierNicole
Copy link
Collaborator

Currently after applying the process above, there are only two tests failing with OCaml 5.2.0:

  • compiler/tests-full/stdlib.cma.expected.js. This is not very surprising, as changes were made to it in both JSOO and WSOO. It seems to me that the simplest way to handle this would be to promote it after the actual merge;
  • compiler/tests-jsoo/test_marshal.ml. This is once again the compression problems that started with OCaml 5.1. I don’t have a good view of what happens here and it seems to depend on the machine, but it does not seem WSOO-specific.

@vouillon
Copy link
Collaborator Author

Regarding compiler/tests-full/stdlib.cma.expected.js, we should keep the version from jsoo-tip. I think that the file is not up to date in jsoo-tip and that the commit "Distinguish float arrays' should be amended to fix that.

By the way, the commit "Source_map_io API changes" should also be amended with these changes:

diff --git a/compiler/lib/source_map_io.unsupported.ml b/compiler/lib/source_map
_io.unsupported.ml
index dbdb35816c..2d4ecf1cc6 100644
--- a/compiler/lib/source_map_io.unsupported.ml
+++ b/compiler/lib/source_map_io.unsupported.ml
@@ -23,6 +23,8 @@ let to_string _ = fail ()
 
 let of_string _ = fail ()
 
-let to_file _ _ = fail ()
+let of_file_no_mappings _ = fail ()
+
+let to_file ?mappings:_ _ ~file:_ = fail ()
 
 let enabled = false

@OlivierNicole
Copy link
Collaborator

Regarding compiler/tests-full/stdlib.cma.expected.js, we should keep the version from jsoo-tip. I think that the file is not up to date in jsoo-tip and that the commit "Distinguish float arrays' should be amended to fix that.

Done, it works!

By the way, the commit "Source_map_io API changes" should also be amended with these changes:

diff --git a/compiler/lib/source_map_io.unsupported.ml b/compiler/lib/source_map
_io.unsupported.ml
index dbdb35816c..2d4ecf1cc6 100644
--- a/compiler/lib/source_map_io.unsupported.ml
+++ b/compiler/lib/source_map_io.unsupported.ml
@@ -23,6 +23,8 @@ let to_string _ = fail ()
 
 let of_string _ = fail ()
 
-let to_file _ _ = fail ()
+let of_file_no_mappings _ = fail ()
+
+let to_file ?mappings:_ _ ~file:_ = fail ()
 
 let enabled = false

Done.

@vouillon
Copy link
Collaborator Author

vouillon commented Jul 18, 2024

The marshaling test pass for me when producing JavaScript.

It fails with dune runtest --profile wasm (after implementing two missing primitives). I think that's because Marshal.header_size has changed in OCaml 5.1.0, so we need to adjust somehow the global $caml_marshal_header_size in runtime/wasm/marshal.wat depending on the OCaml version.

@OlivierNicole
Copy link
Collaborator

The marshaling test pass for me when producing JavaScript.

Not for me. This is completely obscure to me.

@OlivierNicole
Copy link
Collaborator

Status report: I rebased jsoo-tip onto JSOO master. Since then, test compiler/tests-toplevel/test_toplevel.js has been failing with --profile dev after merging jsoo-tip into jsoo-merge. I haven’t yet found the reason but maybe related to ocsigen/js_of_ocaml#1634?

I have also pushed missing primitives to jsoo-merge, as well as cherry-picked ocsigen/js_of_ocaml#1639. Primitives for the new BLAKE2B digests are still missing, as well as caml_zstd_initialize.

@hhugo
Copy link
Contributor

hhugo commented Jul 29, 2024

ocsigen/js_of_ocaml#1634 adds a new test that checks that toplevel built with separate compilation works correctly.
Is it possible that such setup has never worked with the wasm backend ?

@OlivierNicole
Copy link
Collaborator

Hm, what is strange is that it is test_toplevel.js (so, the whole-compilation one IIUC) that has an uncaught exception, and it is with JS and not Wasm.

@OlivierNicole
Copy link
Collaborator

The bulk of the changes needed on the Js_of_ocaml side has been submitted as PRs there, there shouldn’t be many more except to fix problems.

@vouillon
Copy link
Collaborator Author

vouillon commented Jul 31, 2024

Hm, what is strange is that it is test_toplevel.js (so, the whole-compilation one IIUC) that has an uncaught exception, and it is with JS and not Wasm.

There is this error:

Error: The external function "caml_float_of_int" is not available

So, it seems Generate.init is not called (it defines caml_float_of_int as an alias to %identity).

This issue is already in branch jsoo-tip, by the way.

@vouillon
Copy link
Collaborator Author

So, it seems Generate.init is not called (it defines caml_float_of_int as an alias to %identity).

We can add it in Driver.from_string or in lib-dynlink/js_of_ocaml_compiler_dynlink.ml. Not sure which is best...

Oh, and the merged file test_toplevel/dune seems to miss some changes. Maybe we need to cherry-pick 390e6a2 on branch jsoo-merge?

@hhugo
Copy link
Contributor

hhugo commented Jul 31, 2024

We can add it in Driver.from_string or in lib-dynlink/js_of_ocaml_compiler_dynlink.ml. Not sure which is best...

Maybe in Js_of_ocaml_compiler_dynlink.ml is best

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

No branches or pull requests

3 participants