Skip to content

Replace windows-sys dependency with windows-bindgen#161

Closed
Jake-Shadle wants to merge 4 commits intoconsole-rs:masterfrom
EmbarkStudios:remove-windows-sys
Closed

Replace windows-sys dependency with windows-bindgen#161
Jake-Shadle wants to merge 4 commits intoconsole-rs:masterfrom
EmbarkStudios:remove-windows-sys

Conversation

@Jake-Shadle
Copy link

This PR replaces the windows-sys crate with only the minimal binding needed by this crate, removing a source of frustration microsoft/windows-rs#1720

The currently published version uses the outdated 0.42 version, but as we can see in #157, which bumps from 0.42 -> 0.45, the minor version churn of windows-sys is irrelevant for this crate since the bindings are never going to meaningfully change.

@GamePad64
Copy link

There is nothing wrong of having a crate installed multiple times. But there is a problem of having to write the bindings by yourself.
Just imagine, that you want to use some new function from WinAPI, that you have never used before in your project. So, you have to create a binding by yourself, and then test it by hand. So you have to think about the correctness of boilerplate code instead of business logic issues.
Also, the willingness of new contributions by the community would be much lower, bc no one likes writing boilerplate code by themselves :)

windows-sys:
Positive:

  • Reduces boilerplate code
  • Reduces TTM value
  • Ensures correctness, by being autogenerated from win32metadata, which is used in most win32 bindings by Microsoft.
  • Actively maintained
    Negative:
  • About 2MB of gzipped code (srsly?)
  • Can have breaking changes between versions (0.* version numeration)

I strongly advise against pulling the bindings in the project code and taking the burden of maintaining the bindings by the project contributors.

@Jake-Shadle Jake-Shadle changed the title Replace windows-sys dependency with manual bindings Replace windows-sys dependency with windows-bindgen Mar 30, 2023
@@ -0,0 +1,44 @@
// Uncomment this to regenerate bindings

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend running this test unconditionally. Then you can ensure that the bindings are always up to date and regressions haven't snuck in. You can use a diff check via CI to ensure the generated bindings haven't diverged from what's committed to your repo.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's up to the maintainers, personally speaking I think compiling all of that code and writing files to disk every test run for something thst will not meaningfully ever change is wasteful.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely a lot more work to maintain this kind of dependency than simply using a Cargo dependency on the windows-sys crate. 😊

@mitsuhiko
Copy link
Collaborator

Sorry I did not see this. I'm not sure if I like the idea of maintaining those bindings.

@Jake-Shadle
Copy link
Author

No worries.

@MarijnS95
Copy link

MarijnS95 commented Sep 1, 2023

@mitsuhiko is this something you might want to reconsider now that all the tooling is officially published by Microsoft? There are no bindings to "maintain", they only need to be regenerated with the official tooling which is nowadays just a simple api_gen crate in the workspace.

EDIT: Such a thing looks like this Traverse-Research/amd-ext-d3d-rs@fa55fdb, and given the scope of this project it can either use sys bindings or the windows-core-based high-level wrappers.

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.

5 participants