Skip to content

Optional web-specific deps for wasm32 #7565

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Optional web-specific deps for wasm32 #7565

wants to merge 3 commits into from

Conversation

raphaelhetzel
Copy link

@raphaelhetzel raphaelhetzel commented Apr 17, 2025 •
edited
Loading

Connections
Related to what lead to #7533, but idependent.

Description
In the current version, the target_plaform wasm32 (without target_os emscripten) depends on web-specific dependencies. However, wasm can be used outside of the web, and wgpu can be used on wasm runtimes without javascript bindings / the webgpu backend using a custom backend.

Testing
I haven't tested emscripten or webgl (There is an idependent error when building webgl).
There might be some issues people working with emscripten/webgl might be able to detect here.
I am also not sure whether this could be simplified (the not emscripten in wgpu/Cargo.toml might be unnecessary).

Squash or Rebase?

Squash

Checklist

  • [x ] Run cargo fmt.
  • Run taplo format.
  • [ x] Run cargo clippy --tests. If applicable, add:
    • [ x] --target wasm32-unknown-unknown (some independent warnings)
  • [X ] Run cargo xtask test to run tests. (one naga error I don't think is related to this)
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@raphaelhetzel raphaelhetzel requested a review from a team as a code owner April 17, 2025 12:51
@raphaelhetzel raphaelhetzel mentioned this pull request Apr 17, 2025
Closed
cwfitzgerald
cwfitzgerald requested changes Apr 17, 2025
View reviewed changes
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Looks good! Before we land, as dependencies are really tricky and easy to regress, could you add dependency tests for:

  • Ensuring that depending on wgpu on wasm with just the custom feature does not depend on wasm-bindgen et al.
  • Ensuring that turning on either the webgl or webgpu feature does add wasm-bindgen et al.
  • Ensuring turning on both ^ does not bring in wasm-bindgen on a native platform (lets say windows)

@cwfitzgerald cwfitzgerald self-assigned this Apr 17, 2025
cwfitzgerald
cwfitzgerald requested changes Apr 20, 2025
View reviewed changes
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

One issue, one question. Should we still have a web feature on wgpu as well to enable it with a custom backend running on the web? Might not be worth the bother.

tests/tests/wgpu-dependency/main.rs Outdated
@@ -121,7 +121,7 @@ fn wasm32_with_webgpu_and_wgsl_does_not_depend_on_naga() {
human_readable_name: "wasm32 with `webgpu` and `wgsl` feature does not depend on `naga`",
target: "wasm32-unknown-unknown",
packages: &["wgpu"],
features: &["webgpu", "wgsl"],
features: &["custom"],
Copy link
Member

Choose a reason for hiding this comment

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

This change seems errant?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, sorry for that, I must have slipped up while adding the tests below and did not notice it during the push.

@raphaelhetzel raphaelhetzel force-pushed the web_dependencies branch 2 times, most recently from 28d2146 to 2f4c041 Compare April 20, 2025 21:08
@raphaelhetzel
Copy link
Author

Regarding your question, I don't see what the web feature would enable.
There are features for all backends available on wasm: Custom does not require web-sys et. al., while webgpu and webgl require those dependencies. I have not added that test case, but there is no reason why all three can't be enabled at the same time.

We could add a web feature to semantically group the dependency enables instead of duplicating them for the two backends.

@cwfitzgerald
Copy link
Member

The case I was thinking of was: you want a custom backend that runs on web-enabled wasm and want the web-sys types in wgpu-types. I guess you can just enable wgpu-types/web at that point?

sagudev
sagudev reviewed Apr 21, 2025
View reviewed changes
wgpu/Cargo.toml Outdated
@@ -68,7 +72,14 @@ angle = ["wgpu-core?/angle"]
vulkan-portability = ["wgpu-core?/vulkan-portability"]

## Enables the GLES backend on WebAssembly only.
webgl = ["wgpu-core/webgl", "dep:wgpu-hal", "dep:smallvec"]
webgl = [
"wgpu-core/webgl",
Copy link
Collaborator

Choose a reason for hiding this comment

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

wgpu-core/webgl does enables wgpu-types/web, but I am wondering if would be worth being explicit about this:

Suggested change
"wgpu-core/webgl",
"wgpu-core/webgl",
"wgpu-types/web",

@raphaelhetzel
Copy link
Author

I've added a web feature now. That also covers @sagudev's comment.
I also added some additional enables and a check for the custom_backend example.

@raphaelhetzel
Copy link
Author

raphaelhetzel commented Apr 21, 2025 •
edited
Loading

I am not quite sure how I should go about fixing the tests failing due to the custom example.
The standalone examples depend on wgpu with the default parameters and therefore enable the web feature.
The CI runs however disable the default features and therefore disable the web feature for the custom example.

This would work if only the custom example ist built.

I guess I would either need to always depend on web for the custom_example or change the pipeline.
It should not have worked prior to todays commit but did by accident.

@sagudev
Copy link
Collaborator

sagudev commented Apr 21, 2025

Maybe just tweak CI invocations by adding web feature on wasm?

@sagudev
Copy link
Collaborator

sagudev commented Apr 21, 2025

You can take some inspiration from: https://github.com/gfx-rs/wgpu/pull/7407/files

@raphaelhetzel
Copy link
Author

Thanks for the hint. Wasn't quite sure whether that would be acceptable (essentially I could also just change that for that one example). But as I am not sure whether anyone else will actually use this, I did not want to add a separate matrix entry for wasm with and without web specifics.

@sagudev
Copy link
Collaborator

sagudev commented Apr 21, 2025

Thanks for the hint. Wasn't quite sure whether that would be acceptable (essentially I could also just change that for that one example). But as I am not sure whether anyone else will actually use this, I did not want to add a separate matrix entry for wasm with and without web specifics.

The root problem is #7314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sagudev sagudev sagudev left review comments

@cwfitzgerald cwfitzgerald Awaiting requested review from cwfitzgerald

Requested changes must be addressed to merge this pull request.

Assignees

@cwfitzgerald cwfitzgerald

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@raphaelhetzel @cwfitzgerald @sagudev