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

Merge wasm_of_ocaml #1724

Open
wants to merge 531 commits into
base: master
Choose a base branch
from
Open

Merge wasm_of_ocaml #1724

wants to merge 531 commits into from

Conversation

OlivierNicole
Copy link
Contributor

@OlivierNicole OlivierNicole commented Oct 31, 2024

This aims to merge wasm_of_ocaml, currently hosted at https://github.com/ocaml-wasm/wasm_of_ocaml, back into this repo.

This is pending on Dune support ocaml/dune#11029. For now, Dune is pinned for the CI jobs.

vouillon and others added 30 commits July 3, 2024 10:48
Revert test_fun_call and test_poly_compare and don't run them in Wasm.
Add additional tests test_fun_call2 and test_poly_equal that make sense
in Wasm.
This does not make any difference with binaryen which conflated both,
but the spec mandates the former.
dune-build-info use a 64-byte placeholder. This ensures that such
strings are encoded as a sequence of bytes in the wasm module.
This reverts commit 25315fb4d3231ecd0fdc07c57adec59f3e697dc3.

No longer useful with WebAssembly/binaryen#6507
Effects: make continuation format compatible with OCaml 5.2
This stops a few tests from failing with Wasm
Lib: use number_t in Typed_array as well
Lib: use number_t in yet a few more places
Fix typo in `caml_ba_get_3` bounds check
Fix implementation of caml_register_named_value
@@ -0,0 +1,3 @@
#!/bin/sh
export PATH=$(echo $PATH | cut -d : -f 2-) # Do not call oneself recursively
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit fragile. There could be other directories added by dune. I'll commit an alternative.

in
let no_sourcemap =
let doc =
"Don't generate source map. All other source map related flags will be be ignored."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Don't generate source map. All other source map related flags will be be ignored."
"Don't generate source map. All other source map related flags will be ignored."

@OlivierNicole
Copy link
Contributor Author

I think we could make a release soon without this PR so that we have something ready for the ocaml 5.3 release. In parallel, we can finish the review for the wasm merge (we need to wait for a dune release anyway).

There are few points to clarify.

  • who's going to maintain the wasm backend ?

At Tarides we have no intention of letting it bitrot. In the foreseeable future I expect Jérôme to be the main maintainer, with me possibly doing small work here and there.

  • should wasm_of_ocaml be released at the same time as other jsoo packages.

Ideally, big packages that haven’t yet done a release with explicit float conversions should do so first, before wasm_of_ocaml is released. This is because after the release, the type Js.number_t will no longer be a synonym of float and so these packages would break. A number of Jane Street packages, in particular, are impacted.

  • is this repo becoming the main development repo ? Or should I expect other synchronisation PRs?

The intention is for this repo to become the development repo, yes.

@vouillon
Copy link
Member

vouillon commented Nov 8, 2024

I think it's going to be much simpler to use this repo as the main development repo and to release wasm_of_ocaml at the same time as other jsoo packages, given how tightly it depends on the js_of_ocaml compiler library.

@hhugo
Copy link
Member

hhugo commented Nov 8, 2024

Ideally, big packages that haven’t yet done a release with explicit float conversions should do so first, before wasm_of_ocaml is released. This is because after the release, the type Js.number_t will no longer be a synonym of float and so these packages would break. A number of Jane Street packages, in particular, are impacted.

I don't really understand why we would need to wait here ? Isn't it better to have wasmoo out first so that other packages can add support for their next releases. It would be better to get the correct constraint in the opam-repository anyway.

I want to release the current-ish state of master soon (with limited breakage). And then a major release with wasmoo. People not able to upgrade to the latest version of jsoo won't miss much features/changes in that case.

@OlivierNicole
Copy link
Contributor Author

I suppose this works, if all these packages have upper bounds on their version of js_of_ocaml.

@hhugo
Copy link
Member

hhugo commented Nov 23, 2024

One thing that would help A LOT would be identify all packages broken by this PR and add an upper bound on the js_of_ocaml version (< 6.0) in the opam-repo.
That would make releasing the next version much much easier.

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.