Skip to content

Conversation

@MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Jan 8, 2025

Connections

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

Description

Uses the latest windows 0.62 and windows-core 0.62 releases, including some minor code improvements for us. The MSRV has been bumped to 1.74, but wgpu is already on 1.76 anyway.

Testing

WGPU_BACKEND=d3d12 cargo r -p wgpu-examples hello_triangle

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • 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 MarijnS95 requested a review from a team as a code owner January 8, 2025 11:06
Comment on lines -130 to +131
return Ok(());
}
unsafe { OpenGL::wglMakeCurrent(None, None) }
unsafe { OpenGL::wglMakeCurrent(Default::default(), Default::default()) }
Copy link
Contributor Author

@MarijnS95 MarijnS95 Jan 8, 2025

Choose a reason for hiding this comment

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

@kennykerr can you help us here? I did not do my due diligence this time to search through windows-rs and win32metadata for relevant changes, but while most handles have been made optional (see other Some() wrapping changes in this PR), quite strangely these parameters are no longer optional in wglMakeCurrent() where they have a specific use-case to uncurrent the context.

Is this change intended or accidental? We can clearly get around it by passing a defaulted HDC/HGLRC, but it seems strange when compared to other functions where None can be passed.

Choose a reason for hiding this comment

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

The code generator now faithfully honors the metadata's [Optional] attribute - in the case of wglMakeCurrent the handle parameters are not optional whereas in the case of CreateSwapChainForCompositionSurfaceHandle the handle parameter is optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kennykerr. So this'll take another trip through win32metadata to annotate correctly.

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Jan 8, 2025

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 cwfitzgerald marked this pull request as draft January 8, 2025 19:39
@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jan 9, 2025

@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.

@ErichDonGubler

This comment was marked as outdated.

@larseggert
Copy link

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

@mxinden
Copy link

mxinden commented Jan 10, 2025

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 quinn-rs/quinn#2021 as an example from one of our upstream dependencies.

@ErichDonGubler let me know what works best for you.

@ErichDonGubler

This comment was marked as resolved.

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

fwiw code lgtm and took it on a spin running examples locally

@ErichDonGubler

This comment was marked as resolved.

@ErichDonGubler

This comment was marked as resolved.

@MarijnS95
Copy link
Contributor Author

@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).

@Wumpf

This comment was marked as resolved.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented May 24, 2025

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?

@ErichDonGubler
Copy link
Member

@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

This comment was marked as resolved.

@MarijnS95

This comment was marked as resolved.

@MarijnS95

This comment was marked as resolved.

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Sep 30, 2025

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?

We tend not to repeatedly vet these crates, with the recent move in wgpu from Dependabot to Renovate. It was awful before that point.

Those specific dependencies are getting updated in that patches series because IIRC some other crate required a new minimum version on the road to getting upgraded to support windows 0.62. I don't remember specifics for these, though…there were many. 😅

@ErichDonGubler
Copy link
Member

RE: the set of PRs to keep open: I think consolidating everything into this PR would make things significantly easier to follow for everyone. We don't actually intend to land things incrementally, so multiple PRs sound only like additional complication.

@MarijnS95
Copy link
Contributor Author

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.

