-
Notifications
You must be signed in to change notification settings - Fork 969
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 all backends controlled by features (using a very ugly workaround) #6949
base: trunk
Are you sure you want to change the base?
Conversation
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 this is broadly fine, I will have some comments on specifics once this is ready for review, but we definitely shouldn't be scared of crates. Might be worthwhile looking into cargo-release or release-plz or something if the count grows.
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.
quick comments from an outsider
"wgpu", | ||
"wgpu-core-target-feature-conditionals/arch-wasm32", | ||
"wgpu-core-target-feature-conditionals/native-non-apple", | ||
"wgpu-core-target-feature-conditionals/vendor-apple", |
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.
Could we consider shorter names for these, perhaps with slight reordering?
"wgpu", | |
"wgpu-core-target-feature-conditionals/arch-wasm32", | |
"wgpu-core-target-feature-conditionals/native-non-apple", | |
"wgpu-core-target-feature-conditionals/vendor-apple", | |
"wgpu-core-target-features/arch-wasm32", | |
"wgpu-core-target-features/native-non-apple", | |
"wgpu-core-target-features/vendor-apple", | |
"wgpu", |
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.
yeah those names are definitely too crazy right now
|
||
if cfg!(native) { | ||
if cfg!(metal) { | ||
backends = backends.union(Backends::METAL); | ||
} | ||
if cfg!(dx12) { | ||
backends = backends.union(Backends::DX12); | ||
} | ||
|
||
// Windows, Android, Linux currently always enable Vulkan and OpenGL. | ||
// See <https://github.com/gfx-rs/wgpu/issues/3514> | ||
if cfg!(target_os = "windows") || cfg!(unix) { | ||
backends = backends.union(Backends::VULKAN).union(Backends::GL); | ||
} | ||
|
||
// Vulkan on Mac/iOS is only available through vulkan-portability. | ||
if cfg!(target_vendor = "apple") && cfg!(feature = "vulkan-portability") { | ||
backends = backends.union(Backends::VULKAN); | ||
} | ||
|
||
// GL on Mac is only available through angle. | ||
if cfg!(target_os = "macos") && cfg!(feature = "angle") { | ||
backends = backends.union(Backends::GL); | ||
} | ||
} else { | ||
if cfg!(webgpu) { | ||
backends = backends.union(Backends::BROWSER_WEBGPU); | ||
} | ||
if cfg!(webgl) { | ||
backends = backends.union(Backends::GL); | ||
} |
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.
<3 always love for removing large blocks of ugly code :)
Connections
Description
Feature flag changes:
vulkan
feature for enabling Vulkan on Windows/Android/Linux (part of default feature set)gles
feature for enabling Vulkan on Windows/Android/Linux (part of default feature set, debatable if that's a good idea)webgl
enabling GL on all platformsThis is achieved by introducing three proxy crates. Each depends on wgpu-core under a different platform condition and will "forward" all feature flags to it.
Reminder why this horrible construct is the only way:
This is necessary because it's impossible today to drag in a dependency conditionally BOTH on a platform & a feature flag.
It is possible to do all these things in code, so we can forward features all the way to wgpu-hal and not have it compile e.g. it's vulkan backend on Mac iff
vulkan-portability
is off. But without this workaround it's impossible to avoid building vulkan associated dependencies anyways.For more details see
Building with
default-features=false
will now not even drag in wgpu-core!cargo tree -p wgpu --edges normal
for simple application depending on wgpu this way:Similarly, only depending on
dx12
(on Windows) doesn't includeash
orglow
(the former is a slow building dependency!):Testing
Checklist
cargo fmt
.taplo format
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
--target wasm32-unknown-emscripten
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.Draft todo: