-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix compilation for targets that don't have AtomicU64. #7118
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
Conversation
AtomicU64 is not available in PowerPC 32-bit and other targets. When that happens, replace it with the implementation in portable-atomic.
|
One concern from Mozilla's end here is that CC @glandium for guidance here; I'm going to check to see if consuming this PR causes problems. Clicking |
ErichDonGubler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝🏻
|
I will note that we can put it behind a feature still, so mozilla does not get it in tree at all. |
|
Just tested and confirmed locally that this would add |
|
Tried it, and it still appears in the wgpu Using a feature also changes the scope of the change from "leave everything that is working untouched, add support for extra targets" to "replace AtomicU64 if the user wants, even for targets that already work". I think it is better to not use a feature and just wait for mozilla to do it's homework. |
|
Sorry this was poorly explained by us as we actually have a we should have a feature inside Making it a feature like this is actually will add it to our
This is a pretty fundamental thing of how cargo-vet and resolution work, so any extra dependencies, especially unsafe ones, create a large vetting burden on those who have strict protections against supply chain attacks. |
Instead of including the dependency to replace a missing AtomicU64, it replaces AtomicU64 when the portable-atomic feature is enabled. This change was made to accomodate cargo-vet and resolution work.
|
I don't fully understand (lack of domain knowledge), but if it indeed fixes the problem then sure I'll do it. Not sure what would be a good name so I set it to the name of the dependency. |
|
We've agreed that we're going to take this once we validate that portable-atomics isn't pulled into mozilla-central. Will re-review this soon. |
cwfitzgerald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a few more tweaks needed on the cfgs - if anything is unclear just shout :)
Working on getting clearance that this won't affect mozilla central.
|
I can confirm that, when consumed, the current contents of this PR do not add another dependency for Firefox. In that way, LGTM! I'll leave the rest of review to @cwfitzgerald. |
# Conflicts: # wgpu-core/Cargo.toml # wgpu-core/src/device/resource.rs # wgpu-hal/Cargo.toml # wgpu-hal/src/lib.rs # wgpu-hal/src/metal/device.rs # wgpu-hal/src/metal/mod.rs # wgpu/Cargo.toml # wgpu/src/cmp.rs
cwfitzgerald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
Connections
Fixes #5997
Description
AtomicU64 is not available in PowerPC 32-bit and other targets.
When that happens, replace it with the implementation in portable-atomic.
Please backport this to older versions, in my case I need it in v0.20 for MartyPC.
A binfmt-based solution with a ppc32 chroot takes 20-30 times longer to compile so I don't recommend it in CI.
I don't have enough knowledge to try a cross compile that only uses binfmt for testing.
At minimum, I recommend adding a simple "can compile" test with a ubuntu runner to avoid bit rot:
Testing
I do not have a GUI setup in my vm so testing was limited.
I used a linux x86_64 host and a PowerPC 32-bit qemu vm with ArchPOWER.
This is the command line that my script invokes to start the vm in normal runs:
These naga tests fail:
It seems to be related to the difference in pointer size and safe to ignore.
I recommend adding linux x86 to the CI, maybe with Alpine Linux:
Checklist
cargo fmt.taplo format.cargo clippy. If applicable, add:--target powerpc-unknown-linux-gnucargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.