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

Adopt windows version range >=0.54, <=0.57 #1295

Closed
wants to merge 1 commit into from

Conversation

cart
Copy link
Contributor

@cart cart commented Jun 26, 2024

windows is a "heavy" crate. Duplicate windows versions in a tree can add precious seconds to compile times (or even minutes by some Bevy user reports). On the Bevy side, we are trying to ensure that windows is only compiled once. We also believe it is in the ecosystem's best interest (and in the interest of crate owners that consume windows) to align their versioning with the rest of ecosystem. Therefore, I would like to encourage crate owners to adopt the following approach to windows crate usage:

  1. Adopt "range versioning" for windows instead of locking to a specific version.
  2. Before adding a new version to the range, test that each previous version in the range still compiles.
  3. Only remove old versions from the range when absolutely necessary. Try your hardest to ensure that your version support aligns with other ecosystem crates.

We've picked 0.54 as a baseline because breaking API changes were made to use Rust Results instead of HRESULT.

I have personally tested that sysinfo compiles with 0.54, 0.56 (0.55 does not exist), and 0.57.

@GuillaumeGomez
Copy link
Owner

GuillaumeGomez commented Jun 26, 2024

Won't the Cargo.lock reduce this effort to nothing? If so I think it's now safe to remove it as I think cargo now takes into account the MSRV indicated in the Cargo.toml file.

EDIT: Just to be clear: I agree with this change and will happily merge it. :)

@cart
Copy link
Contributor Author

cart commented Jun 26, 2024

Hmmm yeah libraries shouldn't check in cargo.lock as that makes coordination across contexts like this impossible. I'm not familiar with the issue that resulted in you needing to check it in, but it would be much appreciated if you removed it. Especially if it is no longer necessary 😄

@cart
Copy link
Contributor Author

cart commented Jun 26, 2024

Ah I just got educated on https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html and have been told that this does not affect downstream consumers. So I guess I don't care if you keep or remove it / that shouldn't affect us.

@@ -66,7 +66,7 @@ serde = { version = "^1.0.190", optional = true }

[target.'cfg(windows)'.dependencies]
ntapi = "0.4"
windows = { version = "0.56", features = [
windows = { version = ">=0.54, <=0.57", features = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
windows = { version = ">=0.54, <=0.57", features = [
# Support a range of versions in order to avoid duplication of this crate. Make sure to test all
# versions when bumping to a new release, and only increase the minimum when absolutely necessary.
windows = { version = ">=0.54, <=0.57", features = [

I think a few comments here will help future contributors. :)

Copy link
Owner

Choose a reason for hiding this comment

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

Indeed! That'd be very welcome. :)

@GuillaumeGomez
Copy link
Owner

Ping @cart (just saw the bevy release, congrats!).

@GuillaumeGomez
Copy link
Owner

I made the change in #1326 and added you both as co-authors. Closing this one.

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.

None yet

3 participants