Support wasm32-unknown-unknown architecture #458

gelendir · 2024-02-14T21:01:53Z

Description

Modifications needed to compile rspotify for a WebAssembly runtime. (i.e. cargo build --target wasm32-unknown-unknown)

Motivation and Context

I want to build an app that helps me better manage music when using spotify to DJ. I'd like to use rust + WASM to build it, however none of the maintained spotify crates support WASM.

@shiguma127 made an attempt in #293, but the PR was closed. This PR improves on what he started.

This is my most "complex" rust endeavor so far and I would be happy to get feedback on better ways of refactoring or rearchitecturing.

The code is now littered with a bunch of architecture conditionals that might be hard to maintain. I'm open to trying better alternatives if the maintainers don't like this.

One idea would be to make it easier to use the rspotify-http traits. One could implement the BaseHttpClient trait outside of this crate and then build a SpotifyClient<CustomHttpClient>. Feature flags would need to be refactored since this crate forces choosing a http client through the client-reqwest or client-ureq features.

Dependencies

wasm-pack for building and running tests
NodeJS for running tests

The other dependencies are managed through Cargo.toml

Type of change

New feature (non-breaking change which adds functionality)

How has this been tested?

The #[wasm_bindgen_test] attribute has been added to all tests. They all pass when running wasm-pack test --node. The attribute doesn't conflict with #[test] (but it does add an extra dev-dependency)

wasm-pack depends on a WASM runtime to run tests. The 3 currently supported are:

NodeJS
Chrome
Firefox

NodeJS is the default, so that's what's configured in the CI job

Client ID and Client secret

Some of the tests depend on environment variables RSPOTIFY_CLIENT_ID and RSPOTIFY_CLIENT_SECRET. However, WASM does not support the concept of environment variables. As a workaround the variables are imported at compile time only when running tests.

Is this change properly documented?

I wasn't sure if wasm support warrants documentation or not. I'm happy to add documentation as part of this PR

gelendir · 2024-02-14T21:03:15Z

rspotify-http/src/reqwest.rs

@@ -67,6 +70,17 @@ impl Default for ReqwestClient {
    }
 }

