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

Update windows-rs dependency #726

Merged
merged 1 commit into from
Nov 13, 2022
Merged

Update windows-rs dependency #726

merged 1 commit into from
Nov 13, 2022

Conversation

paul-hansen
Copy link
Contributor

Description/motivation

It would be nice to get the windows crate dependency updated to the latest version to avoid duplicated dependencies like this one: https://github.com/bevyengine/bevy/actions/runs/3451394906/jobs/5760584454

This is my attempt to help with that. Feedback is welcome!

Code review notes

A lot of the changes were just changing null pointer arguments to None which should be straightforward.

There are a couple places I don't fully understand the consequences and just did what I needed to satisfy the type checker. If anyone more familiar with FFI and/or the Windows API can confirm it looks correct that would be much appreciated.
Especially around this section here with the pointer to a pointer, that was definitely something I don't deal with often: https://github.com/paul-hansen/cpal/blob/1c1c29ea1b627e433b5dea47343d0c63b4cc85a1/src/host/wasapi/device.rs#L225-L236

@est31
Copy link
Member

est31 commented Nov 13, 2022

Thanks, and it's shocking that the windows crate had this many breaking changes since we added it. There were three breaking changes in september alone. I am okay with this PR in particular, but in general, i do not want cpal to follow every single windows-rs release.

This release cadence seems to have been brought up to maintainers in microsoft/windows-rs#1720 but they have closed it as non actionable... maybe we should revert #669 and switch back to winapi?

@est31 est31 merged commit 7776c66 into RustAudio:master Nov 13, 2022
@est31
Copy link
Member

est31 commented Nov 13, 2022

Okay I've merged this, winapi seems to still be unmaintained, so we can't do much about this. I wonder if it would be possible to switch from windows-rs to windows-sys?

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.

2 participants