Skip to content

Make crate features no-op on incompatible targets #3466

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

Merged
merged 5 commits into from
Feb 21, 2023

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Feb 10, 2023

This changes crate features to be no-op on incompatible targets instead of not compiling.

  • Vulkan is no-op on WASM
  • DirectX12 and DirectX11 is no-op on everything but Windows
  • Metal is no-op on everything but MacOS and iOS

As far as I'm aware Vulkan is needed on MacOS for Vulkan Portability.
I saw a feature called portable_features that is used for testing purposes. I believe I can actually remove this now?

Depends on #3467.

Fixes #3463 (only WASM).
Fixes #3465.

@daxpedda
Copy link
Contributor Author

daxpedda commented Feb 10, 2023

@grovesNL if you could take a look and see if and how you are affected by this.
Any feedback is welcome!

@daxpedda
Copy link
Contributor Author

What exactly is the point of the emscripten crate feature? Couldn't it be completely replaced by target_os = "emscripten"?
This would also make it not necessary to activate the feature for downstream users.

@grovesNL
Copy link
Collaborator

Nice! I thought there was some reason we weren't able to set it up this way originally (maybe @kvark remembers?). Obviously it makes the cfgs slightly messier but it doesn't seem like a big deal since there shouldn't be very many of them anyway.

What exactly is the point of the emscripten crate feature? Couldn't it be completely replaced by target_os = "emscripten"?
This would also make it not necessary to activate the feature for downstream users.

That sounds right - if we don't need a separate feature to conditionally enable it then I think could be replaced by target_os = "emscripten"

@daxpedda
Copy link
Contributor Author

That sounds right - if we don't need a separate feature to conditionally enable it then I think could be replaced by target_os = "emscripten"

I will do this in a separate PR.

@teoxoy
Copy link
Member

teoxoy commented Feb 14, 2023

Fixes #3463 (only WASM).

Shouldn't it also fix metal?

I saw a feature called portable_features that is used for testing purposes. I believe I can actually remove this now?

I think so.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!

We seemed to be moving in this direction already, some cfgs had the target_os (ex windows for dx11/dx12) and also had some dependencies gated by target_os.

This is also more in line with the notion that feature flags should be additive and not cause compilation errors.

@teoxoy
Copy link
Member

teoxoy commented Feb 14, 2023

Could you remove portable_features in this PR as well?

@daxpedda
Copy link
Contributor Author

Fixes #3463 (only WASM).

Shouldn't it also fix metal?

As far as I am aware it will not, currently metal can't be built on Linux.

Could you remove portable_features in this PR as well?

Will do.

@daxpedda daxpedda force-pushed the target-dependent-features branch 2 times, most recently from 9010c76 to ba08bec Compare February 15, 2023 11:25
@daxpedda
Copy link
Contributor Author

I can't remove portable_features right now because I have to put in something instead in the CI, considering this PR I thought it would be appropriate to put in --all-features instead, but that still doesn't work because of emscripten, which should be fixed in #3467.

So either we remove portable_features in a follow-up PR, or I just make this PR depend on #3467.

I tested my assumption in #3488, looks like it works.

@daxpedda daxpedda force-pushed the target-dependent-features branch from ba08bec to 15af157 Compare February 15, 2023 11:40
@daxpedda daxpedda requested a review from teoxoy February 15, 2023 11:41
@daxpedda daxpedda mentioned this pull request Feb 15, 2023
@cwfitzgerald
Copy link
Member

Let's make the PRs dependent, emscripten should go in soon.

@daxpedda daxpedda force-pushed the target-dependent-features branch from 15af157 to b9e00ff Compare February 15, 2023 21:20
@daxpedda daxpedda requested a review from crowlKats as a code owner February 15, 2023 21:20
@daxpedda daxpedda marked this pull request as draft February 15, 2023 21:21
@daxpedda
Copy link
Contributor Author

Done!

@daxpedda daxpedda force-pushed the target-dependent-features branch 2 times, most recently from 8147fcd to bea7960 Compare February 15, 2023 21:51
@daxpedda daxpedda marked this pull request as ready for review February 15, 2023 21:51
@daxpedda
Copy link
Contributor Author

This is ready now!

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.

This is awesome to finally have!

Does this mean https://github.com/gfx-rs/wgpu/blob/master/wgpu/Cargo.toml#L106-L109 we can enable all features on all platforms?

@daxpedda
Copy link
Contributor Author

daxpedda commented Feb 15, 2023

Does this mean https://github.com/gfx-rs/wgpu/blob/master/wgpu/Cargo.toml#L106-L109 we can enable all features on all platforms?

Yes!
And as far as I checked the CI again now this is tested for native.

EDIT: I forgot, it's also tested for web as part of #3467. I remembered wrongly.

@daxpedda
Copy link
Contributor Author

I was just going through the CI again and I think we might want to test for --all-features a bit more.
Specifically web isn't tested with --all-features and wgpu-hal should maybe be tested separately just to make sure everything works as intended. Should I add this here or should I make a new PR?

@cwfitzgerald
Copy link
Member

Let's fix CI in this PR - it's all entangled

@daxpedda daxpedda force-pushed the target-dependent-features branch from ba0b500 to 645ecec Compare February 16, 2023 06:37
@daxpedda daxpedda requested review from cwfitzgerald and removed request for crowlKats and teoxoy February 16, 2023 06:46
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.

Some comments, but looking good

@daxpedda daxpedda force-pushed the target-dependent-features branch 3 times, most recently from bc1d0b1 to 44d11a0 Compare February 19, 2023 22:00
@daxpedda daxpedda force-pushed the target-dependent-features branch 3 times, most recently from bf3f7dc to e917f74 Compare February 20, 2023 10:13
@daxpedda daxpedda force-pushed the target-dependent-features branch from e917f74 to 7465b31 Compare February 21, 2023 12:03
@daxpedda
Copy link
Contributor Author

Just rebased on top of #3505 and #3511.

@teoxoy teoxoy dismissed cwfitzgerald’s stale review February 21, 2023 12:50

comments have been addressed

@teoxoy teoxoy merged commit 7430330 into gfx-rs:master Feb 21, 2023
jimblandy added a commit to jimblandy/wgpu that referenced this pull request Mar 10, 2023
jimblandy added a commit to jimblandy/wgpu that referenced this pull request Mar 10, 2023
jimblandy added a commit that referenced this pull request Mar 10, 2023
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.

Make wgpu-hal crate features target aware WASM and MacOS target not built on docs.rs
4 participants