Replace `windows-sys` with manual bindings by Jake-Shadle · Pull Request #473 · notify-rs/notify

Jake-Shadle · 2023-03-21T09:11:18Z

This replaces the dependency on windows-sys with manual bindings to the win32 functionality used by this crate.

The rationale for this change is that these bindings will never meaningfully change so it doesn't make sense to use an external dependency, especially in light of microsoft/windows-rs#1720, which is why the currently published version of this crate actually ends up pulling in 2 version of windows-sys since the published version depends on 0.42, but the mio dependency uses 0.45. And as shown in #472, the minor version changes in windows-sys are noise, and only meaningful in the negative impact.

0xpr03 · 2023-03-21T14:09:17Z

Note: we just accepted #457 to switch towards windows-sys from winapi (and #472 to update windows-sys)

0xpr03 · 2023-03-21T14:12:19Z

notify/src/windows.rs

+
+#[allow(non_camel_case_types, non_snake_case)]
+mod bindings {
+    pub const INFINITE: u32 = 4294967295;


my first reaction to this line was: this is why I wouldn't want to do the bindings myself, magic numbers from an OS for which I have no way of validating them, and if they're getting stale, it will take years till people notify (heh) that we're the ones getting it wrong

(but it's not a blocker by default)

Yah, understandable, these bindings are sourced from the same https://github.com/microsoft/win32metadata that windows-sys uses if that's any consolation.

Now think about the manual work you signed up for when we fix bugs in upstream win32metadata and push out fixes. (Minor in this crate, I admit, but work is work.)

notify/src/windows.rs

0xpr03 · 2023-03-21T14:28:22Z

I'm currently tending towards reverting #457 and simply going back to winapi. It may not be developed actively, but a) the (x86_64) winapi probably won't change until the heat death of the universe, b) we're not using anything special and c) that means winapi users converge towards one version.

Jake-Shadle · 2023-03-21T17:05:55Z

I think we would be fine with going to back to winapi for the moment, but I think as time moves forward most crates will either switch to windows-sys or use manual bindings to avoid the windows-sys churn, so I guess in the future it could be a situation where only a few crates are still using winapi, which shows they don't actually need to update to stay relevant, and IMO makes the manual binding approach more future proof.

0xpr03 · 2023-03-21T22:14:47Z

ok let's switch this around: if you want to have this, I'm willing to add a feature flag for this, let's call it slim_windows, and if that's enabled, the windows-sys crate isn't used, that will be non-official and no default, but if you want this in our repo, you can have that

Jake-Shadle · 2023-03-21T22:40:32Z

That would work! Traveling tomorrow so might not have it for a bit.

Jake-Shadle · 2023-03-21T22:59:25Z

So unfortunately due to how features work I don't think that exact scenario works, but I've made windows-sys an optional, default crate, and added a slim_windows feature, if either that feature is enabled or windows-sys is disabled then the manual bindings are used instead of the windows-sys ones.

0xpr03 · 2023-03-21T23:29:50Z

notify/src/windows.rs

+#[cfg(all(feature = "windows-sys", not(feature = "slim_windows")))]
+use sys::*;
+
+#[cfg(any(not(feature = "windows-sys"), feature = "slim_windows"))]


This is sadly a slippery slope. People use no-default-features to specify kqueue as macos backend. By this definition they automatically get the slim backend, which is unintentional (and got me wondering why the CI passed in the first place).

I've already had a working change to make it a requirement, but then we'd have to bump the major version as everyone has to migrate their features. So that's not really a solution.

I originally had it on just whether they had the slim feature enabled, but if they disable default features and don't enable slim it would fail to compile, but if you're OK with that I can do that instead.

GamePad64 · 2023-03-22T09:14:08Z

windows-sys is a pretty slim wrapper itself, and its versioning is organized the way it is to provide an ability to use different versions of it in different nodes of dependency tree without any negative circumstances.

We can just pin windows-sys to a specific version and forget about it. mio can use another version of it and it would be pretty fine.

Jake-Shadle · 2023-03-22T09:42:30Z

windows-sys is by far the largest dependency most crates will have in terms of download and disk space, and even with only the needed features enabled, this crate uses a small fraction of the enabled APIs, making the dependency wasteful. Pinning would be even worse as then you are guaranteeing that multiple versions will be used in even a moderately sized dependency graph.

GamePad64 · 2023-03-22T09:59:22Z

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 · 2023-03-24T15:16:26Z

Ok I split out so that the internal bindings will only be used if the feature is enabled, and emit a compile error if neither the windows-sys or the slim_windows features are enabled

0xpr03 · 2023-04-04T12:55:20Z

Ok, so this would still be a breaking change, thus requiring a v6 release. We would have to select windows-sys when none of these two features is specified? Because people followed this example, which doesn't specify that feature by default.

Jake-Shadle · 2023-04-04T13:02:21Z

Yes, it's a breaking change for those who use default-features = false, but the mitigation for that is the explicit compile_error! to tell the user what went wrong.

0xpr03 · 2023-04-04T13:28:59Z

Hm, that's a fair trade I guess ?

Jake-Shadle · 2023-04-04T13:32:56Z

IMO yes, though there could be an issue if a crate depends on notify with default-features = false, then doesn't compile for Windows, does a publish, and then gets transitively depended on by a crate/project that does compile for Windows.

GamePad64 · 2023-04-04T14:05:44Z

There should be a simple process of updating the bindings, prior to merging this PR.

0xpr03 · 2023-04-04T14:14:38Z

simple process of updating the bindings

What do you mean with that ? As it currently stands, this will be v6

0xpr03 · 2023-04-04T14:18:23Z

On one hand it's not really a problem to do a v6 for this. But on the other side, doing a v6 only for a feature that, as of now, exactly one person uses is kind of weird. But I currently have no other breaking changes lined up.

riverar · 2023-04-04T14:58:19Z

The rationale for this change is that these bindings will never meaningfully change

This assertion continues to be untrue, like I've mentioned before in other repositories.

Is there a demonstrable problem here? Doesn't appear version churn was an issue for this crate.

Jake-Shadle · 2023-04-04T15:24:15Z

This assertion continues to be untrue, like I've mentioned before in other repositories.

I take it you're referring to microsoft/windows-rs#1720 (comment)? If so...then you would need to update notify to point to different function/s regardless of where those bindings come from, and adding the additional binding(s) is a one time operation that can be done as needed. If you're saying that the constants/structs/ABI used by this crate can be changed at some point in the future by Microsoft, then I'm just honestly confused. To my knowledge Microsoft has never intentionally done an API/ABI breaking change like that in the history of Windows, which is why I can run a program (other than 16-bit) compiled 30 years ago for Windows 95 on a modern Windows machine without any issue.

Is there a demonstrable problem here? Doesn't appear version churn was an issue for this crate.

It current version depends on 0.42 (6 months old), so it avoided the version churn by not updating, guaranteeing multiple versions since other crates have updated to new versions.

GamePad64 · 2023-04-05T12:04:15Z

guaranteeing multiple versions since other crates have updated to new versions.

So what, exactly? This isn't a problem in Rust ecosystem, as using multiple versions of the same library, when a major version is updated, is a Rust feature, not a bug.
The issue you are trying to solve is miniscule, compared to project contributors' burden to maintain the bindings and regenerate them if they need a new WinAPI function.

The approach you are proposing (slim-windows feature) just makes it more difficult to maintain notify-rs and gives nothing in return.

What real benefit do you want to bring by this change?

Jake-Shadle · 2023-04-05T12:42:22Z

Closing this since this thread is unproductive. Sorry for the headache @0xpr03 .

Replace windows-sys with manual bindings

270c198

Jake-Shadle changed the title ~~Replace windows-sys with manual bindings~~ Replace windows-sys with manual bindings Mar 21, 2023

Jake-Shadle mentioned this pull request Mar 21, 2023

Embark's pull requests EmbarkStudios/rust-ecosystem#20

Open

26 tasks

0xpr03 reviewed Mar 21, 2023

View reviewed changes

notify/src/windows.rs Outdated Show resolved Hide resolved

Add comment on bindings module

c975338

Make manual bindings an optional feature

24e389e

0xpr03 reviewed Mar 21, 2023

View reviewed changes

Jake-Shadle added 2 commits March 24, 2023 16:11

Only use manual bindings if feature is enabled

bb5f88d

Add check build for internal bindings

b602001

Fix CI

3d3695c

Jake-Shadle closed this Apr 5, 2023

Jake-Shadle mentioned this pull request Apr 20, 2023

Replace windows with embedded bindings RustAudio/cpal#778

Closed

0xpr03 mentioned this pull request Apr 26, 2023

Update windows-sys to 0.48 #479

Merged

Uh oh!

Conversation

Jake-Shadle commented Mar 21, 2023

Uh oh!

0xpr03 commented Mar 21, 2023

Uh oh!

0xpr03 Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jake-Shadle Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

riverar Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xpr03 commented Mar 21, 2023

Uh oh!

Jake-Shadle commented Mar 21, 2023

Uh oh!

0xpr03 commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jake-Shadle commented Mar 21, 2023

Uh oh!

Jake-Shadle commented Mar 21, 2023

Uh oh!

0xpr03 Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

0xpr03 Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

Jake-Shadle Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

GamePad64 commented Mar 22, 2023

Uh oh!

Jake-Shadle commented Mar 22, 2023

Uh oh!

GamePad64 commented Mar 22, 2023

Uh oh!

Jake-Shadle commented Mar 24, 2023

Uh oh!

0xpr03 commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jake-Shadle commented Apr 4, 2023

Uh oh!

0xpr03 commented Apr 4, 2023

Uh oh!

Jake-Shadle commented Apr 4, 2023

Uh oh!

GamePad64 commented Apr 4, 2023

Uh oh!

0xpr03 commented Apr 4, 2023

Uh oh!

0xpr03 commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riverar commented Apr 4, 2023

Uh oh!

Jake-Shadle commented Apr 4, 2023

Uh oh!

GamePad64 commented Apr 5, 2023

Uh oh!

Jake-Shadle commented Apr 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

0xpr03 Mar 21, 2023 •

edited

Loading

riverar Apr 4, 2023 •

edited

Loading

0xpr03 commented Mar 21, 2023 •

edited

Loading

0xpr03 commented Apr 4, 2023 •

edited

Loading

0xpr03 commented Apr 4, 2023 •

edited

Loading