-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
22be781
[Emscripten port] Fix core count logic for Emscripten+pthreads
kripken 10c9263
yolo
kripken ef9a169
Merge remote-tracking branch 'myself/emcc.pthread' into wasm-build
kripken ec46361
undo
kripken edb63ba
cleanup
kripken 95f1e7a
docs
kripken a7f24b4
fix
kripken 368f1de
fix
kripken 40b4cea
fix
kripken 7a2a0b9
fix
kripken 15d6c78
feedback
kripken 746a0f3
try to add test
kripken b47ab69
fix
kripken 7d1961d
comment on node
kripken 2906625
rename wasm=>node
kripken File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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. thebinaryen.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.