Adopt windows-sys

[clemenswasser]

Change from using the unofficial winapi bindings, to using the official windows bindings from Microsoft.

• Tested on all platforms changed
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• cargo doc builds successfully
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality
• Updated feature matrix, if new features were added or implemented

[Jasper-Bekkers]

My 2 cents, Microsoft is actively asking for feedback so some of this may be useful to share: microsoft/windows-rs#1285

Other 2 cents, this fork seems to work for our small use-case

[kennykerr]

Nice!

Please create new issues for any feedback: https://github.com/microsoft/windows-rs/issues

Very happy to clean up any warts you may find. Compile time will improve, as the win32 metadata namespaces are being refactored as we speak. That is a high priority for me.

If it helps, here's another reference port that may be interesting to compare: https://github.com/yoshuawuyts/miow/compare/kenny?expand=1

[clemenswasser]

@MarijnS95 I hope I could address all of your comments. Thanks for your review!

[MarijnS95]

@clemenswasser Thanks! I have a couple more review points ready to be submitted and haven't made it to the end yet, but overall this looks really good! Got some high-prio stuff to take care of first but I'll get back to it ASAP :)

[maroider]

I must admit that I'm still on the fence when it comes to adopting the windows crate. winapi's build times are already longer than I'd prefer, and I doubt windows-rs can match winapi's built times when it seemingly generates a whole bunch of convenience wrappers on top of the raw bindings.

That's not to say that I don't appreciate that you're taking the time to do this, it's just that I'm not quite sure the tradeoffs (as things currently stand) are worth it in the end.

[clemenswasser]

I must admit that I'm still on the fence when it comes to adopting the windows crate. winapi's build times are already longer than I'd prefer, and I doubt windows-rs can match winapi's built times when it seemingly generates a whole bunch of convenience wrappers on top of the raw bindings.

That's not to say that I don't appreciate that you're taking the time to do this, it's just that I'm not quite sure the tradeoffs (as things currently stand) are worth it in the end.

I would argue, winapi is currently unmaintained, so nothing on the compile times or other areas will significantly change. windows-rs on the other hand is a pretty young project with Microsoft behind it, compile times will get better (maybe even better than winapi, who knows?). What I want to say is that windows-rs already has many significant improvements today, with more to come.

[msiglreith]

thanks! (not done with reviewing everything so far)

[MarijnS95]

@maroider @ChrisDenton IMO windows-rs is the future:

• Officially built and backed by Microsoft, so expect proper and timely updates;
• Already much, much better/safer APIs, even if there's still quite a way to go;
• Completely autogenerated, APIs will stay up to date as long as the generator is updated to support (probably ever-expanding) win32metadata - and generator improvements affect all API surface;
• Afaik intended to officially replace winapi in the first place.

I don't think it makes sense for winapi to live on, besides the occasional bugfix to support older crates that have not yet or are in the process of migrating.

As for the open issues (eg. compile speed), that's a bit of a chicken-and-egg problem. The faster windows-rs gets widespread adoption, the quicker issues can be properly triaged and addressed.

However I can also see that it's quite early and with windows-rs breaking updates coming in hot, winit will have to move fast in tracking and updating windows-rs in order to not fall behind or run into the age-old duplicate crate "issue" (affecting compile times and cargo-deny if used, and public API if at all exposed anywhere).

[MarijnS95]

@clemenswasser Fyi windows-rs just released 0.26 with slightly refactored namespaces. I hope that's not causing many issues here as they're mostly related to Direct3D :)

[clemenswasser]

Not using the std feature saves ~10 seconds of build time for me and a few mallocs.

[MarijnS95]

Not using the std feature saves ~10 seconds of build time for me and a few mallocs.

We're getting hangs internally with 6629e52 (no_std), before that everything is fine. Haven't bisected the root cause yet.

[clemenswasser]

Not using the std feature saves ~10 seconds of build time for me and a few mallocs.

We're getting hangs internally with 6629e52 (no_std), before that everything is fine. Haven't bisected the root cause yet.

Does 693fef2 fix it? I forgot to add the null terminators, sorry 😅

[MarijnS95]

Does 693fef2 fix it? I forgot to add the null terminators, sorry sweat_smile

Yeah I completely forgot that I reviewed that when being alerted this morning that our upgrade to windows-rs 0.26 and this PR may have caused the breakage. Surprisingly no segfaults, but I've been told that it's fixed now with the null-terminators added back!

[kennykerr]

I must admit that I'm still on the fence when it comes to adopting the windows crate. winapi's build times are already longer than I'd prefer, and I doubt windows-rs can match winapi's built times when it seemingly generates a whole bunch of convenience wrappers on top of the raw bindings.

The windows-sys crate is coming soon (rationale). It provides only low-level declarations and may be more suitable for cases like this. Here's a simple comparison of compile time.

