-
Notifications
You must be signed in to change notification settings - Fork 25
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
Consider windows-bindgen instead of windows-sys dependency #196
Comments
From https://kennykerr.ca/rust-getting-started/standalone-code-generation.html
From chronotope/chrono#1003 regarding what chrono should use
And at this time, chrono is the only non-MS user of windows-bindgen: https://crates.io/crates/windows-bindgen/reverse_dependencies windows-bindgen also has a looser MSRV policy than us. So long as there aren't things we need from the newer versions, that shouldn't be too much of a problem. |
There are a few more users: cc as mentioned above, Rust itself, and a code search on Github turns up iana-time-zone and sys-locale. They just don't show up as reverse (dev-)dependencies because they don't generate the bindings from a test but from a separate crate that isn't published. This lack of ecosystem-wide visibility into who's using which versions of the bindings is another downside of vendoring generated code. The more I read and think about this, the less I like the idea. And I wasn't really sure about it to begin with (hence the "consider" qualifier in the title). |
I'll close this because, as time goes on, I keep stumbling into more reasons why it's unlikely that making this change would benefit me, or anyone else. For many non-trivial programs, trying to avoid windows-sys (and in particular the Win32_Foundation feature) is already hard, not getting easier over time, and has little if any tangible benefit. And the case of tiny programs, where it could make a difference, is rather hypothetical: most of these programs don't really need colored text output, or they don't run on Windows, or they do run on Windows but can make do without colors there, or their from-scratch build time doesn't really matter that much. |
The use of windows-sys in anstyle-query and anstyle-wincon is pretty simple, pretty small in scope, and these crates only enable the minimum necessary features of windows-sys. Unfortunately, one of the necessary features is
Win32_Foundation
, which pulls in a 10k line module. This has an outsized impact on build times: compiling windows-sys as a dependency of anstream or clap takes 0.7--0.8 seconds on my (older but still decently fast) desktop and 1.2--1.7 seconds on my laptop. This is around 10-15% of the clean debug build time for a "hello world" program using clap with default features (no derive), and between a third and a half of the build time when using (only) anstream in a very simple program. For such simple programs, window-sys also sticks out incargo --timings
as a serial bottleneck on the critical path. Even when the biggest serial bottleneck isclap_builder
, that one also transitively depends on windows-sys, so the problem compounds.Most likely, none of that matters in most projects. In the workspace that prompted me to look into this, removing all uses of windows-sys would currently only save ca. 1% of CPU time spent on debug builds (less for release), and wouldn't matter at all for wall-clock time. However, a more lightweight anstream would be nice to have for tiny helper programs (cargo-xtask pattern, cargo-script). I'd consider disabling legacy Windows console support in those cases, but (1) that's a hassle, and (2) I don't see a feature flag for disabling only that part of anstyle-query -- giving up auto-detection entirely would suck.
The alternative to using windows-sys is using
windows-bindgen
to generate only the necessary subset of the bindings ahead of time and commit the generated files. This has some obvious disadvantages: it's more complicated, more error-prone, and it duplicates code from windows-sys that may end up getting compiled anyway for many users of anstyle. But would at least address the issues discussed above without resorting to hand-written bindings, and it should be "only" be around 300 lines of generated code. For comparison, the cc crate has used such generated bindings for the past year (and so has rustc, and probably others).The text was updated successfully, but these errors were encountered: