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

Port winapi_util from winapi to windows-sys. #13

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

sunfishcode
Copy link
Contributor

This ports winapi_util from using winapi internally to using windows-sys
internally.

The port is mostly straightforward, except that windows-sys declares
constants like FOREGROUND_BLUE with type u32, which is inconvenient
because they are passed to an API that uses u16, but the workaround
is simple and I've filed an issue in windows-sys.

Background: Some projects I'm working on which use winapi_util are
switching to windows-sys because it covers some APIs we need that
winapi lacks and winapi is not responsive on a PR to add them. I don't
know if others are interested in making this change; if not, feel free to
close this.

@BurntSushi
Copy link
Owner

@retep998 What do you make of the move to windows-sys from winapi? Do you think that's what I should be doing?

@sunfishcode Assuming I do agree with this move, it's going to be logistically challenging given my constrained time. Namely:

  1. This is maybe a big enough change that it might be worth doing a major bump, even though I don't believe there are any technically breaking changes. It just feels bad to swap out such a fundamental dependency in a minor version upgrade.
  2. If I make the switch for this crate, then I'll probably insist on making the switch in every crate I maintain that depends on winapi. The last thing I want is, for example, ripgrep building both winapi and windows-sys. So that means termcolor and walkdir, for example, which are themselves both quite popular. Which means I do not want to go through with this change until I fully understand the trade offs involved here.

Otherwise though, I personally have no particular affinity towards winapi or windows-sys. But that is most likely a result of ignorance rather than any sort of informed opinion.

sunfishcode added a commit to sunfishcode/walkdir that referenced this pull request Jun 17, 2022
walkdir just uses the Windows bindings for one thing, and the change is
straightforward.

This accompanies BurntSushi/winapi-util#13.
@sunfishcode
Copy link
Contributor Author

sunfishcode commented Jun 17, 2022

It appears this change isn't compatible with winapi_util's minimum supported Rust version of 1.34. Empirically, it needs Rust 1.48, which would be another reason to do a major bump.

Looking at ripgrep's Cargo.lock, I see winapi dependencies in two other places:

sunfishcode added a commit to sunfishcode/winx that referenced this pull request Aug 23, 2022
This forks the parts of winapi_util that winx needs, applying the
BurntSushi/winapi-util#13 patch which updates it to windows-sys.
src/console.rs Outdated Show resolved Hide resolved
@LingMan
Copy link

LingMan commented Jan 29, 2024

Is there any way to get this merged (preferably including an update to windows-sys 0.52)?

Looking at cargo-bisect-rustc's lock file there are only two crates still pulling in winapi. One of them is winapi-util, the other is mostly unmaintained and should be replaced anyway.

In contrast 17 crates pull in windows-sys.

ripgrep, termcolor, walkdir, and same-file all depend on winapi only through winapi-util. Guess that's mostly been taken care of since this PR was posted.

@BurntSushi
Copy link
Owner

I have to look into this and, honestly, it isn't a priority for me. The main issue I have with windows-sys is churn. And I don't know what its compile times look like. I don't want to depend on something that is going to be publishing breaking changes every few months. IMO, it needs to settle down first.

An alternative is inlining the bindings themselves into this crate. I think I'd be more amenable to that.

@LingMan
Copy link

LingMan commented Jan 30, 2024

For the compile times it looks like a win. Cross-compiling winapi-util on my old Linux machine with cargo build --release --offline --target x86_64-pc-windows-gnu gives ~2.5s on master while this PR only takes ~1.4s.

I can't deny that there's some version churn, but I don't think it's that much of a problem. Most of the time it's just a version number bump because the API surface is so large and you're unlikely to be using the part that changed. Pretty sure I've seen a single update in the last two years that required minor code changes.
And windows-sys 0.48 was current for ~8 months, which isn't that short IMO.

