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

Small changes in the internal APIs for Wasm #1654

Merged
merged 5 commits into from
Aug 25, 2024

Conversation

OlivierNicole
Copy link
Contributor

This PR consists in a number of changes in the APIs of compiler/lib/ that @vouillon needed while developing wasm_of_ocaml.

This is part of a series of PRs intending to reduce the diff between js_of_ocaml and wasm_of_ocaml (see ocaml-wasm/wasm_of_ocaml#47).

It is best reviewed commit by commit.

compiler/lib/source_map.ml Outdated Show resolved Hide resolved
compiler/lib/source_map.mli Outdated Show resolved Hide resolved
compiler/lib/unit_info.mli Outdated Show resolved Hide resolved
@hhugo
Copy link
Member

hhugo commented Jul 30, 2024

@OlivierNicole, I wonder if we could move faster by not grouping unrelated commits too much.
In this PR, some commits are trivial and could be merged very quickly, while some other will need much more discussion.

@OlivierNicole
Copy link
Contributor Author

Sure, I was worried of the opposite effect of bombarding the repo with too many PRs, hence the grouping. Let me know what you think should be separate PRs.

@hhugo
Copy link
Member

hhugo commented Jul 30, 2024

Sure, I was worried of the opposite effect of bombarding the repo with too many PRs, hence the grouping. Let me know what you think should be separate PRs.

I think you should at least move out the sourcemap api commit and the build info in sexp one

@OlivierNicole OlivierNicole force-pushed the converge-jsoo-tip-06 branch from 2d5bc4a to 2f22ec5 Compare July 30, 2024 14:53
@OlivierNicole
Copy link
Contributor Author

Done. In fact I think I won’t do a PR for the Source_map changes for now, hopefully I can make progress on #1617 soon enough that it won’t be needed for long.

compiler/lib/stdlib.ml Outdated Show resolved Hide resolved
@hhugo
Copy link
Member

hhugo commented Aug 1, 2024

The CI is not happy. @vouillon mentioned elsewhere that it could be due to a missing Generate.init () possibly in js_of_ocaml_dynlink.ml

@OlivierNicole OlivierNicole force-pushed the converge-jsoo-tip-06 branch 2 times, most recently from 361c0fa to 460cdf4 Compare August 1, 2024 14:34
@OlivierNicole
Copy link
Contributor Author

I still get this error:

 --- a/_build/default/compiler/tests-toplevel/test_toplevel.reference
+++ b/_build/default/compiler/tests-toplevel/test_toplevel.referencebcjs
@@ -4,4 +4,7 @@ Line 4, characters 2-4:
 Error: Syntax error
 Line 5, characters 0-16:
 Error: Unbound module "Missing_module"
-val y : float = 0.333333333333333315
+Dynlink: looking for symbol caml_float_of_int
+Dynlink: looking for symbol caml_float_of_int
+Line 1:
+Error: The external function "caml_float_of_int" is not available

in spite of the Linker.reset () that I added in js_of_ocaml_compiler_dynlink.ml. I’m not sure why.

@hhugo
Copy link
Member

hhugo commented Aug 2, 2024

I still get this error:

 --- a/_build/default/compiler/tests-toplevel/test_toplevel.reference
+++ b/_build/default/compiler/tests-toplevel/test_toplevel.referencebcjs
@@ -4,4 +4,7 @@ Line 4, characters 2-4:
 Error: Syntax error
 Line 5, characters 0-16:
 Error: Unbound module "Missing_module"
-val y : float = 0.333333333333333315
+Dynlink: looking for symbol caml_float_of_int
+Dynlink: looking for symbol caml_float_of_int
+Line 1:
+Error: The external function "caml_float_of_int" is not available

in spite of the Linker.reset () that I added in js_of_ocaml_compiler_dynlink.ml. I’m not sure why.

You need to initialize the thing in compiler/bin-js_of_ocaml/link.ml inside f.

I think I'd be more confortable moving the explicit initialization to its own PR so that we can discuss alternative and correctness without holding on the rest of this PR.

@OlivierNicole
Copy link
Contributor Author

Done. I have opened #1662 for explicit initialization.

@hhugo hhugo force-pushed the converge-jsoo-tip-06 branch from 99e82e0 to a9c69e3 Compare August 25, 2024 11:25
@hhugo hhugo merged commit 2f98d17 into ocsigen:master Aug 25, 2024
17 of 18 checks passed
@OlivierNicole OlivierNicole deleted the converge-jsoo-tip-06 branch August 26, 2024 10:44
@OlivierNicole
Copy link
Contributor Author

Thanks!

@hhugo hhugo added the wasm label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants