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 winit truly optional #290

Merged
merged 1 commit into from
Aug 18, 2019
Merged

Make winit truly optional #290

merged 1 commit into from
Aug 18, 2019

Conversation

kvark
Copy link
Member

@kvark kvark commented Aug 16, 2019

Fixes #64

@kvark
Copy link
Member Author

kvark commented Aug 16, 2019

This looks straightforward (and @grovesNL is travelling)
bors r=travis

bors bot added a commit that referenced this pull request Aug 16, 2019
290: Make winit truly optional r=travis a=kvark

Fixes #64 

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 16, 2019

Build failed

@kvark
Copy link
Member Author

kvark commented Aug 17, 2019

Alright, this is nasty. So when we have a feature that enables "xxx/yyy", where "xxx" is optional, this automatically enables "xxx" out of a sudden. Anybody knows a workaround? @omni-viral hit this before in Amethyst, perhaps?

@zakarumych
Copy link

zakarumych commented Aug 17, 2019

I hit it multiple times. There is an issue in cargo (I'd insert link here, but can't find it). The issue was created long ago, but no one fixed it. Same as issue where dependencies that are filtered out by platform still enable features in other dependencies. We need a hero!

Intermediate solution is listing dependencies that can't be build on any platform under target.'cfg(...)'.dependencies. It will fix building. It would include more backends than necessary, but this should affect building times only, which is better than non-compilation anyway.

For gfx backends I write it like this:

[dependencies]
gfx-hal = "0.3"
gfx-backend-empty = { version = "0.3", optional = true }
gfx-backend-gl = { version = "0.3", optional = true }

[target.'cfg(all(target_os = "windows", not(target_arch = "wasm32")))'.dependencies]
gfx-backend-dx12 = { version = "0.3", optional = true }

[target.'cfg(all(any(target_os = "windows", all(unix, not(any(target_os = "macos", target_os = "ios")))), not(target_arch = "wasm32")))'.dependencies]
gfx-backend-vulkan = { version = "0.3", optional = true }

[target.'cfg(any(all(target_os = "macos", not(target_arch = "wasm32"), all(target_arch = "aarch64", target_os = "ios"))))'.dependencies]
gfx-backend-metal = { version = "0.3", optional = true }

And I have a macro that repeat those cfgs for code that uses backend specifics.

@kvark
Copy link
Member Author

kvark commented Aug 18, 2019

I decided to go a different route and just route the responsibility to the user of the library (which shouldn't be a problem for wgpu-rs, and possibly easy for others?..). Also filed #7259.

Copy link
Contributor

@seivan seivan left a comment

Choose a reason for hiding this comment

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

I suspect this is a typo.

wgpu-native/src/instance.rs Outdated Show resolved Hide resolved
@kvark kvark force-pushed the winit branch 2 times, most recently from 17f2c30 to 596cf2c Compare August 18, 2019 02:52
@kvark
Copy link
Member Author

kvark commented Aug 18, 2019

@seivan thank you for the quick review! Apparently, there was a bunch of code pieces using the old "window-winit" that are now rewritten.

@kvark
Copy link
Member Author

kvark commented Aug 18, 2019

Finally, CI appears to be happy.
bors r=seivan

bors bot added a commit that referenced this pull request Aug 18, 2019
290: Make winit truly optional r=seivan a=kvark

Fixes #64 

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 18, 2019

Build succeeded

@bors bors bot merged commit f30cb20 into gfx-rs:master Aug 18, 2019
@zakarumych
Copy link

@kvark This can be real problem because cargo enables features eagerly. User won't be able to enable features based on target platform and thus would end up reexport them same way transitively to the binary. And features would have to be enabled to build final binary.

@kvark kvark deleted the winit branch August 19, 2019 01:25
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.

Separate winit optional feature
3 participants