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

Migrate to Windows-rs from winapi #3207

Closed
Elabajaba opened this issue Nov 14, 2022 · 32 comments · Fixed by #5956
Closed

Migrate to Windows-rs from winapi #3207

Elabajaba opened this issue Nov 14, 2022 · 32 comments · Fixed by #5956
Labels
api: dx12 Issues with DX12 or DXGI external: upstream Issues happening in lower level APIs or platforms type: enhancement New feature or request

Comments

@Elabajaba
Copy link
Contributor

Is your feature request related to a problem? Please describe.
winapi-rs should probably be considered dead at this point, and Microsoft seems to have settled on windows-rs as the officially supported Windows api bindings. (and have deprecated all the other ones they were experimenting with).

Describe the solution you'd like
Migrate all the winapi code and dependencies to windows-rs.

Additional context
I believe this was previously blocked on a combination of windows-rs being immature, and Mozilla having to get windows-rs approved internally. Windows-rs seems to have settled down a lot, and I'm not sure whether or not Mozilla has okayed it, or if that's even still considered a blocker these days.

@jrmuizel
Copy link
Contributor

It seems like there's still a lot of churn happening: microsoft/windows-rs#2156. We don't really want to end up with a bunch of different versions of windows-rs vendored into the mozilla tree so I think we'll need to wait longer for things to settle more.

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Nov 15, 2022

Copying what I said on matrix last night for long term transparency on the reasoning for the switch and why it isn't possible at this time:

wgpu is going to need more modern dx12 features at some point (including access to new windows 11 apis). winapi, as it stands, doesn't offer bindings to any of these because development died about 2 years ago. winapi is extensable though as they do export their macros. So we can add bindings to these things to d3d12-rs, it just means we have to manually write them.

Before we commit to that, if it would be possible to upgrade to the new, "en vogue" windows bindings it would be good to do so. If we can't due to one of a variety of reasons, that's fine, we just need to expand our bindings when the time comes for us to have newer features

From the comments we've gotten from @jimblandy, @kdashg, and jrmuizel it seems like it's not possible at this point - which is fine, we just should commit to our stance while windows-rs, moz, etc all figure out what's going on wrt windows bindings

@cwfitzgerald cwfitzgerald added type: enhancement New feature or request api: dx12 Issues with DX12 or DXGI api: dx11 external: upstream Issues happening in lower level APIs or platforms labels Nov 15, 2022
@cwfitzgerald
Copy link
Member

There's also a standalone PR retep998/winapi-rs#812 that never got merged that we could yank.

@Elabajaba
Copy link
Contributor Author

Windows-rs maintainers have stated they're going to slow down their cadence and release new windows versions "as necessary", instead of a regular monthly release (but still at a faster rate than the every ~6 months of windows-sys).

There's also been work on updated directx bindings (microsoft/DirectX-Headers#79 and microsoft/DirectX-Headers#82 for the initial legwork), though I'm not sure what the timeline on that might be, or if those would be released as part of windows-rs, or as a separate crate.

Elabajaba added a commit to Elabajaba/wgpu that referenced this issue Nov 26, 2022
@MarijnS95
Copy link
Contributor

@Elabajaba fyi I've closed those DirectX-Headers PRs, the headers are now instead directly consumed by win32metadata and windows-rs so the Agility SDK is immediately available in the official windows crate 🎉

@Elabajaba
Copy link
Contributor Author

Elabajaba commented Jun 8, 2023

I'm not sure what exact APIs we need, but windows-bindgen now exists and lets us generate our own (minimal) windows bindings if we want. (see RustAudio/cpal#778 for an example)

windows-core also exists now, not really sure what it is exactly. It seems to be a "small" base crate that you use when generating bindings?

@ErichDonGubler
Copy link
Member

Another example of using windows-targets to generate bindings directly for windows-sys-like bindings can be found in parking-lot: https://github.com/Amanieu/parking_lot/blob/e9f9f40b860d7de8238a8c07aee3ae05992ab681/core/src/thread_parker/windows/bindings.rs

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Jun 8, 2023

@Elabajaba: It seems like windows-targets is the only necessity for manual windows-sys-like bindings.

If we want windows-like bindings (i.e., to use COM APIs), we'll also need windows-core at a minimum, for manual bindings from that point. If we want automatic bindings, then windows-bindgen does indeed appear to be the way to go for a subset of windows.

No action items here, just noting what's available.

@MarijnS95
Copy link
Contributor

windows-core also exists now, not really sure what it is exactly.

It is a minimal subset - the hand-written boilerplate if you wish - of the windows crate. Intended to be used with windows-bindgen to generate minimal windows (so with COM support!) bindings for just the items that you need from the API.

@Elabajaba
Copy link
Contributor Author

Looks like windows-bindgen has been deprecated in favour of riddle? microsoft/windows-rs#2544

@MarijnS95
Copy link
Contributor

@Elabajaba riddle is just a more consolidated though also less Rusty (from a user-perspective) tool to achieve the same as windows-bindgen.

@Elabajaba
Copy link
Contributor Author

@MarijnS95 do you know if there's any interoperability between different crates that generated their own binding? (eg. could we pass a wgpu ID3D12Resource to a function that expects a gpu-allocator ID3D12Resource, or even a windows-rs ID3D12Resource?)

@MarijnS95
Copy link
Contributor

@Elabajaba while they may have the same layout, they are type-incompatible if coming from locally generated bindings vs other crates' bindings and/or windows-rs: that makes this method infeasible for public types.

It's similar to having two types from different versions of the same create: they may look the same but the compiler rightfully considers them different.

@Elabajaba
Copy link
Contributor Author

That's what I figured :/

@MarijnS95
Copy link
Contributor

Note that windows-bindgen is alive and kicking again, it's just a library interface to riddle (which itself won't be published for some time). Basically replacing the cargo install riddle; riddle --etc bindings.txt with a crate that does the same and can be invoked as cargo r -p api_gen.

You may find this example useful, where I do just that: https://github.com/Traverse-Research/amd-ext-d3d-rs/pull/3/files

@jimblandy
Copy link
Member

In case it matters: Firefox now permits depending on windows-sys, but still not windows. My understanding is that windows-sys does not offer COM API bindings, which are the ones we need, so this doesn't help us much.

@cwfitzgerald
Copy link
Member

Yeah, so it seems like the plan is for us to generate our own bindings using the appropriate binding generator, then wherever we need to expose things, allow POD-type conversions to windows types.

@jimblandy
Copy link
Member

In Traverse-Research/gpu-allocator#181, they said they're cool going with a bindgen-based approach. So maybe we have a way forward here.

@MarijnS95
Copy link
Contributor

@jimblandy if we're going the windows-bindgen route (in every crate that Mozilla uses), be sure to vet windows-core first, which provides the most basic types support needed for COM APIs (and other things) - it is heavily referenced by windows-bindgen output.


@cwfitzgerald yeah, that's the only unfortunate downside. I'm thinking of having an optional full-windows-api-something feature that provides convenient entrypoints with windows types for users to not mistake types, but we'll have to provide POD-based (void pointers, or the internally generated types) entrypoints for those that cannot use the windows crate. Not sure how to model that conveniently just yet.

@MarijnS95
Copy link
Contributor

Some prior art that we had to do for winapi compatibility:

https://github.com/Traverse-Research/gpu-allocator/blob/main/src/d3d12/mod.rs

I'm thinking of replacing that with windows-rs or PODs so that it will work with windows-bindgen types internally.

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Jan 24, 2024

Firefox downstream issue for using windows-rs: bug 1773509

In case anybody finds it interesting, I've been pragmatically exploring consuming a forked gpu-allocator based on winapi with only a [patch] entry in mozilla-central (where Firefox's code lives). This makes Firefox consume ErichDonGubler/gpu-allocator#1, which is source-compatible for wgpu, but not necessarily SemVer-adherent in general. 😅

To be clear, Firefox's WebGPU team prefers long-term that Firefox move on to simply allow the usage of the windows ecosystem (and it's being worked on in the bug I mentioned above as of recently). Still, perhaps another downstream might benefit from knowing that the above work compiles and runs; see this CI run for the current version's Windows run specifically: try:39a00cfbc374. There are regressions, but mostly in resource limitation boundary testing, with which we were already having issues anyway.

@MarijnS95
Copy link
Contributor

