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

[wgpu-hal] gles/wgl: Migrate from ancient/unmaintained winapi to windows-rs #6006

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Jul 21, 2024

Connections
Contributes to #3207
Same as PR #5956 but for GLES/WGL, instead of the Dx12 backend.

Description
Replaces ancient, deprecated, unmaintained winapi crate with windows-rs. The full justification is in the linked issue and PR above.

Testing

WGPU_BACKEND=gles cargo run --bin wgpu-examples shadow

Checklist

  • Run cargo fmt.
  • 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.

@MarijnS95 MarijnS95 requested a review from a team as a code owner July 21, 2024 20:21
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.

Great stuff, thank you so much for taking care of the windows-rs migration everywhere!

Imho good to go after the workspace dependency nit (see comment)

CHANGELOG.md Show resolved Hide resolved
wgpu-hal/Cargo.toml Outdated Show resolved Hide resolved
Cargo.lock Show resolved Hide resolved
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

See comment above

@cwfitzgerald
Copy link
Member

I suspect this windows version dance is going to be a recurring theme in our future. That is one upside off using a years-abandoned crate, nothing ever changes 😛

Anywho I think we (the maintainers) should figure out what the viable paths forward are that satisfy mozilla-central and wgpu's external users and then clearly communicate that, as I don't want Marijn to do duplicate work and I don't want this to explode mozilla-central.

  • Ideally we can figure out a way to land 0.57/58. It's not improbable that nothing else in the stack breaks and mozilla-central can just update, though I know there will be politics around that
  • if not, dropping back and supporting as wide of a range as we can, to minimize duplicate dependencies. We should earmark Marjin's improvements, so when et do update, we can re-implement them.
  • loom is indeed irrelevant
  • if there is no range beyond 0.52

Copy link
Contributor

@glandium glandium left a comment

Choose a reason for hiding this comment

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

No problem for me to upgrade to windows 0.58.

@MarijnS95
Copy link
Contributor Author

@cwfitzgerald since we've made the resolution to just go ahead and use windows 0.58 directly (it seems), can we get reviews finished? Landing this PR first will make it much easier and cleaner to rebase #5956 on top.

@MarijnS95 MarijnS95 changed the title gles/wgl: Migrate from ancient/unmaintained winapi to windows-rs [wgpu-hal] gles/wgl: Migrate from ancient/unmaintained winapi to windows-rs Jul 24, 2024
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Great stuff! Got the all clear to merge from the moz team.

@cwfitzgerald
Copy link
Member

Ready to land, just needs merge conflicts resolved

@teoxoy teoxoy merged commit 2611d18 into gfx-rs:trunk Jul 25, 2024
25 checks passed
@MarijnS95 MarijnS95 deleted the gles-wgl-windows-rs branch July 25, 2024 09:16
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.

7 participants