AccessKit / accesskit

Accessibility infrastructure for UI toolkits
https://accesskit.dev
BSD 3-Clause "New" or "Revised" License
1.05k stars 53 forks source link

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

Closed cart closed 1 week ago

cart commented 3 months ago

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

mwcampbell commented 3 months ago

@cart Thanks for the contribution!

The only question now is whether we should leave our Cargo.lock file on 0.54, as you've apparently done, or update it to 0.57. I guess the advantage of leaving it on the old version is that we can make sure we don't break support for that minimum supported version. @DataTriny What do you think?

cart commented 3 months ago

Given that accesskit is a library, I'm pretty sure you shouldn't be checking in a Cargo.lock in the first place?

mwcampbell commented 3 months ago

The guidance on committing Cargo.lock changed last year. But even before it changed, we committed Cargo.lock, because we have build artifacts (the C and Python bindings), and we want predictability and reproducibility for those.

cart commented 3 months ago

Ahhhhh I was just told that this doesn't affect downstream consumers. So that shouldn't affect this effort. Carry on :)

DataTriny commented 3 months ago

I wouldn't want to merge this without changes to CI as well, and it's not obvious to me how you could compile accesskit_windows with different versions of the windows crate. It is likely that we'll have a similar issue with the objc2* crates, so I don't think this approach will scale well.

While I understand the convenience of this, I think it is bad practice (and potentially dangerous) to do so, especially with a pre 1.0 crate.

I'd rather push for more automated dependency updates and crate publishing so that everyone is on the latest version.

mwcampbell commented 3 months ago

I want us to go ahead with this change. The AccessKit project has to go out of its way to fit into the larger ecosystem so developers have no excuse for not implementing accessibility. When the issue with zbus code size and dependency count came up earlier, I didn't see a clear way to resolve it that would please everyone. But in this case, if we have to add more to our CI to test with different windows-rs versions, then I think we should. I guess we could have CI steps that manipulate our Cargo.lock file to compile with different windows-rs versions, basically doing the same thing that @cart did manually.

alice-i-cecile commented 3 months ago

@BD103 may have advice on the CI side of this.

BD103 commented 3 months ago

@BD103 may have advice on the CI side of this.

I have an unfinished demo here that showcases what this may look like. It uses sed to pin the windows version, then runs the test suite. (I'm inexperienced with sed, so maybe someone else will be able to figure out why it's only setting the windows-core version and not windows.)

DataTriny commented 1 week ago

Superseded by #453 in which we've updated to 0.58. This is a breaking change so an hypothetical version range would have to start from 0.58 for us.

DataTriny commented 1 week ago

Feel free to open a new PR here @cart with CI checks. I'll close this one. Thank you.