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

Add wasm-bindgen #383

Merged
merged 3 commits into from
Mar 2, 2024
Merged

Add wasm-bindgen #383

merged 3 commits into from
Mar 2, 2024

Conversation

Ekleog
Copy link
Contributor

@Ekleog Ekleog commented Feb 23, 2024

Hey! I need to use wasm-bindgen-test-runner in CI, and thought I'd contribute back implementation of it.

Just to say, the wasm-bindgen-cli package actually includes other binaries (wasm-bindgen and wasm2es6js). I'm not a user of these packages and I don't know how useful they'd be in CI, so I didn't focus on them, but… it looks like in order to download multiple packages from a single archive, the user would have to download the archive one time for each binary, and we'd have to maintain the json file once for each binary?

So the main thing I'm unsure of is the naming of the tool. Should I have it be wasm-bindgen-test-runner, as there currently can be a single executable per tool? Or should we first support multiple executables per tool and then name it wasm-bindgen-cli, to match the way it can currently be installed via binstall? (I'll say that I probably won't have time to do the second option)

Also I'll take advantage to ask: I also wanted to integrate sqlx-cli, but AFAICT its releases do not have binaries published. Do I understand correctly that install-action is thus unable to help here?

@Ekleog Ekleog force-pushed the wasm-bindgen branch 7 times, most recently from 7880f30 to a9a6264 Compare February 23, 2024 20:34
@Ekleog Ekleog changed the title Add the wasm-bindgen-cli tool Add the wasm-bindgen-test-runner tool Feb 23, 2024
@Ekleog Ekleog marked this pull request as ready for review February 23, 2024 20:41
@Ekleog
Copy link
Contributor Author

Ekleog commented Feb 23, 2024

I think this is ready for review! :D There is just the tidy test that is failing, but it is not related to this PR and seems to be due to the way CI works. I tried simply moving main.sh to tools/, but then other things started failing. Not knowing whether that is actually the way you want to go, I just gave up on fixing CI and left this one red :)

main.sh Outdated Show resolved Hide resolved
tools/codegen/Cargo.toml Outdated Show resolved Hide resolved
@Ekleog
Copy link
Contributor Author

Ekleog commented Feb 25, 2024

Ooh I see, thank you for your answers!

I just removed the drive-by addition of the version field, if as you say codegen isn't supposed to work with older rustc versions and version-less manifests are not going away anyway :)

Now CI passed, please let me know if you think there are other changes to make to this PR!

The main question I have is still how to deal with the multiple-binaries-inside-a-single-crate situation: currently it's possible to install things via wasm-bindgen-cli which will proxy through binstall, but AFAICT I can't just add a manifest that'd install multiple binaries, so I can't just name the tool wasm-bindgen-cli :/

@Ekleog Ekleog requested review from NobodyXu and taiki-e February 26, 2024 06:21
NobodyXu
NobodyXu previously approved these changes Feb 26, 2024
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd let taiki-e to merge this, just in case I missed something.

@taiki-e
Copy link
Owner

taiki-e commented Feb 26, 2024

LGTM assuming we merge this with a single binary. However, I would like to do a little evaluation to see how difficult it would be to support multiple binaries before merging. (We have already done something more complicated for protoc but that requires some manually written code.)

@taiki-e taiki-e changed the title Add the wasm-bindgen-test-runner tool Add wasm-bindgen Mar 2, 2024
@taiki-e
Copy link
Owner

taiki-e commented Mar 2, 2024

Ok, I added a commit to support multiple binaries:

info: installing wasm-bindgen@latest
info: downloading https://github.com/rustwasm/wasm-bindgen/releases/download/0.2.91/wasm-bindgen-0.2.91-x86_64-unknown-linux-musl.tar.gz
info: verifying sha256 checksum for wasm-bindgen-0.2.91-x86_64-unknown-linux-musl.tar.gz
info: wasm-bindgen-test-runner installed at /home/runner/.cargo/bin/wasm-bindgen-test-runner
info: wasm-bindgen installed at /home/runner/.cargo/bin/wasm-bindgen
info: wasm2es6js installed at /home/runner/.cargo/bin/wasm2es6js
+ wasm-bindgen --version
wasm-bindgen 0.2.91 (fe8bc949b)

The bin field in base manifest is now:

"bin": [
"wasm-bindgen-${version}-${rust_target}/wasm-bindgen-test-runner${exe}",
"wasm-bindgen-${version}-${rust_target}/wasm-bindgen${exe}",
"wasm-bindgen-${version}-${rust_target}/wasm2es6js${exe}"
],

@taiki-e taiki-e merged commit 70fd2d4 into taiki-e:main Mar 2, 2024
26 checks passed
@taiki-e
Copy link
Owner

taiki-e commented Mar 2, 2024

Published in 2.28.0. Thanks for the PR!

@Ekleog
Copy link
Contributor Author

Ekleog commented Mar 18, 2024

Awesome, thank you for all the improvements you did to support this use case!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants