microsoft / windows-rs

Rust for Windows
https://kennykerr.ca/rust-getting-started/
Apache License 2.0
10.23k stars 475 forks source link

Upgrade win32metadata to 60.0.34 #3044

Closed MarijnS95 closed 3 months ago

MarijnS95 commented 3 months ago

https://www.nuget.org/packages/Microsoft.Windows.SDK.Win32Metadata/60.0.34-preview

For me this most notably includes D3D12 Agility SDK 1.613.1 and annotations that leverage #2771.

kennykerr commented 3 months ago

Thanks for the reminder - I'll update the metadata shortly.

MarijnS95 commented 3 months ago

@kennykerr why insta-close this when an external contributor has already put in the work? Don't you want non-Microsoft contributors in your commit history?

kennykerr commented 3 months ago
MarijnS95 commented 3 months ago

Can I submit the "unrelated changes" separately without risking them being closed, such as readability improvements to crates/libs/bindgen/default/readme.md and handling PSID?


For what it's worth, starting an insta-close with a more thorough review like the above would have been a lot more friendly.

kennykerr commented 3 months ago
MarijnS95 commented 3 months ago
  • An "unrelated changes" PR that includes fixes to a variety of unrelated things will not be accepted.

Yeah, that's implicit. I shouldn't have sneaked the formatting fixes in for sure.

MarijnS95 commented 3 months ago

and handling PSID?

This won't have an effect without the metadata update, and is probably not very useful to you in isolation.

MarijnS95 commented 3 months ago

@kennykerr I do have a specific question with regards to DXGI_SWAP_CHAIN_DESC(1)::Flags (and the decode desc) which is of type UINT. In https://github.com/microsoft/win32metadata/pull/1757 I associated that with DXGI_SWAP_CHAIN_FLAG, but enums have int ABI by default (unlike the enums I added in that PR in enums.json which were all given "type": "uint", and ILSpy shows e.g. enum DXGI_PRESENT : uint).

Is there anything we can do in windows-rs to tie this back to an enum type when the ABI mismatches? Is there a tracking issue open about this? Or should win32metadata override the enum ABI to be UINT (here and probably for others)?

riverar commented 3 months ago

[..] unlike the enums I added in that PR in enums.json which were all given "type": "uint", and ILSpy shows e.g. enum DXGI_PRESENT : uint).

We need to go back and fix that in metadata; looks like we missed these uint types. I probably should have been more clear in my PR comment.

kennykerr commented 3 months ago

We'll have to wait for this (and https://github.com/microsoft/win32metadata/issues/1908) to get fixed before updating windows-rs.

MarijnS95 commented 3 months ago

@riverar thanks, what's the right way to correct the type, list it in enums.json?

riverar commented 3 months ago

@MarijnS95 Ideally yup, with the rsp method as a fallback. We can double-check with Mike too on the other side of the fence.

MarijnS95 commented 3 months ago

[..] unlike the enums I added in that PR in enums.json which were all given "type": "uint", and ILSpy shows e.g. enum DXGI_PRESENT : uint).

We need to go back and fix that in metadata; looks like we missed these uint types. I probably should have been more clear in my PR comment.

@riverar just to double-check, what do you mean by this?

  1. The type: uint was wrong, and should have been kept at the default int?
  2. Forcing the untyped enums like DXGI_SWAP_CHAIN_FLAG mentioned here to uint, as that is how all current ABI is using it?

I assume the latter.