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

Upgrade win32metadata to 60.0.34 #3044

Closed
wants to merge 2 commits into from

Conversation

MarijnS95
Copy link
Contributor

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.

@MarijnS95 MarijnS95 force-pushed the d3d12-agility-1.613 branch from fb4ac2e to f3bed1f Compare May 16, 2024 18:45
@kennykerr
Copy link
Collaborator

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

@kennykerr kennykerr closed this May 16, 2024
@MarijnS95
Copy link
Contributor Author

@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
Copy link
Collaborator

  • There are many external contributors. The top contributors are all non-Microsoft last I checked.
  • The metadata has blocking bugs that need to be resolved first (e.g. BCRYPT_HANDLE should not have a RAIIFree attribute win32metadata#1908)
  • There isn't currently a way to validate the winmd provenance, so it has to be done in house. Once we have a yml workflow to validate provenance, external contributors can update metadata.
  • Your PR snuck in unrelated changes beyond simply updating metadata.

@MarijnS95
Copy link
Contributor Author

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
Copy link
Collaborator

  • Discrete contributions fixing very specific things are welcome. A PR to improve a readme file for example is great.
  • An "unrelated changes" PR that includes fixes to a variety of unrelated things will not be accepted.
  • If there's anything that requires discussion, please open an issue for discussion to avoid spending time on a PR unnecessarily.

@MarijnS95
Copy link
Contributor Author

  • 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
Copy link
Contributor Author

and handling PSID?

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

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented May 17, 2024

@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 microsoft/win32metadata#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
Copy link
Collaborator

riverar commented May 17, 2024

[..] 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
Copy link
Collaborator

We'll have to wait for this (and microsoft/win32metadata#1908) to get fixed before updating windows-rs.

@MarijnS95
Copy link
Contributor Author

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

@riverar
Copy link
Collaborator

riverar commented May 17, 2024

@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
Copy link
Contributor Author

[..] 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.

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.

3 participants