@ErichDonGubler for the record, note that gpu-allocator is still open to switching to windows-bindgen + windows-core which would be allowed for Firefox, right?

@ErichDonGubler
Copy link
Member

@MarijnS95: I had dropped getting back to you on this, sorry about that! Firefox actually uses the windows crate wholesale now, so we should be good on that front.

All we need to do at this point is migrate away from winapi code to "only" the windows crate.

@MarijnS95
Copy link
Contributor

@ErichDonGubler that's good news! Fortunately gpu-allocator already moved away from winapi if not for the optional public-winapi feature to make it easier to use gpu-allocator with winapi types.

I've been contemplating removing that altogether as I hope less and less people rely on these legacy/unmaintained bindings.


Separately we're still thinking about switching to windows-core with generated bindings (and an optionally-public windows crate interface to avoid pointer casts), to see if that addresses some of our compile-time concerns. windows takes a concerning amount of time to build, especially after resolver2 unified all the feature flags across many crate dependencies. That would not only reduce the build time for the windows crate, but also allow dependent crates to start compiling earlier.
(At this point I'm yet uncertain if generating bindings for the Dxgi and Direct3D12 namespaces is as expensive to compile as compiling the windows crate with just those two features)

@notgull
Copy link

notgull commented Apr 6, 2024

Separately we're still thinking about switching to windows-core with generated bindings (and an optionally-public windows crate interface to avoid pointer casts), to see if that addresses some of our compile-time concerns.

Might not be the right place for this discussion, but I would avoid this. Since the windows-sys crate is already used in the winit source tree, I doubt it would have much of a positive effect.

cc smol-rs/async-signal#14

@MarijnS95
Copy link
Contributor

@notgull windows != windows-sys, fwiw. The sys crate does not contain COM bindings.

@ErichDonGubler
Copy link
Member

@MarijnS95: We're already paying the cost of compiling windows in Firefox now, so 🤷🏻‍♂️ do what's good for your other users, I guess! I'd recommend adding a windows feature, so people can consolidate into the windows if they so choose; that way, Firefox doesn't have to compile both windows-core + bindings and windows. 🙂

@MarijnS95
Copy link
Contributor

Yeah - I remember why I did not want to do this.

windows-bindgen wraps all cross-module references in the same cfg(feature = "...")s used to turn on those modules, to reduce massive automatically-enabled dependency chains. That's not something we'd like to use when generating an in-crate file, as we'd have to have all those features described and default-enabled in our own [features] table. Generating feature cfgs is easily turned off (by default even), but that does generate unwrapped cross-module references all over the place (rather than disabling generation of affected functions/structs or hiding them in plain sight as commented-out code like gir does).

I'm quite certain that this was already reported upstream, but can no longer find an open issue about it. It's either closed as "this is a feature", or implemented and will come in a followup release.

@teoxoy
Copy link
Member

teoxoy commented Jun 14, 2024

I bumped into this sample, I found it useful to see how the API is used in practice since the docs are not very useful.
https://github.com/microsoft/windows-rs/blob/0.57.0/crates/samples/windows/direct3d12/src/main.rs

@MarijnS95
Copy link
Contributor

Since I've ported and maintained my fair share of windows-rs based Direct3D12 renderers, I've started picking up this task: a branch is on my fork. Currently in the "second stage" of porting, there are still 350 errors to address :)

@teoxoy teoxoy removed the api: dx11 label Jul 3, 2024
@MarijnS95
Copy link
Contributor

Short update: the branch on my fork now compiles, but there are still a few clippy lints to resolve (mainly thanks to windows' use of Result and #[must_use] on previously-unchecked functions). I've been developing this from a Linux machine with cross-compilation (because Windows is so mindbendingly slow by design 😩), and haven't ran any of it yet. Most notably, all the linking vs dynamic loading code from "the d3d12 crate" hasn't been ported yet and is still todo!(), so you won't get very far. I'll port this really soon and open a draft PR to get going!

@MarijnS95
Copy link
Contributor

It is very late but I just got this over with: #5956

Still draft, feel free to try it out but don't bother giving it a review until I've ±finalized the refactor, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: dx12 Issues with DX12 or DXGI external: upstream Issues happening in lower level APIs or platforms type: enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants