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

Disable winit default features #477

Merged
merged 8 commits into from Sep 6, 2021
Merged

Conversation

dzil123
Copy link
Contributor

@dzil123 dzil123 commented Apr 30, 2021

Allows users to set their own winit features.

Previously the winit default features would have been enabled even if the user disabled it in their own Cargo.toml. This is because of the way Cargo does feature unification.

This modification does not change any functionality because imgui-winit-support does not use winit::platform.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

Thanks, looks good assuming CI is green.

@thomcc
Copy link
Member

thomcc commented Apr 30, 2021

I don't think I can get this in a release easily soon, since there are a lot of breaking changes and we have a lot more planned (see #462). If you need it badly I could probably backport it and cut a patch release, but probably not until after the weekend.

@dzil123
Copy link
Contributor Author

dzil123 commented Apr 30, 2021

Thank you for the patch release offer, but this isn't urgent. I understand v0.8.0 is a big change.

@thomcc
Copy link
Member

thomcc commented Apr 30, 2021

Ah, it looks like it won't be fully backwards compatible anyway...

@dzil123
Copy link
Contributor Author

dzil123 commented Apr 30, 2021

The default features can be re-enabled only during CI with the --features winit-2x/default flag.

I haven't figured out how to do this for the --all-features clippy run.

@@ -46,7 +46,7 @@ jobs:
${{ runner.os }}-target-lint-
- run: cargo clippy --workspace --all-targets
# supported winit versions
- run: cargo clippy --manifest-path imgui-winit-support/Cargo.toml --all-features --all-targets
- run: cargo clippy --manifest-path imgui-winit-support/Cargo.toml --features winit-19 --features winit-20 --features winit-22 --features winit-23/default --features winit-24/default --all-targets
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, this pretty much sucks. I'd like to find a better way. Note that among other things, you can pass more than one feature at a time, but is there no way to make --all-features work?

Also, please reflect any changes you make here to https://github.com/imgui-rs/imgui-rs/blob/master/xtask/src/main.rs#L38-L61, which is used for local dev (it's not used for CI too so that we get more insight on which operation in particular failed)

@dzil123 dzil123 requested a review from thomcc May 7, 2021 06:43
@thomcc
Copy link
Member

thomcc commented May 23, 2021

This will need some updates after #485, sorry.

@dzil123
Copy link
Contributor Author

dzil123 commented May 24, 2021

How's that? I wasn't sure if I should add winit 0.25 to Github CI and xtask.

@sanbox-irl
Copy link
Member

hey @dzil123 looks like we lost the thread on this one here.

I think we're all good to go right? Thom's primary issue was losing -- all-features and you did a trick with a fake feature to take care of that. I think we just need a rebase and then it can merge?

@dzil123
Copy link
Contributor Author

dzil123 commented Sep 6, 2021

Thanks for checking in! Yeah, I think this is good to go.

I modified the default features list to include the winit default features as well, to avoid breaking projects that somehow depend on imgui-winit-support but not winit. So to opt out of winit default features in a consumer crate, default-features should be set to false for both imgui-winit-support and winit.

I also added winit 0.25 to Github CI and xtask as I said above.

One thing however is that cargo clippy/test for winit 0.19 is failing on imgui-rs:master, so Github may not allow this to be merged.

@sanbox-irl
Copy link
Member

Fixed the winit19 error, rebased this commit to it, and added to the changelog. Thanks so much for the work! This is an area that I am not well equipped, and our winit dependency specifically is a mess, so I appreciate you doing the leg work here. Thank you!

@sanbox-irl sanbox-irl merged commit a1456f3 into imgui-rs:master Sep 6, 2021
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