-
Notifications
You must be signed in to change notification settings - Fork 747
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
Add a WebAssembly build to release #6351
Conversation
(context: #6334 (comment)) |
README.md
Outdated
## Releases | ||
|
||
Builds are distributed by the various toolchains that use Binaryen, like | ||
Emscripten, `wasm-pack`, etc. There are also official releases on GitHub in this |
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 you can drop in this repo itself
.
- name: update path | ||
run: echo "PATH=$PATH:$HOME/emsdk" >> $GITHUB_ENV | ||
|
||
# Configure with wasm EH and pthreads for maximal performance. |
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.
What is the min version of node needed to run the resulting code? Perhaps we should mention/document that somewhere? Perhaps the filename should binaryen-node18
(or something like that) to reflect the way it needs to be run?
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 did mention that in the README docs added in this PR. It is Node 18.
Interesting idea about putting it in the name... but I think that might seem odd, as it's 18+. That is, I worry someone might have node 22 and think they need another build.
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.
How about at least calling to -node
then.. since it can't run on anything else.
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.
Calling it just -wasm
is little misleading as it might suggest that all you need is a wasm engine to run it.
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.
Well, it should actually run on deno and bun as well - they are all working to support node APIs now. I didn't test them though. Also, this build is not actually done with ENVIRONMENT=node
, which means it can run on the web too.
I get what you're saying about "wasm" being inaccurate but I feel "node" is inaccurate as well. These are basically generic js+wasm builds, or as generic as I can make them. I feel "wasm" is the more forward looking name as they are 95% wasm, and may some day become 100%.
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.
But the intended audience for this build is purely command line folks who will need to use node to run it right?
If somebody wants to run binaryen on the web then binaryen.js is a much better choice, right?
I suggest we do build with -sENVIRONMENT=node
to make this intent clear (at least for now).
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.
Interesting. I hadn't thought of it that way.
I'm not sure ENVIRONMENT=node
helps though as this isn't an assertions build, so there won't be a nice clear error message in another environment. I'd also prefer not to limit experimentation here, as experimentation is really the goal - maybe we'll find uses in the browser. E.g. the binaryen.js
build uses the JS API, while this build uses the commandline API - maybe replicating commandline workflows would be simpler with it.
But the more I think on it the more I agree with your larger point about the name, so I renamed it to node
now and added some docs to clarify the purpose and status. What do you think?
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 get where you are coming from too. Perhaps we can change this in the future but I like the its explicit for now.
I added a passing test that verifies we can round-trip a |
Simply build wasm-opt with Emscripten and bundle that up. Example build: https://github.com/kripken/binaryen/releases/tag/wasm-build-1 Specifically binaryen-wasm-build-1-wasm.tar.gz Only 1.72 MB, as it's just wasm-opt and not any other tool, so it is much smaller than our other targets. Perhaps we will add more of the tools later as needed (wasm-metadce, wasm-split, etc.). Also update the readme regarding which toolchains use us as a library, that I noticed while editing it to add the release platforms.
Simply build
wasm-opt
with Emscripten and bundle that up.Example build:
https://github.com/kripken/binaryen/releases/tag/wasm-build-1
Specifically
binaryen-wasm-build-1-wasm.tar.gz
Only 1.72 MB, as it's just
wasm-opt
and not any other tool, so it ismuch smaller than our other targets. Perhaps we will add more of the
tools later as needed (
wasm-metadce
,wasm-split
, etc.).Also update the readme regarding which toolchains use us as a library, that I
noticed while editing it to add the release platforms.