parking_lot went the inlined bindings route, but that caused linker and target support issues which had to be fixed by depending on windows-targets instead (Amanieu/parking_lot#378). At that point nothing really was gained in regards to version churn.
(Apparently winapi already misses some import libs which would cause issues with some targets.)

PS: Thanks for the updating the PR so quickly, @sunfishcode.

This ports winapi_util from using winapi internally to using windows-sys
internally.
@BurntSushi
Copy link
Owner

https://kennykerr.ca/rust-getting-started/standalone-code-generation.html

For the compile times it looks like a win. Cross-compiling winapi-util on my old Linux machine with cargo build --release --offline --target x86_64-pc-windows-gnu gives ~2.5s on master while this PR only takes ~1.4s.

Awesome! Thanks for testing that.

but I don't think it's that much of a problem.

I hear you. But to be clear here, this is a decision for me to make, since I'm the maintainer and the one who will be experiencing the churn. I move pretty slowly, and needing to do semver incompatible updates every few months is too much for me.

If their cadence is every 8 months, that is... perhaps okay. Do they have a release policy written down anywhere?

parking_lot went the inlined bindings route, but that caused linker and target support issues which had to be fixed by depending on windows-targets instead (Amanieu/parking_lot#378). At that point nothing really was gained in regards to version churn.

I was unaware of this issue. That's unfortunate. Indeed, it looks like that doesn't avoid version churn.

The next thing on my list here would be to look at what windows-targets is actually doing, and whether version churn is an intrinsic property of the Windows support required for this crate (I doubt it), and whether we can do something cheaper.

@LingMan
Copy link

LingMan commented Jan 30, 2024

I hear you. But to be clear here, this is a decision for me to make, since I'm the maintainer and the one who will be experiencing the churn.

Obviously and it wasn't my intention to imply otherwise. Apologies if it came across that way.

If their cadence is every 8 months, that is... perhaps okay. Do they have a release policy written down anywhere?

Apparently they switched from a 3-6 months cadence to on-demand releases in Nov 2022 (microsoft/windows-rs#2156 (comment) and microsoft/windows-rs#2156 (comment)). Might at times be more volatile than every 8 months then.

@ChrisDenton
Copy link
Contributor

The next thing on my list here would be to look at what windows-targets is actually doing, and whether version churn is an intrinsic property of the Windows support required for this crate (I doubt it), and whether we can do something cheaper.

winapi contains a bunch of binaries (import libraries) for different platforms that all get downloaded if you use the crate. On the other hand windows-targets serves as a kind of meta-crate for instead only picking the right libs for your target. Alternatively it can use the new raw-dylib rustc feature to avoid the binaries altogether but only if the user sets an environment variable. This will hopefully become the default (and maybe only) way once the windows-sys MSRV is high enough.

I'll admit you can't beat winapi for stability. It is now only getting security patches so it's very unlikely to ever have a breaking change. On the flip side though it doesn't get new functions, bug fixes or support for new targets.

@BurntSushi
Copy link
Owner

It is now only getting security patches

While it's not declared as such, from the looks of it, winapi is unmaintained. Its last commit was >=3 years ago. So I'm not sure it's even getting security patches.

It's because of that that I acknowledge that winapi is not the long term solution here. And, windows-sys is maintained by the Microsoft folks, which is nice.

I've been thinking about this more, and I'm leaning towards just merging this PR and seeing what the churn feels like.

@retep998
Copy link

It is now only getting security patches

While it's not declared as such, from the looks of it, winapi is unmaintained. Its last commit was >=3 years ago. So I'm not sure it's even getting security patches.

If a security patch is ever needed, you can always just poke me on discord. But yeah, windows-rs killed any interest I had in continuing to work on winapi, so barring something serious I have no intention of updating winapi further.

@Palkovsky
Copy link

Palkovsky commented Apr 23, 2024

We've hit issues while porting our project to the new arm64ec-pc-windows-msvc target . winapi doesn't and probably won't support it, but gets included anyway through the walkdir dependency tree, effectively breaking our build. windows-sys on the other hand supports it.

@BurntSushi BurntSushi merged commit 732de05 into BurntSushi:master Apr 23, 2024
8 checks passed
@BurntSushi
Copy link
Owner

This PR is on crates.io in winapi-util 0.1.7.

BurntSushi added a commit to BurntSushi/ripgrep that referenced this pull request Apr 23, 2024
Notably, this removes winapi in favor of windows-sys, as a result of
winapi-util switching over to windows-sys[1].

Annoyingly, when PCRE2 is enabled, this brings in a dependency on
`once_cell`[2]. I had worked to remove it from my dependencies and now
it's back. Gah. I suppose I could disable the `parallel` feature of
`cc`, but that doesn't seem like a good trade-off.

[1]: BurntSushi/winapi-util#13
[2]: rust-lang/cc-rs#1037
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.

7 participants