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

ObjectStore WASM32 Support #7226

Merged
merged 3 commits into from
Mar 3, 2025
Merged

ObjectStore WASM32 Support #7226

merged 3 commits into from
Mar 3, 2025

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Mar 3, 2025

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

@github-actions github-actions bot added the object-store Object Store Interface label Mar 3, 2025
- 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉

@alamb
Copy link
Contributor

alamb commented Mar 3, 2025

I will review this shortyl

Copy link
Contributor

@alamb alamb left a 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 cfgs 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"
Copy link
Contributor

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

Copy link
Contributor Author

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

@crepererum
Copy link
Contributor

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.

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.

@kylebarron
Copy link
Contributor

Does this compile on wasm32-unknown-unknown as well, or just wasm32-wasip1?

cc'ing also @H-Plus-Time who made an ObjectStore implementation for wasm32-unknown-unknown here: https://github.com/H-Plus-Time/object-store-wasm

@tustvold
Copy link
Contributor Author

tustvold commented Mar 3, 2025

Does this compile on wasm32-unknown-unknown as well, or just wasm32-wasip1?

It should do but requires some shenanigans with getrandom - https://docs.rs/getrandom/latest/getrandom/#webassembly-support

So why are we bothering w/ wasip1 at all?

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.

@crepererum
Copy link
Contributor

My understanding is wasip2 is not properly standardised yet

They are BOTH standardized equally: as previews. See https://wasi.dev/

@tustvold
Copy link
Contributor Author

tustvold commented Mar 3, 2025

My understanding is wasip2 is not properly standardised yet

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...

@crepererum
Copy link
Contributor

My understanding is wasip2 is not properly standardised yet

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:

  1. do we need random numbers at all?
  2. if we need any side effects, then I think we should settle on WASIp2.

@tustvold
Copy link
Contributor Author

tustvold commented Mar 3, 2025

The CI specifically cares about WASIp1, not about p2.

This is fair, although this is simply because it predates wasmp2 existing, rather than any particular design decision.

do we need random numbers at all?

Yes for retries and crypto

I think we should settle on WASIp2.

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.

@tustvold tustvold merged commit 3674073 into apache:main Mar 3, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ObjectStore WASM32 Support
4 participants