wgpu icon indicating copy to clipboard operation
wgpu copied to clipboard

[d3d12 wgl] Upgrade to `windows 0.59` crates

Open MarijnS95 opened this issue 11 months ago • 15 comments

Connections Depends on https://github.com/Traverse-Research/gpu-allocator/pull/258 and https://github.com/Xudong-Huang/generator-rs/pull/72.

https://github.com/microsoft/windows-rs/releases/tag/0.61.0

Description The latest windows 0.59 and windows-core 0.59 crates were just released (strangely tagged 0.61), including some minor code improvements for us. The MSRV has been bumped to 1.74, but wgpu is already on 1.76 anyway.

Testing Not yet, only cross-compiled.

Checklist

  • [x] Run cargo fmt.
  • [x] Run taplo format.
  • [x] Run cargo clippy. If applicable, add:
    • [ ] --target wasm32-unknown-unknown
    • [ ] --target wasm32-unknown-emscripten
  • [ ] Run cargo xtask test to run tests.
  • [ ] Add change to CHANGELOG.md. See simple instructions inside file.
  • [ ] Replaced crate patches with actual releases.

MarijnS95 avatar Jan 08 '25 11:01 MarijnS95

Just to set expectations here, we likely can't merge this for a bit, as firefox is currently on 0.58 and migration will be a bit of a thing. @ErichDonGubler will take charge of this.

I don't expect there to be a ton of problems with this PR just hanging out for a while. I'm going to mark this as draft as we can't merge it, but @Wumpf will review it still.

cwfitzgerald avatar Jan 08 '25 19:01 cwfitzgerald

@cwfitzgerald that's all good, I don't expect this to be merged until the upgrade has fully trickled through the ecosystem. Just making sure we have a PR open with all the necessary changes that I couldn't make in the original windows-rs migration due to missing or incorrect upstream annotations.

MarijnS95 avatar Jan 09 '25 19:01 MarijnS95

There are a few things that I'm already aware need to happen in mozilla-central before this can land:

  • [ ] Dependencies on windows
    • [ ] Bump gpu-allocator after a version releases that consumes https://github.com/Traverse-Research/gpu-allocator/pull/258.
      • We might need this backported to 0.27.0, since mainline upstream now uses objc2-metal, which we're blocked on moving to because of #5641 (CC @Jasper-Bekkers, @MarijnS95).
    • [x] mtu migrated from windows-{bindgen,core} and windows-targets to windows >=0.58.0,<0.60.0: https://github.com/mozilla/mtu/pull/65
  • [ ] Dependencies on windows-sys
    • [x] mio: https://github.com/tokio-rs/mio/pull/1857
    • [ ] Coordinate upgrades with folks working on neqo: https://github.com/mozilla/neqo/pull/2662
    • [x] audio_thread_priority needs migration: https://github.com/mozilla/audio_thread_priority/pull/30
    • [x] ~~tempfile can have its version range extended: https://github.com/Stebalien/tempfile/pull/319~~ Actually already works, but it has a nitpick issue with its range specification.
    • [x] socket2 can have its version range extended:
      • https://github.com/rust-lang/socket2/pull/545
      • https://github.com/rust-lang/socket2/pull/579
    • [x] tokio needs migration: https://github.com/tokio-rs/tokio/pull/7117
      • [x] is-terminal can have its version range extended: https://github.com/sunfishcode/is-terminal/pull/43
  • [ ] Direct dependencies on windows-targets
    • [x] libloading can widen its accepted ranges of windows-targets to include 0.53.*: https://github.com/nagisa/rust_libloading/pull/169
    • [ ] backtrace: consume a new libloading and update workspace: https://github.com/rust-lang/backtrace-rs/pull/694
    • [ ] parking_lot_core: consume a new backtrace and update workspace: https://github.com/Amanieu/parking_lot/pull/458

ErichDonGubler avatar Jan 10 '25 03:01 ErichDonGubler

Regarding neqo and mtu, we can bump whenever you're ready - just let us know. (CC @mxinden for info).

larseggert avatar Jan 10 '25 06:01 larseggert

Regarding neqo and mtu, we can bump whenever you're ready - just let us know. (CC @mxinden for info).