This was mostly in respect to first being on one end of the spectrum (no response for weeks) to then jumping all the way to the other with force-pushes and respin PRs (something I tend to use sparingly when a contributor hasn't responded for weeks and I really want to get their change in after some minor CI/review fixups).


We tend not to repeatedly vet these crates, with the recent move in wgpu from Dependabot to Renovate. It was awful before that point.

Does that mean you're now skipping certain patch releases or just postponing the Renovate PRs until a vet is possible/completed?

Those specific dependencies are getting updated in that patches series because IIRC some other crate required a new minimum version on the road to getting upgraded to support `windows 0.62. I don't remember specifics for these, though...there were many. 😅

Ah, yup, https://github.com/gfx-rs/wgpu/pull/8013/files#diff-13ee4b2252c9e516a0547f2891aa2105c3ca71c6d7a1e682c69be97998dfc87e hits quite a lot of transitive bumps. Wish those were (mostly) unnecessary 😓

@MarijnS95 MarijnS95 changed the title [d3d12 wgl] Upgrade to windows 0.59 crates [d3d12 wgl] Upgrade to windows 0.62 crates Sep 30, 2025
@ErichDonGubler
Copy link
Member

ErichDonGubler commented Sep 30, 2025

This was mostly in respect to first being on one end of the spectrum (no response for weeks) to then jumping all the way to the other with force-pushes and respin PRs (something I tend to use sparingly when a contributor hasn't responded for weeks and I really want to get their change in after some minor CI/review fixups).

Ah, that wasn't intended as a respin, sorry! I had pushed branches and a temp PR already so I had a stable point of reference for making sure things were viable in Firefox. I had no illusions that I'd get that through without working with you.

The force-pushes were mostly so I could keep 0.62 changes on top of a rebased set of your patches. I promise I didn't change anything substantially! But then again...force-pushes on others' stuff still usually deserves more explanation than a Compare link, so point taken. Sorry about that! 💦

ETA: Oh, yeah, I had also force-pushed to take care of a TOML formatting failure.

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Sep 30, 2025

Does that mean you're now skipping certain patch releases or just postponing the Renovate PRs until a vet is possible/completed?

Actually, we can have our cake and eat it, too. Renovate actually implements bumping Cargo.lock without forcing a bump in Cargo.toml, so we get benefit from both upgrading and not upgrading. Combined with our MSRV/dep.-minimization checks in CI, it's actually quite nice!

@MarijnS95
Copy link
Contributor Author

I've dropped all transitive dependency upgrades now, the revert seems to mostly contain windows-sys related bumps that should be consumed via Renovate instead.

Some windows crates had a patch-release from https://github.com/microsoft/windows-rs/releases/70, we can incorporate those as long as they don't affect transitive dependencies.

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Sep 30, 2025

We'll also need microsoft/windows-rs#3785; having case-insensitive collisions b/w README.md and readme.md tripped up initial integration. That seems on track for another patch release, tho!

@MarijnS95
Copy link
Contributor Author

Oh noes, cased files on a case-insensitive filesystem. /me shudders in include vs Include used in the Vulkan SDK, I have yet to learn how to best set that up for a Windows cross-compile from a Linux machine (for which no Windows-targeted SDK tarball exists...).

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Oct 29, 2025

I'm making a new index comment here for the set of dependencies that need an upgrade before Mozilla can consume this. Fortunately, it's a rather small list!

MarijnS95 and others added 3 commits November 2, 2025 18:08
https://github.com/microsoft/windows-rs/releases/tag/0.61.0

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.
@ErichDonGubler ErichDonGubler merged commit 59a03ea into gfx-rs:trunk Nov 5, 2025
41 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in WebGPU for Firefox Nov 5, 2025
@ErichDonGubler
Copy link
Member

ErichDonGubler commented Nov 5, 2025

I'm going to merge this, because the last straggler for upgrade in Firefox (CC @larseggert) has an approved PR, and it's fairly easy to backport that PR to whatever revision base that neqo needs in Firefox. Things are looking surprisingly not painful! 👏🏻😭

@ErichDonGubler
Copy link
Member

On behalf of the entire wgpu maintainership in a meeting just now, I'd like to thank @MarijnS95 for the tireless work in this PR to keep things coordinated, and patience with my (sometimes conflicting) efforts to shepherd this forward. It's not easy to move forward an entire software ecosystem, but you've done just that. ❤️

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Nov 5, 2025

Notes from today's maintainers' meeting:

  • #6876 [d3d12 wgl] Upgrade to windows 0.62 crates
    • CF: Are we good to merge this, then?
    • EG: Yep. Merged!
    • Consensus: General noises of excitement, relief, and catharsis

@MarijnS95 MarijnS95 deleted the windows-0.59 branch November 5, 2025 17:40
ErichDonGubler pushed a commit to erichdongubler-mozilla/wgpu that referenced this pull request Nov 7, 2025
* [d3d12 wgl] Upgrade to `windows 0.59` crates

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

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.

* [d3d12 wgl] Upgrade to `windows 0.61` crates

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

* [d3d12 wgl] Upgrade to windows 0.62 crates

https://github.com/microsoft/windows-rs/releases/tag/69
ErichDonGubler added a commit to erichdongubler-mozilla/firefox that referenced this pull request Nov 13, 2025
This required a coordinated upgrade of several crates at the same time:

- Backports in `third_party/rust/`, which have been merged upstream, and should pose no
  conflict when next integrated from mainline history:
	- `midir`: [Boddlnagg/midir#178](Boddlnagg/midir#178)
	- `neqo`: [mozilla/neqo#3091](mozilla/neqo#3091)
	- `memtest`: [mozilla/memtest#18](mozilla/memtest#18)
		- Consumed by `toolkit/crashreporter/client/app/`.
	- `wgpu*` and `naga` crates: [wgpu#6876](gfx-rs/wgpu#6876)
		- This is consumed by `gfx/wgpu_bindings/`.
		- Includes an upgrade to `gpu_allocator` 0.27.0 → 0.28.0. This could have been an incremental migration first, but I missed it while figuring out what supporting work to break out. Fortunately, this isn't difficult to review.
- `widget/windows/rust/` needed some migration in its direct dependencies, too.

All backports have already been merged upstream, and so should pose on
conflict when next integrated.

Differential Revision: https://phabricator.services.mozilla.com/D266878
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants