rust-cli / anstyle

ANSI text styling
https://docs.rs/anstyle
Other
106 stars 16 forks source link

Consider windows-bindgen instead of windows-sys dependency #196

Closed hanna-kruppe closed 1 month ago

hanna-kruppe commented 3 months ago

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 in cargo --timings as a serial bottleneck on the critical path. Even when the biggest serial bottleneck is clap_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).

epage commented 3 months ago

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

Warning: Standalone code generation should only be used as a last resort for the most demanding scenarios. It is much simpler to use the windows-sys crate and let Cargo manage this dependency. This windows-sys crate provides raw bindings, is heavily tested and widely used, and should not meaningfully impact your build time.

From chronotope/chrono#1003 regarding what chrono should use

I don't recommend using windows-bindgen directly. I think your best bet is to stick with windows-sys.

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

And at this time, chrono is the only non-MS user of windows-bindgen: https://crates.io/crates/windows-bindgen/reverse_dependencies (and they never checked in again with kennykerr after they said not to use it)

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.

hanna-kruppe commented 3 months ago

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).

hanna-kruppe commented 1 month ago

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.