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

Make wgpu-core users responsible for choosing back ends. #3254

Merged
merged 2 commits into from Dec 8, 2022

Conversation

jimblandy
Copy link
Member

@jimblandy jimblandy commented Dec 2, 2022

NOTE: This PR extends #3253.

Before this change, backend selection is done by target dependencies in
wgpu-core/Cargo.toml, giving wgpu-core users no way to override those
choices. (Firefox doesn't want the gles back end, for example.)

There doesn't seem to be any way to have a crate select backends based on target
architecture and OS that users of that crate can still override. The default
features can't be selected based on the target, for example. That implies that
we should do the selection as late in the dependency DAG as feasible. Having
wgpu (and wgpu-core's other dependents) choose backends seems good enough.

Draft, because there are still some issues around the renderdoc feature. But comments are welcome!

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories

Description
Describe what problem this is solving, and how it's solved.

Testing
Explain how this change is tested.

@jimblandy jimblandy marked this pull request as ready for review December 2, 2022 22:03
@jimblandy jimblandy force-pushed the backend-simplification branch 2 times, most recently from a085cbc to 6124f8e Compare December 2, 2022 22:14
@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2022

Codecov Report

Merging #3254 (db23fc9) into master (88acaf7) will decrease coverage by 0.62%.
The diff coverage is 64.10%.

@@            Coverage Diff             @@
##           master    #3254      +/-   ##
==========================================
- Coverage   65.60%   64.98%   -0.63%     
==========================================
  Files          82       82              
  Lines       39468    39474       +6     
==========================================
- Hits        25894    25652     -242     
- Misses      13574    13822     +248     
Impacted Files Coverage Δ
wgpu-core/src/device/mod.rs 66.64% <0.00%> (ø)
wgpu-core/src/lib.rs 96.55% <ø> (ø)
wgpu-hal/src/lib.rs 26.21% <ø> (ø)
wgpu-core/src/instance.rs 66.01% <55.55%> (+0.09%) ⬆️
wgpu-core/src/hub.rs 60.67% <90.90%> (ø)
wgpu-core/src/track/texture.rs 59.46% <0.00%> (-21.52%) ⬇️
wgpu-core/src/track/buffer.rs 84.86% <0.00%> (-7.28%) ⬇️
wgpu-hal/src/gles/mod.rs 61.53% <0.00%> (ø)
wgpu-hal/src/gles/adapter.rs 83.89% <0.00%> (+0.30%) ⬆️
wgpu-hal/src/gles/egl.rs 38.49% <0.00%> (+0.49%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@crowlKats crowlKats removed their request for review December 3, 2022 04:15
@jimblandy jimblandy force-pushed the backend-simplification branch 2 times, most recently from db23fc9 to 25f66c0 Compare December 6, 2022 18:47
Give `wgpu-core` a set of features parallel to those in `wgpu-hal`,
and pass them through. Within `wgpu-core`, use features to decide when
back-end-specific code is enabled. Move the platform-specific
dependencies that select appropriate backends as high as possible in
the dependency DAG: `wgpu` and `deno_webgpu`.

Since all the `gfx_select` macros now use configuration predicates of
the same form, simplify those macros and their uses.

Similarly, let `wgpu-core`'s users decide whether to support
renderdoc and emscripten, not `wgpu-core` itself.

In wgpu-hal give a more helpful error when no backends are enabled.

Don't select backends in CI that the current platform can't handle.

Before this change, backend selection is done by target dependencies
in `wgpu-core/Cargo.toml`, giving `wgpu-core` users no way to override
those choices. (Firefox doesn't want the GLES back end, for example.)

There doesn't seem to be any way to have a crate select backends based on target
architecture and OS that users of that crate can still override. The default
features can't be selected based on the target, for example. That implies that
we should do the selection as late in the dependency DAG as feasible. Having
`wgpu` (and `wgpu-core`'s other dependents) choose backends seems good
enough.
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!

I have ideas to build on this on how we can expand this to the users and have non-platform specific features.

@cwfitzgerald cwfitzgerald merged commit a50836e into gfx-rs:master Dec 8, 2022
@jimblandy jimblandy deleted the backend-simplification branch December 13, 2022 00:53
jimblandy added a commit that referenced this pull request Feb 7, 2023
When building for WebAssembly using the WebGPU backend, don't try to
build the entire workspace. wgpu-hal isn't meant to be used in this
case, and since #3254, it complains if no backends have been
explicitly selected.

Instead, pass `--package wgpu` when doing the WebGPU-backend build.
jimblandy added a commit to jimblandy/wgpu that referenced this pull request Feb 7, 2023
When building for WebAssembly using the WebGPU backend, don't try to
build the entire workspace. wgpu-hal isn't meant to be used in this
case, and since gfx-rs#3254, it complains if no backends have been
explicitly selected.

Instead, pass `--package wgpu` when doing the WebGPU-backend build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants