Skip to content

Update windows-rs dependency#726

Merged
est31 merged 1 commit intoRustAudio:masterfrom
paul-hansen:windows-deps
Nov 13, 2022
Merged

Update windows-rs dependency#726
est31 merged 1 commit intoRustAudio:masterfrom
paul-hansen:windows-deps

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