-
Notifications
You must be signed in to change notification settings - Fork 20
Build WebAssembly tests for the wasm32-wasip1 target and run them using wasmtime
#122
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
Conversation
| uses: dtolnay/rust-toolchain@master | ||
| with: | ||
| toolchain: ${{ env.RUST_STABLE_VER }} | ||
| targets: wasm32-unknown-unknown |
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.
First, thank you for the awesome PR! This comment can be treated as non-blocking – but I am just curious and honestly lacking knowledge of other wasm32 targets.
We (Canva) currently use the vello_hybrid / vello_cpu renderers via wasm32-unknown-unknown using wasm-bindgen directly. From that perspective I'd be concerned that we lose that test coverage.
Is there a solution for migrating from wasm32-unknown-unknown to wasm32-wasip1 for Rust on the browser?
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 don't think so at the moment; there's wasm-bindgen/wasm-bindgen#3421 but it's kinda dead (as is wasm-bindgen in general, unfortunately).
The Vello tests will still run on wasm32-unknown-unknown, so test coverage should be maintained through those. I'm also not removing wasm32-unknown-unknown support from this library; this is just a matter of making the tests easier to run and extend.
It might be worth adding a CI check that wasm32-unknown-unknown still builds, but I seriously doubt there'll be any functional differences between wasm32-wasip1 and wasm32-unknown-unknown. For example, there are a lot of JavaScript packages on NPM that run all their unit tests on Node, but that doesn't mean those packages don't work in the browser.
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.
That sounds good to me! Thank you!
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've added an additional step to the "cargo test (wasm32)" workflow that runs cargo build --target wasm32-unknown-unknown. This should ensure that target still builds.
DJMcNab
left a comment
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've not followed these steps locally, but this looks much less fragile to me. There is value in this stuff being tested in the browser, to ensure that we're aware (and ideally can report it upstream) if we're exercising code paths which some browser doesn't support or miscompiles. However, a WASI emulation layer would be a much nicer path towards that than requiring a JavaScript layer, and that isn't exactly high-priority.
| // Note that we cannot feature-gate this with `target_arch`. If we run | ||
| // `wasm-pack test --headless --chrome`, then the `target_arch` will still be set to | ||
| // `cargo test --target wasm32-wasip1`, then the `target_arch` will still be set to | ||
| // the operating system you are running on. Because of this, we instead add the `target_arch` | ||
| // feature gate to the actual test. | ||
| let include_wasm = !exclude_wasm(&input_fn_name.to_string()); |
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.
It's worth checking whether we can use CARGO_CFG_TARGET_ARCH (ideally for all of these tests, because in theory we could be cross-compiling to run through miri or qemu).
This should be a follow-up/an issue though.
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.
We can't; it's not exposed at the time proc macros are run.
Ralith
left a comment
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 don't have much stake in wasm, but this seems like nice cleanup!
|
I'll go ahead and merge this. If we want to do something as sophisticated as checking for miscompilations in each browser engine, it'd probably be helpful to have complex integration tests instead of just the unit tests we have here. All of that can be deferred to the future. |
This was discussed a bit in #122. We were previously omitting architecture-specific tests based on the *host* architecture, which is incorrect and prevents cross-compiling tests to run under something like qemu. We can't access `CARGO_CFG_TARGET_ARCH` at the time that proc macros are evaluated, so we just have to build every architecture-specific test and use `#[cfg]` to disable ones that aren't applicable to the current architecture. Even more annoyingly, there's no way to conditionally ignore a test at runtime. This means that if the CPU we're running the tests on doesn't support a target feature, we have to either pass or fail that test. I've chosen to fail, to prevent faulty code from appearing to pass tests. I've also updated the architecture-specific `exclude` functions to add `#[ignore]` attributes rather than omitting the tests entirely. This makes it more clear that the tests exist, but are not being run because they don't yet pass.
WASI is now mature enough that we can build binaries that target WebAssembly, and they will "just work" if run in a WebAssembly runtime.
This means we can get rid of the increasingly-unmaintained
wasm-packandwasm-bindgenmachinery, and run tests the same way we do for any other target, including doctests.We could add a
.cargo/config.tomlto this repository that specifies the rustflags and runner, but that would affect every package and cargo command. Not sure if that's desirable.Note that some of the doctests for
wasm32-wasip1fail to compile, probably due to #105:Error messages
I've
ignored those doctests for now.