-
Notifications
You must be signed in to change notification settings - Fork 884
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
ObjectStore WASM32 Support #7226
Conversation
- name: Build wasm32-unknown-unknown | ||
run: cargo build --target wasm32-unknown-unknown | ||
- name: Build wasm32-wasip1 | ||
run: cargo build --target wasm32-wasip1 | ||
run: cargo build --all-features --target wasm32-wasip1 |
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 will review this shortyl |
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.
Makes sense to me -- thank you @tustvold
It is somewhat unfortunate that we need to sprinkle cfg
s around, but I don't see any way around this and I think the new CI checks should prevent regressions
#[cfg(not(target_arch = "wasm32"))]
match custom { | ||
Some(x) => Ok(x), | ||
None => Err(crate::Error::NotSupported { | ||
source: "WASM32 architectures must provide an HTTPConnector" |
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.
At some point it would be great to have an example of implementing WASM / a documentation guide for how to do it
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 think it would be good to even go as far as providing first-party support, but I don't have time to figure out the complexities involved.
#7227 is my attempt to fish for contributions
So why are we bothering w/ wasip1 at all? It's outdated (not just the APIs that we can use, but also how it works under the hood), it's unnecessarily limiting, and I would argue that if you wanna use WASI, at least you the 0.2 preview version. |
Does this compile on cc'ing also @H-Plus-Time who made an |
It should do but requires some shenanigans with getrandom - https://docs.rs/getrandom/latest/getrandom/#webassembly-support
My understanding is wasip2 is not properly standardised yet, and there are real platforms that support wasip1 and that people might want to target. That being said I am really just using wasip1 as it is a well-defined WASM32 target, see above for the challenges of wasm32-unknown-unknown. Ultimately this PR is to allow people to support wasm32 targets at all, it is then up to the user to provide an HttpConnector that exploits whatever APIs are available in their particular WASM32 flavour. |
They are BOTH standardized equally: as previews. See https://wasi.dev/ |
I'm not sure what you are suggesting here, are you saying we should not support wasip1, we should add a test showing we support wasip2 (which seems redundant given it is a superset), something else? This PR is not about supporting particular WASM32 flavours, it is about supporting WASM32 at all... |
The CI specifically cares about WASIp1, not about p2. And as you said: plain WASM32 is gonna be a pain, because you to get random numbers, it's a side effect that the host needs to support (either via JS or via WASI). So I think my questions are:
|
This is fair, although this is simply because it predates wasmp2 existing, rather than any particular design decision.
Yes for retries and crypto
At least at the time of writing wasip2 is explicitly labelled an experimental target here, whereas the same language is not used for wasip1. As such, and because wasip1 is closer to the browser target I suspect 90% or more of users are actually interested in, I think we should leave the CI unchanged. Although I don't feel strongly on this matter, my main goal with this PR is just to make WASM32 possible, I don't intend to personally work on wasm32 support beyond that. |
Which issue does this PR close?
Closes #6818.
Rationale for this change
This allows building object_store with all features enabled on wasm32-wasip1.
It is worth noting, providing an HttpConnector and by extension HttpClient is an exercise for the user, but I could see this changing - it just needs someone to go away and figure out how best to do this (#7227). I am also aware that wasm32-wasip2 adds proper socket support, and so there is the possibility it could use a more "normal" HTTP client eventually.
What changes are included in this PR?
Some feature gating to make it possible to enable the cloud features on WASM platforms.
Are there any user-facing changes?
FYI @kylebarron