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

Replace winapi with windows-sys #120

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

vthib
Copy link
Contributor

@vthib vthib commented Mar 6, 2023

windows-sys is a crate with bindings to the windows API that is maintained by Microsoft. A lot of crates are migrating from winapi to it, and it would be nice for libloading to migrate as well.

The changes are quite straightforward:

  • the DWORD, WCHAR, etc types are not exposed by windows-sys, the standard types are used instead: u32, u16, etc.
  • HMODULE does not exist, HINSTANCE is used instead, and is a isize instead of a pointer.
  • FARPROC is a Option<unsafe extern "system" fn() -> isize>, which is a bit more annoying to use in debug prints: I used the {:#x} formatter instead of the {:p} one: before: ``, after: Symbol@0x7ffb29135bf0.

Closes #118

Copy link
Owner

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, some comments inline. Some of them apply more broadly than just to the line indicated, for example the one about using the type alias in load_with_flags applies just as well to the flag (re-)definitions as well.

src/os/windows/mod.rs Outdated Show resolved Hide resolved
src/os/windows/mod.rs Outdated Show resolved Hide resolved
src/os/windows/mod.rs Show resolved Hide resolved
src/os/windows/mod.rs Outdated Show resolved Hide resolved
src/os/windows/mod.rs Outdated Show resolved Hide resolved
@vthib vthib force-pushed the migration-winapi-windows-sys branch 2 times, most recently from 2581015 to debf372 Compare March 7, 2023 11:34
@vthib
Copy link
Contributor Author

vthib commented Mar 20, 2023

@nagisa can you take another look?

@nagisa
Copy link
Owner

nagisa commented Mar 21, 2023

Sorry for the delay, it looks like the rustdoc tests are failing. See CI results.

As for MSRV issues, we can bump that version at this time to something more reasonable. This would need modifications to the CI matrix in .github/workflows. What is the earliest rustc that's still supported by windows-sys?

@vthib
Copy link
Contributor Author

vthib commented Mar 21, 2023

I have fixed the rustdoc issues. There is however a clippy one, that stems from a windows metadata bug: microsoft/win32metadata#1505

This is quite unfortunate, we'll have to wait for a new windows_sys release before we can merge this MR.

@vthib vthib force-pushed the migration-winapi-windows-sys branch from debf372 to d2020b3 Compare March 21, 2023 14:25
@nagisa
Copy link
Owner

nagisa commented Mar 26, 2023

Looks like there is still a failure (and thus the release of windows-sys hasn't happened yet). Please ping me when the release occurs, thanks!

@vthib vthib force-pushed the migration-winapi-windows-sys branch from d2020b3 to 824276f Compare April 3, 2023 14:45
@vthib
Copy link
Contributor Author

vthib commented Apr 3, 2023

@nagisa windows-sys released the fix on 0.48, should be working fine now!

@nagisa
Copy link
Owner

nagisa commented Apr 4, 2023

Looks like there are some tests that are failing, but we will still need to raise the minimum supported Rust version checked by the CI.

vthib added 2 commits April 5, 2023 16:22
windows-sys has a MSRV of 1.48, which forces us to bump the MSRV
of this crate.
@vthib
Copy link
Contributor Author

vthib commented Apr 5, 2023

Indeed, sorry about that. I have fixed the issues, I did not handle the Deref impl on the Symbol object properly.

I also added a MSRV bump to 1.48, this should fix the MSRV pipeline as well. I think everything should be fine now

@vthib vthib force-pushed the migration-winapi-windows-sys branch from 824276f to a6a394a Compare April 5, 2023 14:24
@nagisa nagisa merged commit a6a394a into nagisa:master Apr 11, 2023
@nagisa
Copy link
Owner

nagisa commented Apr 11, 2023

Thanks!

@vthib vthib deleted the migration-winapi-windows-sys branch April 11, 2023 13:57
@MarijnS95
Copy link

Fwiw the status-quo is to use windows-bindgen nowadays when accessing only a minimal windows-sys subset, rather than pulling in that massive crate.

microsoft/windows-rs#1720 (comment)

@vthib
Copy link
Contributor Author

vthib commented Apr 11, 2023

Yes, discussion about this was ongoing while this PR was already in progress.

Migrating to windows-bindgen could be done, but it's more of a maintenance choice than a recommended change, both windows-sys and windows-bindgen are valid options. windows-sys is not that massive (the windows crate can however be) and the surface API of the crate won't change, so it's up to the maintainers of this crate to see which option they favor.

@MarijnS95
Copy link

Yes, discussion about this was ongoing while this PR was already in progress.

Curious where that's at, it's not in #118 nor the comments here.

windows-sys is not that massive

Clocking in at 2.63MB it is by far the largest crate in most of my dependency graphs, while only a microscopic portion of its API surface is used.

@vthib
Copy link
Contributor Author

vthib commented Apr 11, 2023

Curious where that's at, it's not in #118 nor the comments here.

I meant this discussion: microsoft/windows-rs#1720 which started again about two weeks ago.
I don't think the option of using windows-bindgen for libloading has been discussed.

@nagisa
Copy link
Owner

nagisa commented Apr 12, 2023 via email

@riverar
Copy link

riverar commented Apr 12, 2023

Fwiw the status-quo is to use windows-bindgen nowadays when accessing only a minimal windows-sys subset, rather than pulling in that massive crate.

It's a two week old feature with ~1 user and only designed for the most demanding of scenarios. I don't think you should be referring to this as the status quo (or at all).

@MarijnS95
Copy link

It looked as if some high-value crates were getting this conversion to windows-bindgen as their API usage has been extremely light to warrant pulling in the massive-in-comparison windows-sys crate, but those PRs have been closed now.

@ErichDonGubler
Copy link

Another interesting thing to note is that parking_lot has taken to using windows-targets directly, because they considered even the size of windows-sys intolerably high for their use case as a cornerstone crate of Rust's ecosystem.

I don't know what tolerances are here for code size of dependencies in this project vs. manual bindings work. Just examining this PR, it seems that there's a significantly higher number of APIs being used than parking-lot's 5 functions plus a dozen or so data types and constants.

To be clear: I'm not proposing any action items for the project. Just wanted to raise something that might be relevant.

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.

Idea: Switch to windows-sys
5 participants