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

Remove --wasm2asm flag, use binaryen directly #787

Merged
merged 3 commits into from Sep 3, 2018

Conversation

Projects
None yet
2 participants
@bspeice
Contributor

bspeice commented Sep 3, 2018

Binaryen renamed the tool to wasm2js instead of wasm2asm - WebAssembly/binaryen#1642

Update wasm2es6js.rs
Binaryen renamed the tool to wasm2js instead of wasm2asm - WebAssembly/binaryen#1642

@bspeice bspeice changed the title from Update wasm2es6js.rs to Rename wasm2asm flag wasm2es6js.rs Sep 3, 2018

@bspeice

This comment has been minimized.

Contributor

bspeice commented Sep 3, 2018

The failing job is test-web-sys, and seems unrelated to what I'm proposing here. Should I be concerned about that?

@alexcrichton

This comment has been minimized.

Collaborator

alexcrichton commented Sep 3, 2018

Thanks for the PR! I think though that --wasm2asm is basically a deprecated mode now that wasm2js exists, as the output of wasm2js shoudl be equivalent to wasm2es6js --wasm2asm. If you use the wasm2js tool though is it not working for you?

@bspeice

This comment has been minimized.

Contributor

bspeice commented Sep 3, 2018

It's working fine, I just wanted to update things since this blog post recommended use of wasm2asm mode, and recent releases of binaryen broke that. Don't have a huge preference on rename vs. remove, but I was confused for a while as to why the command was breaking.

@alexcrichton

This comment has been minimized.

Collaborator

alexcrichton commented Sep 3, 2018

Ah ok, great! If we want to keep the --wasm2asm mode working here then I think we'll want to tweak this PR to just simply forward to wasm2js and don't worry about the rest of the output

@bspeice

This comment has been minimized.

Contributor

bspeice commented Sep 3, 2018

Not totally sure what you mean - are you referring to keeping the --wasm2asm flag live in wasm-bindgen and just changing the internal call to binaryen to use wasm2js?

@alexcrichton

This comment has been minimized.

Collaborator

alexcrichton commented Sep 3, 2018

Well I'm not entirely sure. It sounds like you want to keep the --wasm2asm flag, right? (to keep the blog post working?)

If so, then this implemention is incorrect because it will wrap ES imports twice, once by wasm2js and once by this tool.

Deleting --wasm2asm is fine by me, but otherwise keeping it working is also fine by me but will require a different patch.

@bspeice

This comment has been minimized.

Contributor

bspeice commented Sep 3, 2018

Ah, that makes more sense. I'm actually mostly in favor of deleting the flag; if it's just a shim around binaryen I'd rather reduce functionality and make a note in the docs that functionality provided by wasm2asm is now a part of wasm2js in binaryen. I'll get started on that unless I hear otherwise.

@alexcrichton

This comment has been minimized.

Collaborator

alexcrichton commented Sep 3, 2018

Sounds good to me!

@bspeice bspeice changed the title from Rename wasm2asm flag wasm2es6js.rs to [WIP] Remove --wasm2asm flag, use binaryen directly Sep 3, 2018

@alexcrichton alexcrichton merged commit a22094c into rustwasm:master Sep 3, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexcrichton

This comment has been minimized.

Collaborator

alexcrichton commented Sep 3, 2018

Thanks!

@bspeice bspeice deleted the bspeice:patch-1 branch Sep 3, 2018

@bspeice bspeice changed the title from [WIP] Remove --wasm2asm flag, use binaryen directly to Remove --wasm2asm flag, use binaryen directly Sep 3, 2018

Hywan added a commit to Hywan/gutenberg-parser-rs that referenced this pull request Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment