-
Notifications
You must be signed in to change notification settings - Fork 253
remove CallbackCode::Poll
#1471
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 CallbackCode::Poll
#1471
Conversation
|
|
|
Can you look into the CI failure before merging? Looks like the C generator needs to be adjusted since some behavior changed, and it also looks like there's a panic in Wasmtime itself |
I went ahead and updated the C and Go generators, and the tests are green when I run them locally using the branch from bytecodealliance/wasmtime#12182. I suspect we'll need to get that merged before CI is green here. |
As of WebAssembly/component-model#578, that code no longer exists. Instead, we can emulate it via a combination of `CallbackCode::Yield` and `waitable-set.poll`. I'm working on a PR to make the corresponding changes to Wasmtime, and needed this to make the tests pass.
See my earlier commit updating the Rust generator for details.
621a4ee to
22e66c7
Compare
|
@dicej for this: that's pretty bad in that it's a build of Wasmtime today which will panic with a given input. Is that fixed by your PR to wasmtime, caused recently, and/or worth investigation? It's a bad thing that the fuzzer I wrote isn't uncovering this since it means that it's not exploring the space well enough |
|
I'll look into it. |
|
That panic was fixed by bytecodealliance/wasmtime#12190 and of the three remaining failures: cancel-import DetailsDetailsI think these are related to reentrance checks changing? Maybe the tests are invalid now? ping-pong Details``` ------ Failure: ping-pong -------- runner: tests/runtime-async/async/ping-pong/runner.c compiled runner: /home/runner/work/wit-bindgen/wit-bindgen/target/artifacts/ping-pong/runner-c.wasm test: tests/runtime-async/async/ping-pong/test.rs compiled test: /home/runner/work/wit-bindgen/wit-bindgen/target/artifacts/ping-pong/test-rust.wasm error: failed to run `ping-pong`Caused by: |
|
The remaining failures not addressed by bytecodealliance/wasmtime#12190 are fixed by bytecodealliance/wasmtime#12182, i.e. the tests all pass when using that branch of Wasmtime, where |
|
Would it be possible to update the tests to pass here, with wasmtime-as-is, and then once the wasmtime changes land re-update the tests here? |
Sure. Would it be acceptable for me to just outright remove the offending test files and then add them back later? |
|
I'd prefer to keep them if it's not too much work. For ping-pong what I'd expect is minor adjustments to the expectations on the C side of things, but for cancel-import I'm not actually sure what to expect and/or how removing poll is going to fix/affect those tests |
|
Talked with Joel a bit offline, we'll merge with failing tests for now and get it working again once Wasmtime changes are merged |
d4f0885
As of WebAssembly/component-model#578, that code no longer exists. Instead, we can emulate it via a combination of
CallbackCode::Yieldandwaitable-set.poll.I'm working on a PR to make the corresponding changes to Wasmtime, and needed this to make the tests pass.