Under the assumption that windows v0.59 does not introduce a breaking change for neqo or mtu, we can also make neqo and mtu compatible to both windows v0.58 and v0.59 by configuring a range:

windows-sys = { version = ">=0.58, <=0.59"

See https://github.com/quinn-rs/quinn/pull/2021/ as an example from one of our upstream dependencies.

@ErichDonGubler let me know what works best for you.

mxinden avatar Jan 10 '25 09:01 mxinden

@mxinden: If a range dependency works for neqo and mtu, then wonderful, let's please do that. 😀 Did you want me to file PRs?

ETA: Filed! See the comment (https://github.com/gfx-rs/wgpu/pull/6876#issuecomment-2581684258) for details.

ErichDonGubler avatar Jan 10 '25 20:01 ErichDonGubler

https://github.com/mozilla/mtu/pull/65 has landed! 🙌🏻

ErichDonGubler avatar Jan 21 '25 15:01 ErichDonGubler

I've updated my dependency index comment to be organized by direct dependency. I've filed the PRs that I'm aware are necessary in the larger Crates.io ecosystem, and am now ready (but not yet trying in earnest) to try compiling mozilla-central with the updated deps.

ErichDonGubler avatar Jan 21 '25 20:01 ErichDonGubler

@ErichDonGubler I think we can arrange this pretty quickly, let us know when the time is there (and you haven't yet upgraded to objc2-metal by then).

MarijnS95 avatar Jan 27 '25 17:01 MarijnS95

@ErichDonGubler any update on the Mozilla side? looks like a lot of the PRs you referenced are landed by now

Wumpf avatar May 24 '25 09:05 Wumpf

Sorry I completely dropped the ball on this and there are quite a few new versions already. I'll get back to the second bump ASAP, as long as Mozilla is okay with the next bumps?

MarijnS95 avatar May 24 '25 09:05 MarijnS95

@MarijnS95: The way to keep this tractable is almost certainly to keep uograde steps incremental, so don't feel like you need to throw away efforts to target 0.59 without a strong concrete blocker.

ErichDonGubler avatar May 24 '25 13:05 ErichDonGubler

@Wumpf: Some PRs have landed, and some have released, but many of them have not been exposed in a Crates.io release yet. I'm checking those boxes as releases become available on Crates.io, not when the PRs land.

ErichDonGubler avatar May 24 '25 13:05 ErichDonGubler

@ErichDonGubler It is expected that the upgrade to 0.61 is incremental on top of 0.59, nothing to be thrown away. But this PR is definitely in need of a rebase and conflict-resolve.

MarijnS95 avatar May 24 '25 20:05 MarijnS95

@ErichDonGubler you can cross off mio which released 1.0.4 with the bump just 6 hours ago (tokio also released, but the bump wasn't merged yet).

MarijnS95 avatar May 24 '25 20:05 MarijnS95

I'm a little confused by the latest CI error https://github.com/gfx-rs/wgpu/actions/runs/16555966628/job/46817429146?pr=6876:

error: failed to select a version for `libc`.
    ... required by package `socket2 v0.6.0`
    ... which satisfies dependency `socket2 = "^0.6.0"` of package `tokio v1.47.0`
    ... which satisfies dependency `tokio = "^1.47"` of package `cts_runner v26.0.0 (/home/runner/work/wgpu/wgpu/cts_runner)`
versions that meet the requirements `^0.2.172` are: 0.2.174, 0.2.173, 0.2.172

all possible versions conflict with previously selected packages.

  previously selected package `libc v0.2.168`
    ... which satisfies dependency `libc = "^0.2.168"` of package `wgpu-hal v26.0.0 (/home/runner/work/wgpu/wgpu/wgpu-hal)`
    ... which satisfies path dependency `wgpu-hal` of package `wgpu v26.0.0 (/home/runner/work/wgpu/wgpu/wgpu)`
    ... which satisfies path dependency `wgpu` of package `wgpu-benchmark v26.0.0 (/home/runner/work/wgpu/wgpu/benches)`

failed to select a version for `libc` which could resolve this conflict

This PR only bumps socket2 from 0.5.10 to 0.6 via tokio 1.47. That 0.5.10 already had a libc 0.2.171 bound, which should similarly be incompatible with that "previously selected libc 0.2.168" for wgpu-hal. However, it seems then the resolver understood it's an open-ended dependency and wgpu-hal can trivially use libc 0.2.172?

The solution is probably simple, just bump our dep to libc = "0.2.172", I just feel like understanding why it didn't need to be libc = "0.2.171" on trunk already.

MarijnS95 avatar Jul 28 '25 11:07 MarijnS95

This is a really weird error that you hit with -Zdirect-minimal-versions. When you have a dependency that has a higher requirement than you do, instead of resolving it to that higher version, it throws an error as it's trying to lock your immediate versions to exactly the versions you need. This may end up being a unnecessary requirement, but would require downgrading a transitive dependency, which -Zdirect-minimal-versions won't do.

cwfitzgerald avatar Jul 28 '25 13:07 cwfitzgerald

Never mind, I was mistaken. While I said that socket2 0.5.10 requires libc 0.2.171 already, tokio 1.64.1 only requires socket2 0.5.5 which requires libc 0.2.149, so the requirement is satisfied when -Zdirect-minimal-versions sets it at libc 0.2.168 for wgpu-hal.

MarijnS95 avatar Jul 28 '25 15:07 MarijnS95

@ErichDonGubler how are we doing here, mozilla side?

cwfitzgerald avatar Aug 27 '25 20:08 cwfitzgerald

@cwfitzgerald: The dependency status index comment is up-to-date, as far as I know. Some upstream PRs have stalled, and now some of the deps. have even started to move on to later windows versions. There's also some complication with some upstream movement to windows-link, instead of windows-sys.

I think we might benefit from retrying with the latest windows crate ecosystem, rather than trying to continue with 0.59.

ErichDonGubler avatar Sep 04 '25 07:09 ErichDonGubler

I think we might benefit from retrying with the latest windows crate ecosystem, rather than trying to continue with 0.59.

@ErichDonGubler with that, are you referring to 0.61 from https://github.com/gfx-rs/wgpu/pull/8013 or the latest 0.62 release which barely misses out on range compatibility due to https://github.com/gfx-rs/wgpu/pull/8013#issuecomment-3271792993?

MarijnS95 avatar Sep 09 '25 18:09 MarijnS95

We dropped gpu-allocator 0.28 :)

MarijnS95 avatar Sep 29 '25 09:09 MarijnS95

@MarijnS95:

I think we might benefit from retrying with the latest windows crate ecosystem, rather than trying to continue with 0.59.

@ErichDonGubler with that, are you referring to 0.61 from #8013 or the latest 0.62 release which barely misses out on range compatibility due to #8013 (comment)?

I'm referring to 0.62, sorry for any confusion!

Much of the ecosystem has caught up with 0.62, with some refusing to use range dependencies. I've also confirmed that we can build it in Firefox, done all the auditing work for new dependencies. The main supply chain task is some upstreaming before review that can be done incrementally per-dependency (see Bug 1991226 - Upgrade/audit most Rust deps. for WGPU revendor and windows 0.62, part 1).

I'll let you judge for yourself, though. I've put a PR for 0.62 up—based on work here—for your perusal: https://github.com/gfx-rs/wgpu/pull/8281

ErichDonGubler avatar Sep 30 '25 19:09 ErichDonGubler

Much of the ecosystem has caught up with 0.62, with some refusing to use range dependencies.

As already highlighted in https://github.com/gfx-rs/wgpu/pull/8013#issuecomment-3271792993 three whole weeks ago wgpu itself cannot take a range dependency because of the Error::from_win32() -> Error::from_thread() rename.

I'll let you judge for yourself, though. I've put a PR for 0.62 up—based on work here—for your perusal: https://github.com/gfx-rs/wgpu/pull/8281

I thought I had also already shared https://github.com/MarijnS95/wgpu/compare/windows-69 with you those three weeks ago, and was merely waiting for confirmation to push it into this PR or #8013.

MarijnS95 avatar Sep 30 '25 19:09 MarijnS95

Note that all these force-pushes to contributor PRs -in particular after such a long time of radio silence- seem a little disingenuous: I cannot even read the CI failure and git rebase --update-refs to amend the fixes into multiple PRs at once, before having git push --force-with-lease tell me that my remote branch changed since checkout? Would be awesome if you could ask next time whether help is needed 🙏

MarijnS95 avatar Sep 30 '25 19:09 MarijnS95

As already highlighted in #8013 (comment) three whole weeks ago wgpu itself cannot take a range dependency because of the Error::from_win32() -> Error::from_thread() rename.

Sure, but that only means WGPU specifically suffers from being an all-or-nothing upgrade WRT this windows upgrade. It's still beneficial for WGPU's dependencies to be incrementally upgradeable, because incremental upgrades at Mozilla have a significantly lower risk of getting stuck. If you examine bug 1991226, that's exactly what I'm doing.

When you submitted that comment three weeks ago, this was, unfortunately, not something I could justify looking at (and I'm still not sure that I can drive this to completion, based on Mozilla's current priorities). I'm trying to split a bit of attention to take a look at this now, though, because I think it's important for Mozilla to have concrete reasons for either letting things through or being blocked, rather than "I don't got bandwidth indefinitely, soz."

ErichDonGubler avatar Sep 30 '25 19:09 ErichDonGubler

Note that all these force-pushes to contributor PRs -in particular after such a long time of radio silence- seem a little disingenuous: I cannot even read the CI failure and git rebase --update-refs to amend the fixes into multiple PRs at once, before having git push --force-with-lease tell me that my remote branch changed since checkout? Would be awesome if you could ask next time whether help is needed 🙏

Ah, sorry, I'm not trying to create obstacles for you! I'm happy to switch to appending fixup!s, or giving you diffs via PRs. Force-pushing generally works very well with less dedicated contributions, but you've been quite dedicated. ❤️ Happy to accommodate a better workflow here!

ErichDonGubler avatar Sep 30 '25 19:09 ErichDonGubler

I thought I had also already shared https://github.com/MarijnS95/wgpu/compare/windows-69 with you those three weeks ago, and was merely waiting for confirmation to push it into this PR or https://github.com/gfx-rs/wgpu/pull/8013.

I don't remember this being shared, but I think it's safe to say we've reached consensus for trying to target windows 0.62. I'm fine dismissing #8281, if you intend to reconcile it with #8013.

ErichDonGubler avatar Sep 30 '25 19:09 ErichDonGubler

Sure thing, having downstream dependencies commit to ranges (when possible) would be awesome and I've been trying to sneak them into crates whenever possible. Thus happen to contribute to quite a few too many, and it usually means preempting the maintainer or other contributors who are oblivious to this behavior (and getting the range extended backwards has thus far proven harder).

Hadn't looked at the Bugzilla tracker but it looks like a lot of crates to repeatedly vet; any way that could be reduced by less aggressively bumping patch versions where not necessary for e.g. this Windows upgrade?

Glad to see that you're at least trying to get something going here despite not finding time/priority. Apologies for the hasty response. Makes me fear the ever-increasing stack of PRs, issues and (mostly) complaints on popular repos that I'm supposed to be maintaining, ... 🤷

I'll happily keep #8013 up to date and rebased on this PR, and leave it to you whether to temporarily jump to 0.59 via this PR or go straight to 0.62. Since most context lives here, I can merge it into this PR too.

MarijnS95 avatar Sep 30 '25 19:09 MarijnS95

I'd like to address this tension specifically, since I consider cultural gardening an important exercise:

Note that all these force-pushes to contributor PRs -in particular after such a long time of radio silence- seem a little disingenuous: ...

As I'm sure you know, being radio-silent generally isn't a problem for one-off contributions. This case is already different; IMO, you've definitely earned some negotiating power on how you'd like to communicate, given how much (welcome) work you've been doing. Thoughts? I'm guessing you would have preferred that we told you specifically that we knew we didn't have immediate bandwidth for windows upgrades. Alternatively, we don't mind getting pinged in issues like this if and when we do fail to communicate, though responsiveness can vary between Mozillians and from week to week. Sometimes, we have week-or-longer interruptions (like the recent WebGPU F2F, or Mozilla all-company meetings).

☝🏻 CC @gfx-rs/wgpu.

ErichDonGubler avatar Sep 30 '25 20:09 ErichDonGubler