[maroider]

The windows-sys crate is coming soon (rationale). It provides only low-level declarations and may be more suitable for cases like this. Here's a simple comparison of compile time.

This seems like it would address my biggest concern with migrating to windows-rs.

I do, of course, have smaller complaints, like the atypical module name style and the deepening of use statements, but these are hardly showstoppers.

I also have a minor concern about the soundness of marking simple wrapper-structs like LRESULT(isize) as repr(C) rather than repr(transparent) when said struct is used directly when defining extern fns. The latter more accurately captures the intent of "this struct should be laid out in memory as if it was the contained type".

[kennykerr]

FYI

The windows-sys crate has now been published: https://crates.io/crates/windows-sys

Here's a small windowing example: https://github.com/microsoft/windows-samples-rs/tree/master/create_window_sys

[kennykerr]

I also have a minor concern about the soundness of marking simple wrapper-structs like LRESULT(isize) as repr(C) rather than repr(transparent) when said struct is used directly when defining extern fns. The latter more accurately captures the intent of "this struct should be laid out in memory as if it was the contained type".

The windows and windows-sys crates are derived from metadata and that metadata distinguishes between C and transparent layouts. For example here's LRESULT using repr(transparent):

https://github.com/microsoft/windows-rs/blob/master/crates/deps/sys/src/Windows/Win32/Foundation/mod.rs#L2021-L2022

While MSG uses repr(C):

https://github.com/microsoft/windows-rs/blob/master/crates/deps/sys/src/Windows/Win32/UI/WindowsAndMessaging/mod.rs#L2795-L2804

[maroider]

The windows and windows-sys crates are derived from metadata and that metadata distinguishes between C and transparent layouts. For example here's LRESULT using repr(transparent):

It's nice to see that you fixed this so quickly, but the way you've worded your comment makes it sound like it wasn't a concern in the first place.

I could admittedly be misinterpreting your intent, and it's not a massive problem, but I must admit I'm a tad miffed.

[kennykerr]

but I must admit I'm a tad miffed

I don't follow. It's been unchanged in the windows crate for some time. I only just started generating the windows-sys crate last week and I missed the repr when I first wrote the code gen but its correct as of the first published version. If you spot any problems feel free to open an issue.

My point was just that we have the type information to generate the code correctly.

[maroider]

I'd consider microsoft/windows-rs#1504 to be a blocking issue for the time being.

[clemenswasser]

For experimentation and early adopting, I have updated to the latest windows-sys master branch, which includes microsoft/windows-rs#1550
With that I hope, I could address all outstanding comments. When nobody has anymore feedback, I'll guess we will have to wait until this lands in windows-sys 0.33 so that this branch is mergeable.

[kennykerr]

until this lands in windows-sys 0.33

0.33 should be released this week.

[clemenswasser]

Updated to windows-sys 0.33

[clemenswasser]

Rebased to current master

[clemenswasser]

Some compile time benchmarks:

current master: b7e7755 with winapi 0.3.9:

> hyperfine.exe -n 10 --warmup 2 -p 'cargo clean' 'cargo build'
Benchmark 1: 10
  Time (mean ± σ):      3.641 s ±  0.033 s    [User: 0.000 s, System: 0.003 s]
  Range (min … max):    3.610 s …  3.722 s    10 runs

this branch: a48c1e7 with windows-sys 0.33 & 0.32 (through parking-lot):

> hyperfine.exe -n 10 --warmup 2 -p 'cargo clean' 'cargo build'
Benchmark 1: 10
  Time (mean ± σ):      3.910 s ±  0.017 s    [User: 0.000 s, System: 0.000 s]
  Range (min … max):    3.872 s …  3.938 s    10 runs

this branch: a48c1e7 with windows-sys 0.33 and custom parking-lot also updated to windows-sys 0.33:

> hyperfine.exe -n 10 --warmup 2 -p 'cargo clean' 'cargo build'
Benchmark 1: 10
  Time (mean ± σ):      3.970 s ±  0.034 s    [User: 0.000 s, System: 0.000 s]
  Range (min … max):    3.933 s …  4.041 s    10 runs

So currently a light regression.

[madsmtm]

Compile times are no problem when we're talking such low numbers, no worries!

[msiglreith]

I'm going forward with merging this in order to avoid any new merge conflicts. Given the great number of reviews (thanks to all!) this looks good to go.

Thanks again to @clemenswasser for consistently updating and taking care of this PR!

[madsmtm]

Btw, I was very vocal in this issue about compile-times going from "13 seconds to around 1 minute" because there wasn't really a benefit code-wise; but if someone wants to go and make our Windows platform use the new features in the windows crate to make the backend safer and easier to use, and that ends up increasing compile-times, that would be acceptable in my eyes.