nagisa / rust_libloading

Bindings around the platform's dynamic library loading primitives with greatly improved memory safety.
https://docs.rs/libloading
ISC License
1.24k stars 102 forks source link

Replace winapi with windows-sys #120

Closed vthib closed 1 year ago

vthib commented 1 year ago

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:

Closes #118

vthib commented 1 year ago

@nagisa can you take another look?

nagisa commented 1 year ago

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 commented 1 year ago

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

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

nagisa commented 1 year ago

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 commented 1 year ago

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

nagisa commented 1 year ago

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 commented 1 year ago

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

nagisa commented 1 year ago

Thanks!

MarijnS95 commented 1 year ago

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.

https://github.com/microsoft/windows-rs/issues/1720#issuecomment-1497497708

vthib commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

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

nagisa commented 1 year ago

Thanks for drawing my attention to this. I'm personally quite partial to making lovloading dependency-free given it's half-purpose of being kinda an extremely narrowly-focused crate. This is what I've done with Unix targets, and I'd love to see something like this for windows too.

riverar commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.