-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Setuptools rust hook for cross compilation #212795
Conversation
pkgs/development/python-modules/setuptools-rust/setuptools-rust-hook.sh
Outdated
Show resolved
Hide resolved
pkgs/development/tools/poetry2nix/poetry2nix/overrides/default.nix
Outdated
Show resolved
Hide resolved
pkgs/development/python-modules/setuptools-rust/setuptools-rust-hook.sh
Outdated
Show resolved
Hide resolved
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.
rustc and cargo were on purpose not propagated by the cargo setup hook so that the hook also functions with different rustc cargo versions. Thus this needs to be undone.
The setup hooks are also defined separately from the package. This is for a similar reason; users should be able to use the package without having the hook running.
That is something I asked for but got no response. A drop of the package changes is easy. Updating the packages will be more complicated.
@FRidh and where they should be created actually? I found that python setup hooks are being created in |
41d7dba
to
7e42ea2
Compare
Yes, indeed there. |
7e42ea2
to
bfbc440
Compare
It took me some time to get back to it, but now it contains the hook, hopefully in the correct location, and it also fixes the cryptography build. I left out the fix of other packages this time. |
bfbc440
to
c046787
Compare
c0c3e77
to
03c2e42
Compare
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.
@Cynerd, after resolving the merge conflict python-cryptography
no longer cross-builds for me. Perhaps I resolved the conflict incorrectly; in any case please double-check that python-cryptography
cross-compiles after you resolve the conflict.
@amjoseph-nixpkgs let me see.. (sorry for disregarding this PR for some time). Edit: I had some older rebase prepared in my git tree not pushed. It is now rebased on latest master. I tested build and it correctly cross compiles rust library so I would say "it works". |
03c2e42
to
97bdba9
Compare
The setuptools-rust requires some environment variables to really perform cross build, otherwise it just builds for build platform. This adds setup hook that introduces these environment variables. There are three variables. The PYO3_CROSS_LIB_DIR has to point to the target's Python library directory. This has to be directory for the target not for the build or host. We have to choose the correct target Python. I am unsure how to do that simply in nixpkgs and this this implementations just delays this and waits for the correct Python when package using this hook is build. The CARGO_BUILD_TARGET triggers cross compilation in setuptools-rust. This is simply the Rust target specification. The CARGO_TARGET_*_LINKER variable should not be essentially required but setuptools-rust probably mangles the Rust build environment somewhat and that results to the missing linker. By explicitly specifying it using the environment variable we force the correct linker.
The rustPlatform.rustc has both host and target set to build platform. The rustc has build and host platform same but target is correctly set for cross compilation.
Targeting now staging. |
97bdba9
to
f5f80fa
Compare
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.
LGTM.
@Cynerd, in the future, please use the "Commit suggestion" button when you implement the verbatim changes suggested by a reviewer. Or if it's some kind of sharing-credit thing, at least make a separate commit with the requested change (once the PR is approved then you can squash).
When you implement peoples' suggestions by force-pushing a new commit with other stuff mixed in it is a hassle for the reviewer to figure out exactly what happened.
@FRidh I would like to merge this, because a lot of stuff is waiting on this PR or at least something equivalent to it. Have your concerns been addressed? If not, can you suggest a concrete change, or else perhaps wait for your concern to be dealt with in a follow-up PR? |
@ofborg eval |
Is this supposed to have fixed cross compiling cryptography (#205807)?
|
My mistake: it's only failing on armv6. |
I'm seeing this on armv7 too:
|
Create a issue. |
|
Description of changes
Introduces setup hook for setuptools-rust that provides environment variables to really perform cross compilation of Rust code when using setuptools-rust.
This is a followup of #208401 and #205807.
Things done
Added setup hook. I also added
rustc
andcargo
to the propagated dependencies and removed them from packages usingsetuptools-rust
. This is for sure optional and thus can be dropped if you disagree with it.sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes