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

build: use windows-{core,bindgen} 0.59, windows-targets 0.53 #65

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

Conversation

ErichDonGubler
Copy link

@ErichDonGubler ErichDonGubler changed the title WIP: build: allow windows-core 0.59, windows-targets 0.53 build: allow windows-core 0.59, windows-targets 0.53 Jan 10, 2025
@ErichDonGubler ErichDonGubler changed the title build: allow windows-core 0.59, windows-targets 0.53 build: use windows-{core,bindgen} 0.59, windows-targets 0.53 Jan 10, 2025
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.28%. Comparing base (63f322f) to head (8b708e3).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #65   +/-   ##
=======================================
  Coverage   86.28%   86.28%           
=======================================
  Files           5        5           
  Lines         554      554           
  Branches      554      554           
=======================================
  Hits          478      478           
  Misses         48       48           
  Partials       28       28           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ErichDonGubler
Copy link
Author

Not sure what the Linux-only nightly debug failure is, but it's something to do with LSAN. 😬 I don't think I have the bandwidth to figure it out right this moment, though.

@ErichDonGubler
Copy link
Author

This PR only upgrades windows-* dependencies, though @mxinden had invited me to make them a range dependency instead. The reason I didn't is that while things seem source-compatible between versions of windows-core releases, windows-bindgen's output supposedly changes significantly between the 0.58 and 0.59 versions. I wasn't sure if it was safe to use a range in that case.

It seems that bindgen is/was used to avoid bringing in the windows crate, but I think that should actually be fine to use now in mozilla-central. Was there another reason not to use the windows crate directly? If not, we might be able to use a range over the windows crate itself.

@mxinden
Copy link
Collaborator

mxinden commented Jan 13, 2025

Not sure what the Linux-only nightly debug failure is, but it's something to do with LSAN. 😬 I don't think I have the bandwidth to figure it out right this moment, though.

Will be fixed with #69.

@mxinden
Copy link
Collaborator

mxinden commented Jan 13, 2025

It seems that bindgen is/was used to avoid bringing in the windows crate, but I think that should actually be fine to use now in mozilla-central. Was there another reason not to use the windows crate directly? If not, we might be able to use a range over the windows crate itself.

@larseggert can you explain why bindgen is used?

@@ -48,3 +48,6 @@ gecko = ["dep:mozbuild"]
cargo = { level = "warn", priority = -1 }
nursery = { level = "warn", priority = -1 }
pedantic = { level = "warn", priority = -1 }

[patch.crates-io]
libloading = { git = "https://github.com/erichdongubler-contrib/rust_libloading", branch = "windows-0.59" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the record: nagisa/rust_libloading#169

@larseggert
Copy link
Collaborator

In earlier versions, we tried to avoid pulling in the other largerwindows crates by having a static Rust binding file for the few Windows APIs we called, together with a CI test that called bindgen to detect any changes.

We abandoned that at some point, and converted to calling bindgen during build. I don't think there was any reason other than it was a simply change.

If we can avoid bindgen by just using the other windows crates, let's do so, esp. if it makes integration easier.

@larseggert
Copy link
Collaborator

@ErichDonGubler I don't seem to have the ability to rebase your PR; did you maybe not give owners the relevant permission?

@ErichDonGubler
Copy link
Author

ErichDonGubler commented Jan 13, 2025

@larseggert: Huh, that's strange, I'm not sure why you can't rebase. I don't see the Allow maintainers to edit this PR checkbox, but that's normally what I'd check. 🤔

I've rebased using the GH UI.

@larseggert
Copy link
Collaborator

@ErichDonGubler Thanks for rebasing. Would you want to switch us from the bindgen dep to the other windows deps in this PR?

@ErichDonGubler
Copy link
Author

@larseggert: Sure!

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.

3 participants