-
-
Notifications
You must be signed in to change notification settings - Fork 17.5k
maturin: Sorts RECORD file in wheel archives to make them deterministic #397290
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
5c8f788 to
3e16215
Compare
e89674b to
8e8365c
Compare
raboof
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.
As you say in #384708 (comment) it's a dilemma whether we should apply this fix 'surgically' to packages that are known to be affected, or to the 'global' setuptools package.
I think applying it 'surgically' in nixpkgs while discussing it for 'global' inclusion upstream might be a reasonable start.
pkgs/development/python-modules/pydantic-core/wheel-deterministic-record.patch
Outdated
Show resolved
Hide resolved
Thanks! That sounds like a good approach to me—Would you like me open an upstream issue or PR to start the conversation? |
c803168 to
4f42425
Compare
Yeah go for it! |
|
The wheel builder in this case would be https://github.com/PyO3/maturin. |
Really? But then that patch shouldn't have had any effect, right? As that seems to patch https://github.com/pypa/wheel/blob/main/src/wheel/wheelfile.py#L232 ? |
|
Not sure what modifying the build system like that will do, but the build does in fact use maturin. No visible effect. |
Thank you for the feedback @mweinelt, I'll look again into the source and maturin to see what actually causes the ordering differences. |
|
So I checked the source and didn’t find anything suspicious there. Then I decided to look into other Nix packages that use the maturin hook to see if they had the same ordering issue—and I confirmed that at least four other packages show the same problem. I didn’t keep going after that since it was clear the issue likely originates from maturin. I took a look at the maturin source code (I'm not very familiar with Rust), but I tried patching #L156 and #L392 to sort the I'll push my update shortly and also propose this in a pr upstream |
4f42425 to
132ae2a
Compare
|
@raboof since this issue affects other packages, I don't this change should be in this pr right? it should be on the maturin package itself. Also I don't think I'm applying this patch the right way and it looks like the build system is deceiving me just like when I patched the python setuptools wheel. If you notice I commented out the sort commands in the patch but somehow this ends up building with no differences. |
raboof
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.
since you verified other modules that use maturin are also affected, it might indeed make more sense to create a PR against the maturin package.
pkgs/development/python-modules/pydantic-core/wheel-deterministic-record.patch
Outdated
Show resolved
Hide resolved
132ae2a to
47b0ed9
Compare
47b0ed9 to
405e953
Compare
|
Since this PR now causes many rebuilds, you'll likely want it to target staging. For more information, see https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#changes-causing-mass-rebuilds |
|
Thanks for submitting this upstream. I think we're not in a hurry to land this in 25.05, so we can wait for some review upstream. |
|
Hi @raboof, the upstream pr has been merged :) |
405e953 to
2f9995f
Compare
that is awesome, congratulations! since AFAIK none of the packages in our milestones (ISO runtime closures) are affected by this issue, I agree with @mweinelt that we're not in a particular rush to get this in. Upstream seems to be making regular releases, so we could consider just waiting for that release. Up to the maintainer (@getchoo) I'd say. |
Thank you ❤️
Okay no problem at all. I've include a comment that the patch should be removed on the next version bump |
getchoo
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.
Otherwise LGTM. Thanks for getting this upstreamed!
2f9995f to
7b88be4
Compare
getchoo
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 again!
nixpkgs-review result
Generated using nixpkgs-review.
Command: nixpkgs-review pr 397290 --package python312Packages.cryptography --package maturin
x86_64-linux
✅ 3 packages built:
- maturin
- python312Packages.cryptography
- python312Packages.cryptography.dist (python312Packages.cryptography.dist.dist)
|
Successfully created backport PR for |
|
Thank you all for your help with this. I'm excited to see this get merged! 🥹🙏 |
Things done
Should resolve #384709
@raboof please help review
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.