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

Use winapi crate instead of windows crate #35

Merged
merged 2 commits into from Apr 15, 2022
Merged

Use winapi crate instead of windows crate #35

merged 2 commits into from Apr 15, 2022

Conversation

Kijewski
Copy link
Collaborator

This reduces the minimum supported rust version by quite a lot.

@astraw
Copy link
Member

astraw commented Apr 13, 2022

Yes, I was a little surprised that windows 0.35 updated their MSRV to 1.59 recently, which is what caused me to take the opportunity to update the MSRV in iana-time-zone also to 1.59. (Although for mac and linux, this wasn't actually necessary.)

But with this PR, I am concerned that increases dramatically the amount of code to be maintained here. Also, I assumed due to Microsoft's own support that windows was "the future" but I don't actually follow this very closely. I guess winapi and windows crates are two ways of doing the same thing, one widely used and independently maintained (winapi) and the other less widely used for now and maintained by microsoft (windows)?

At the time of bumping the MSRV here, I checked into the issue "does updating MSRV imply the semver should be bumped to indicate a breaking change?" to which I found the general consensus to be "no" because a) people can painlessly update, b) otherwise people have Cargo.lock and c) otherwise the ecosystem gets split. The other general point I looked into was an answer to the question "what reasons to people have for sticking with an old rust version that cannot be addressed fairly easily with a Cargo.lock file?" to which the consensus I found to be "few good reasons". With that research done, I updated my rust compiler in CI and then bumped the MSRV in the libraries and moved on...

Due to those reasons, I am hesitant about the switch to winapi but the other changes here are certainly interesting. What do you think?

@Kijewski
Copy link
Collaborator Author

It's difficult. I for one am not even sure what's stopping people from simply using the most current stable rust version, but I guess there's a bunch of Centos users out there. :)

The windows crate (probably) is the future, but I think it's still in its infancy with many breaking changes between versions. (But don't take my word for it, because I'm neither a windows user nor do a program for windows normally.) It's too bad that the winapi crate is not supported by a team, esp. since its used by fundamental crates like tokio.

My code is ugly, but I don't think that there is the need to ever touch it again. I added few comments, but I don't think that it's easy to read or understand. I would totally understand if you choose not to adopt my PR.

This reduces the minimum supported rust version by quite a lot.
@Kijewski
Copy link
Collaborator Author

Kijewski commented Apr 14, 2022

I included the Cargo.lock so cargo audit works.

Comment on lines +7 to 15
//! # mod chrono_tz { pub type Tz = String; }
//! // Get the current time zone as a string.
//! let tz_str = iana_time_zone::get_timezone()?;
//! let tz_str = iana_time_zone::get_timezone().unwrap();
//! println!("The current time zone is: {}", tz_str);
//!
//! // Convert the time zone string to a `chrono-tz::Tz` variant.
//! let tz: chrono_tz::Tz = tz_str.parse().map_err(|e| anyhow::anyhow!("Error: {}", e))?;
//! let tz: chrono_tz::Tz = tz_str.parse().unwrap();
//! println!("The current time zone is: {}", tz);
//! # Ok(())
//! # }
//! ```
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is little need to actually use chrono-tz in the example. It has open security advises, increases the build time, and increases the MSRV to 1.40, when the library itself could be 1.38.

@astraw astraw merged commit 9280e0d into strawlab:main Apr 15, 2022
@astraw
Copy link
Member

astraw commented Apr 15, 2022

The argument that tokio uses winapi is enough for me. And indeed the constant breaking changes in windows (although it has slowed lately) is a bit tiring. There are also many other nice things here and in the absence of a reason to really need a recent rust compiler, I agree it is just nicer to support older versions.

@Kijewski Kijewski deleted the pr-winapi branch April 15, 2022 19:49
@lopopolo lopopolo mentioned this pull request Sep 23, 2022
@lopopolo lopopolo added dependencies Pull requests that update a dependency file Tier-1 Rust Tier-1 platform labels Oct 1, 2022
@niyaznigmatullin
Copy link

It wasn't mentioned here I hope you are aware, but there is windows-sys crate with MSRV 1.49 and is lightweight version of windows crate, which is maintained unlike winapi.

@Kijewski
Copy link
Collaborator Author

When you look at the history of their Cargo.lock, you'll see that they randomly bump their msrv every other release. It's still quite an old rust version they need, but I don't think their release pattern makes the windows-sys crate any more useful than the windows crate itself.

@niyaznigmatullin
Copy link

You are right about that. To defend the crate, the maintainers seem to have some thoughts on this matter: microsoft/windows-rs#1987

(more of a joke: considering only this point, winapi is certainly better, because they will never bump their MSRV, since there will be no release in the future :))

Kijewski added a commit to Kijewski/iana-time-zone that referenced this pull request Jan 6, 2023
[Some time ago] I spoke in favor of using [`winapi`] instead of
[`windows-sys`], because it had fewer msrv bumps, and because it was
used by e.g. `tokio`, and `chrono`, so even if it was not being
developed for anymore, it still had a ton of users who would hopefully
notice any bugs.

By now `tokio` has switched to `windows-sys`, and `chrono` will too in
the next release. I guess it's time then to make the switch, too.

[Some time ago]: strawlab#35
[`winapi`]: https://crates.io/crates/winapi
[`windows-sys`]: https://crates.io/crates/windows-sys
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file Tier-1 Rust Tier-1 platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants