chore: Add Cmake target for js_of_ocaml build target #4637
Conversation
src/js/binaryen.jsoo-extern-pre.js
Outdated
| @@ -0,0 +1,7 @@ | |||
| //Provides: binaryen | |||
| var binaryen = {}; | |||
There was a problem hiding this comment.
What are the differences between this and the main binaryen.js build? This line seems to be added, and also -s WASM=0 - anything else? (If just those two then maybe we can add a build flag with an ifdef for each of them, which would avoid duplication.)
Also, what does this line do? I'm not sure why it's needed in your build but not the default.
There was a problem hiding this comment.
WASM=0 is actually specified in the binaryen_js target, but even binaryen.js only uses the wasm version now.
A few things are going on with this line:
- If a
varmatchingEXPORT_NAMEdoesn't exist, the value is attached to the global, which doesn't work for js_of_ocaml - js_of_ocaml uses the
//Provides:syntax to treeshake values from the program
Other changes include:
ENVIRONMENTto onlywebandworkerbecause js_of_ocaml tries to get smart about node globals and it breaks the usage.- Compiling to ES5, which was changed previously.
EXPORT_ES6=0because we need plain ES5 javascript until js_of_ocaml finishes their ES6 supportNO_FILESYSTEM=1- since node is not supported, this ensures there is no filesystem related code in the file that might make js_of_ocaml think it is running in node.
I'm happy to use ifdef or any other solution you want, this was just the patch that I had to generate the file we need.
There was a problem hiding this comment.
I see, thanks. Ok, then I think a new build flag would be best. It sounds like these are specific to js_of_ocaml, so perhaps literally a new flag JS_OF_OCAML? Then we'd ifdef based on that on each of the bullet points you mention 1-2 and 1-4. That would avoid duplication and make it clear what the differences actually are (comments on each would be good as well).
There was a problem hiding this comment.
Thanks! Should be updated now (I also updated this branch to with main)
phated
force-pushed
the
phated/js-patch/version_106
branch
from
e5164e6 to
2f0bf1a
Compare
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
lgtm with a declaration of that new variable, something like
// (comment with more details?)
option(JS_OF_OCAML "Build binaryen.js for js_of_ocaml" OFF)I think that help text shows up in cmake's help for the project, or something like that.
|
Cool! Didn't know about that. TIL about |
2 participants
I lost my previous patch and needed to recreate it, and I'm submitting this upstream so I don't lose it again. We need to have a special build target for libbinaryen compiled for js_of_ocaml and our binaryen.ml bindings (also compiled with js_of_ocaml).
This shouldn't add any overhead to normal workflows, as you need to use emcmake with the specific target to run it and it would make my life a lot easier for generating everything in the Grain libraries. What do you think?