-
Notifications
You must be signed in to change notification settings - Fork 4
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 tests and CI job #29
Conversation
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.
Thanks a lot for this PR! Your analysis is correct, that is the current scenario. This PR also fixes #21
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
=======================================
Coverage 66.52% 66.52%
=======================================
Files 4 4
Lines 923 923
=======================================
Hits 614 614
Misses 309 309 ☔ View full report in Codecov by Sentry. |
Since this is a core dependency of rav1e, I'd like to have some feedback from the rav1e folks on this. Not sure who to ask though. @lu-zero maybe? |
@@ -9,7 +9,6 @@ repository = "https://github.com/rust-av/v_frame" | |||
|
|||
[features] | |||
serialize = ["serde", "aligned-vec/serde"] | |||
wasm = ["wasm-bindgen"] |
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.
Might be a good idea to leave it as no-op to stay compatible with rav1e
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 mind, but we have some breaking changes in the next release anyways, and it might be a good time to remove this immediately?
If you prefer keeping a wasm feature, I'll do it no problem
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.
Let's land as-is then.
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 looks fine beside that nit
The way I understand it, there are 2 different "main" WASM configurations:
wasm32-unknown-unknown
which is the most basic WebAssembly configuration that is used by browsers, andwasm32-wasi
, a newer extension to wasm32 which comes with a (small) standard library and some sort of component model.These two targets have different limitations and very different tooling in Rust. I'll try to give an overview:
#[test]
#[wasm_bindgen_test]
#[test]
target_arch != "wasm32"
target_arch = "wasm32"
,target_os = "unknown"
target_arch = "wasm32"
,target_os = "wasi"
4That said, this PR implements a CI job and testing for
wasm32-unknown-unknown
in a way that should allow addingwasm32-wasi
without breakage later.wasm
feature (now detected automatically)criterion
dev-dependency into wasm and non-wasmwasm32-unknown-unknown
. Including these onwasm32-wasi
seems to cause problems#[wasm_bindgen_test]
attribute on all tests ifwasm32-unknown-unknown
is selectedFootnotes
rayon doesn't work on wasm32, at least not out of the box. In criterion this means
default-features = false
↩Either
cargo test --no-run
&wasmtime <test binary>
(or wasmer etc.) orcargo test
with a test runner set in config.toml ↩The goal of WASI is to be "run anywhere", but browsers don't support it yet. https://webassembly.sh/ is one option to run WASI in a browser. ↩
target_os = "wasi"
is probably good enough, but for completeness, target_arch should be included too. ↩