+#[cfg(target_arch = "wasm32")]
+impl Default for ReqwestClient {
+    fn default() -> Self {


reqwest doesn't implement the timeout function when built for wasm, hence the need for a separate implementation.

gelendir · 2024-02-14T21:06:07Z

src/clients/pagination/wasm_stream.rs

This stumped me the most. The async runtime in wasm doesn't support Send so it had to be removed from all trait bounds in stream.rs.

At first I tried to extract the trait bounds into "aliases" with the goal of annotating them with #[cfg...] but didn't find a way of making it work. There's trait_alias that might help, but it's available only on nightly.

gelendir · 2024-02-14T21:06:37Z

src/lib.rs

@@ -442,11 +442,13 @@ impl OAuth {
 }

 #[cfg(test)]
-mod test {
+pub mod test {


pub is needed so that wasm-pack can find the tests.

gelendir · 2024-02-14T21:07:24Z

tests/test_with_credential.rs

 };

 use maybe_async::maybe_async;

+#[cfg(target_arch="wasm32")]


Added to remove a warning on unused import when building for other architectures

gelendir · 2024-02-14T21:19:41Z

Cargo.toml

+tokio = { version = "1.11.0", features = ["rt-multi-thread", "macros"] }
+
+[target.'cfg(target_arch = "wasm32")'.dev-dependencies]
+tokio = { version = "1.11.0", features = ["rt", "macros"] }


Strictly speaking tokio isn't needed for running wasm tests, but to removing it would have made the #[cfg... attributes on tests more complex

gelendir · 2024-02-14T21:20:33Z

Cargo.toml

+
+[target.'cfg(target_arch = "wasm32")'.dev-dependencies]
+tokio = { version = "1.11.0", features = ["rt", "macros"] }
+dotenvy_macro = { version = "0.15.7" }


Used to read .env when running wasm tests

Does it meanenv:var in the standard library won't work in the wasm32 architecture?

Shouldn't we add a #[cfg...] flag for it, because it's wasm32 specific, right?

Correct, std::env::var won't work on wasm32-unknown-unknown.Wasm is designed to run in a web browser which is isolated from the host OS.

From my understanding of cargo the #[cfg...] on line 63 creates a block section, so it should already be imported only for wasm. It doesn't seem to get built when running cargo build --tests:

% cargo build --tests Compiling wasm-bindgen-backend v0.2.91 Compiling tokio v1.35.1 Compiling futures-util v0.3.30 Compiling serde v1.0.195 Compiling thiserror v1.0.56 Compiling strum v0.26.1 Compiling enum_dispatch v0.3.12 Compiling async-stream v0.3.5 Compiling wasm-bindgen-macro-support v0.2.91 Compiling wasm-bindgen-macro v0.2.91 Compiling wasm-bindgen v0.2.91 Compiling futures-executor v0.3.30 Compiling serde_json v1.0.111 Compiling serde_urlencoded v0.7.1 Compiling chrono v0.4.31 Compiling futures v0.3.30 Compiling js-sys v0.3.68 Compiling console_error_panic_hook v0.1.7 Compiling tokio-util v0.7.10 Compiling tokio-native-tls v0.3.1 Compiling tokio-socks v0.5.1 Compiling h2 v0.3.23 Compiling rspotify-model v0.12.0 (/Users/gregory/rust/rspotify/rspotify-model) Compiling wasm-bindgen-futures v0.4.41 Compiling wasm-bindgen-test v0.3.41 Compiling hyper v0.14.28 Compiling hyper-tls v0.5.0 Compiling reqwest v0.11.23 Compiling rspotify-http v0.12.0 (/Users/gregory/rust/rspotify/rspotify-http) Compiling rspotify v0.12.0 (/Users/gregory/rust/rspotify) Finished dev [unoptimized + debuginfo] target(s) in 6.82s ```

For comparison, here's the output with wasm:

% cargo build --tests --target wasm32-unknown-unknown Compiling proc-macro2 v1.0.76 Compiling unicode-ident v1.0.12 Compiling wasm-bindgen-shared v0.2.91 Compiling bumpalo v3.14.0 Compiling log v0.4.20 Compiling autocfg v1.1.0 Compiling wasm-bindgen v0.2.91 Compiling cfg-if v1.0.0 Compiling serde v1.0.195 Compiling version_check v0.9.4 Compiling pin-project-lite v0.2.13 Compiling typenum v1.17.0 Compiling futures-core v0.3.30 Compiling itoa v1.0.10 Compiling tinyvec_macros v0.1.1 Compiling futures-sink v0.3.30 Compiling tinyvec v1.6.0 Compiling syn v1.0.109 Compiling futures-channel v0.3.30 Compiling percent-encoding v2.3.1 Compiling futures-task v0.3.30 Compiling rustversion v1.0.14 Compiling pin-utils v0.1.0 Compiling memchr v2.7.1 Compiling serde_json v1.0.111 Compiling generic-array v0.14.7 Compiling ryu v1.0.16 Compiling slab v0.4.9 Compiling futures-io v0.3.30 Compiling num-traits v0.2.17 Compiling form_urlencoded v1.2.1 Compiling unicode-bidi v0.3.14 Compiling thiserror v1.0.56 Compiling fnv v1.0.7 Compiling bytes v1.5.0 Compiling quote v1.0.35 Compiling unicode-normalization v0.1.22 Compiling async-trait v0.1.77 Compiling heck v0.4.1 Compiling syn v2.0.48 Compiling http v0.2.11 Compiling tower-service v0.3.2 Compiling idna v0.5.0 Compiling base64 v0.21.7 Compiling env_filter v0.1.0 Compiling dotenvy v0.15.7 Compiling rspotify-macros v0.12.0 (/Users/gregory/rust/rspotify/rspotify-macros) Compiling scoped-tls v1.0.1 Compiling env_logger v0.11.1 Compiling url v2.5.0 Compiling block-buffer v0.10.4 Compiling crypto-common v0.1.6 Compiling digest v0.10.7 Compiling sha2 v0.10.8 Compiling wasm-bindgen-backend v0.2.91 Compiling maybe-async v0.2.7 Compiling dotenvy_macro v0.15.7 Compiling wasm-bindgen-macro-support v0.2.91 Compiling serde_derive v1.0.195 Compiling futures-macro v0.3.30 Compiling thiserror-impl v1.0.56 Compiling strum_macros v0.26.1 Compiling async-stream-impl v0.3.5 Compiling enum_dispatch v0.3.12 Compiling wasm-bindgen-test-macro v0.3.41 Compiling tokio-macros v2.2.0 Compiling async-stream v0.3.5 Compiling wasm-bindgen-macro v0.2.91 Compiling futures-util v0.3.30 Compiling tokio v1.35.1 Compiling strum v0.26.1 Compiling js-sys v0.3.68 Compiling console_error_panic_hook v0.1.7 Compiling futures-executor v0.3.30 Compiling futures v0.3.30 Compiling wasm-bindgen-futures v0.4.41 Compiling web-sys v0.3.67 Compiling getrandom v0.2.12 Compiling serde_urlencoded v0.7.1 Compiling chrono v0.4.31 Compiling wasm-bindgen-test v0.3.41 Compiling rspotify-model v0.12.0 (/Users/gregory/rust/rspotify/rspotify-model) Compiling reqwest v0.11.23 Compiling rspotify-http v0.12.0 (/Users/gregory/rust/rspotify/rspotify-http) Compiling rspotify v0.12.0 (/Users/gregory/rust/rspotify) Finished dev [unoptimized + debuginfo] target(s) in 9.77s

gelendir · 2024-02-14T21:21:10Z

rspotify-http/src/reqwest.rs

@@ -4,6 +4,8 @@
 use super::{BaseHttpClient, Form, Headers, Query};

 use std::convert::TryInto;
+
+#[cfg(not(target_arch = "wasm32"))]


added to remove a warning on unused imports

ramsayleung · 2024-02-27T18:31:04Z

tests/test_with_oauth.rs

+}
+
+#[cfg(target_arch = "wasm32")]
+fn get_access_tokens() -> (Option<String>, Option<String>) {


I'm not familiar with wasm32 architecture, does it mean wasm32 doesn't support env::var function, so we need another macro to obtain the environment variable?

Indeed, wasm32-unknown-unknown doesn't support accessing OS environment variables. As a work-around the tokens are imported at compile time. I extracted the code into a function to make it easier to manage the difference between architectures.

ramsayleung · 2024-02-27T19:45:22Z

Cargo.toml

+
+[target.'cfg(target_arch = "wasm32")'.dev-dependencies]
+tokio = { version = "1.11.0", features = ["rt", "macros"] }
+dotenvy_macro = { version = "0.15.7" }


Does it meanenv:var in the standard library won't work in the wasm32 architecture?

Shouldn't we add a #[cfg...] flag for it, because it's wasm32 specific, right?

ramsayleung · 2024-02-27T19:49:23Z

rspotify-http/src/reqwest.rs

@@ -109,7 +123,8 @@ impl ReqwestClient {
    }
 }

-#[async_impl]
+#[cfg_attr(target_arch = "wasm32", async_impl(?Send))]


I'm wondering is there any way to encapsulate these cfg_attr flags, because they are duplicated, and we might need to do the same thing for other architecture in the future.

I was wondering the same thing. I found in the rust docs that the #![cfg_attr] form applies to an entire file, but that doesn't help much here. Another idea would be to abstract this within a custom macro.

I've never written custom macros yet but could experiment if you think it's a good idea.

Custom macros is not an ideal idea for me, it will also increase complexity and mental effort to maintain the codebase.

ramsayleung · 2024-02-27T19:54:06Z

src/clients/pagination/wasm_stream.rs

@@ -0,0 +1,62 @@
+//! Asynchronous implementation of automatic pagination requests.


Why do we need a separate stream module for wasm32 architecture, is there anything the existing stream module is unable to satisfy?

Looks like my previous comment dissapeared, I explained it here: #458 (comment)

Basically I didn't find a way of encapsulating the Send trait bounds on the function signature behind a #[cfg...]

ramsayleung · 2024-02-27T19:56:49Z

src/lib.rs

@@ -456,6 +458,7 @@ mod test {
    }

    #[test]
+    #[wasm_bindgen_test]


A naive question, what does wasm_bindgen_test mean? Do we need this macro?

The regualar #[test] isn't compatible with wasm. Excerpt from the wasm-bindgen-test README:

The normal #[test] cannot be used and will not work. Eventually it's intended that the #[wasm_bindgen_test] attribute could gain arguments like "run in a browser" or something like a minimum Node version.

I believe the macro exports the tests into a separate WASM module to make it easier to run in a browser.

ramsayleung · 2024-02-28T02:10:20Z

Thanks for your contribution, everything looks good to me now, is it ready to merge?

Furthermore, are you planning to add a section in the CHANGELOG to illustrate the support for wasm?

gelendir · 2024-02-28T03:34:45Z

Good to hear ! Yes I can add some docs and notes to the changelog. It's getting late over here but will push the docs to this PR tomorrow ?

gelendir · 2024-02-28T19:40:46Z

Documentation and CHANGELOG have been added. I didn't figureout how you manage version increments, so I guessed that this would be consifered a new feature and added version 1.3.0.

If the docs look good then I don't have any other changes planned and you can merge.

ramsayleung · 2024-02-29T03:04:57Z

LGTM, but some CI tasks failed.

gelendir · 2024-02-29T05:25:05Z

It seems to be a transient error with the spotify API, the test received a HTTP 504. I just tried running the CI on my repo and everything passes: https://github.com/gelendir/rspotify/actions/runs/8091405577

Could you try re-running the CI ?

ramsayleung · 2024-02-29T06:56:24Z

Merged :)

Support wasm32-unknown-unknown architecture

2b23e5b

gelendir commented Feb 14, 2024

View reviewed changes

gelendir mentioned this pull request Feb 14, 2024

Support for wasm and Cloudflare Workers #292

Closed

gelendir commented Feb 14, 2024

View reviewed changes

gelendir added 2 commits February 19, 2024 21:50

code formatting

64be77d

fix clippy errors

70d5026

ramsayleung reviewed Feb 27, 2024

View reviewed changes

document wasm support

9bf39b4

gelendir requested a review from ramsayleung February 28, 2024 19:40

ramsayleung approved these changes Feb 29, 2024

View reviewed changes

ramsayleung merged commit 0849ff3 into ramsayleung:master Feb 29, 2024
6 checks passed

OliverEvans96 mentioned this pull request Sep 2, 2024

[WIP] Remove Send bounds on wasm32 open-telemetry/opentelemetry-rust#2074

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support wasm32-unknown-unknown architecture #458

Support wasm32-unknown-unknown architecture #458

gelendir commented Feb 14, 2024 •

edited

Loading

gelendir Feb 14, 2024

gelendir Feb 14, 2024 •

edited

Loading

gelendir Feb 14, 2024

gelendir Feb 14, 2024

gelendir Feb 14, 2024 •

edited

Loading

gelendir Feb 14, 2024

ramsayleung Feb 27, 2024

gelendir Feb 28, 2024

gelendir Feb 28, 2024

gelendir Feb 14, 2024

ramsayleung Feb 27, 2024

gelendir Feb 28, 2024

ramsayleung Feb 27, 2024

ramsayleung Feb 27, 2024

gelendir Feb 28, 2024

ramsayleung Feb 28, 2024

ramsayleung Feb 27, 2024

gelendir Feb 28, 2024

ramsayleung Feb 27, 2024

gelendir Feb 28, 2024

ramsayleung commented Feb 28, 2024

gelendir commented Feb 28, 2024

gelendir commented Feb 28, 2024

ramsayleung commented Feb 29, 2024

gelendir commented Feb 29, 2024

ramsayleung commented Feb 29, 2024

		@@ -0,0 +1,62 @@
		//! Asynchronous implementation of automatic pagination requests.

Support wasm32-unknown-unknown architecture #458

Support wasm32-unknown-unknown architecture #458

Conversation

gelendir commented Feb 14, 2024 • edited Loading

Description

Motivation and Context

Dependencies

Type of change

How has this been tested?

Client ID and Client secret

Is this change properly documented?

Choose a reason for hiding this comment

gelendir Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gelendir Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramsayleung commented Feb 28, 2024

gelendir commented Feb 28, 2024

gelendir commented Feb 28, 2024

ramsayleung commented Feb 29, 2024

gelendir commented Feb 29, 2024

ramsayleung commented Feb 29, 2024

gelendir commented Feb 14, 2024 •

edited

Loading

gelendir Feb 14, 2024 •

edited

Loading

gelendir Feb 14, 2024 •

edited

Loading