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 #893

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cart
Copy link

@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 cpal compiles with 0.54, 0.56 (0.55 does not exist), and 0.57.

@@ -26,7 +26,7 @@ clap = { version = "4.0", features = ["derive"] }
ndk-glue = "0.7"

[target.'cfg(target_os = "windows")'.dependencies]
windows = { version = "0.54.0", features = [
windows = { version = ">=0.54, <=0.57.0", features = [
Copy link

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.0", 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.0", features = [

This is copied from my review of GuillaumeGomez/sysinfo#1295. I don't want the reasoning behind this change to be lost.

@Ralith
Copy link
Contributor

Ralith commented Jun 27, 2024

I think this needs CI checking compatibility with the oldest listed version to not be a liability.

@BD103
Copy link

BD103 commented Jun 27, 2024

I think this needs CI checking compatibility with the oldest listed version to not be a liability.

The windows-test job could be modified with a matrix to test this. I know its possible on to resolve the minimum versions of all dependencies on nightly, though I don't know if you can resolve just a single dependency. (Perhaps you need to modify the lockfile or patch in the version?)

@est31
Copy link
Member

est31 commented Jun 30, 2024

These ranges are a good idea to the too many windows versions problem, so good catch!

It's kinda outsourcing the problem of checking for compatibility to the community, but still better than having multiple versions of windows in your crate graph.

I agree with @Ralith, there should be CI checks for all of the supported windows versions, and we'd probably still only want to support a limited amount to keep the number of CI checked versions down (but ideally 3-4 different versions would sufficiently avoid duplication). You'd probably do something like cargo update --precise to get the needed windows version.

@BD103
Copy link

BD103 commented Jun 30, 2024

You'd probably do something like cargo update --precise to get the needed windows version.

I tried using cargo update --precise, but I had some issues with duplicate dependencies. The one solution that worked for me was using patch files, though I'm sure Regex would be more stable.

For reference, here's my demo that automatically tests windows versions for accesskit.

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

4 participants