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

Setuptools rust hook for cross compilation #212795

Merged
2 commits merged into from
Jul 28, 2023
Merged

Conversation

Cynerd
Copy link
Contributor

@Cynerd Cynerd commented Jan 26, 2023

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 and cargo to the propagated dependencies and removed them from packages using setuptools-rust. This is for sure optional and thus can be dropped if you disagree with it.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Copy link
Member

@FRidh FRidh left a 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.

@Cynerd
Copy link
Contributor Author

Cynerd commented Jan 27, 2023

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.

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.

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.

@FRidh and where they should be created actually? I found that python setup hooks are being created in pkgs/development/interpreters/python/hooks/default.nix. Should I move hook there?

@FRidh
Copy link
Member

FRidh commented Jan 27, 2023

@FRidh and where they should be created actually? I found that python setup hooks are being created in pkgs/development/interpreters/python/hooks/default.nix. Should I move hook there?

Yes, indeed there.

@Cynerd
Copy link
Contributor Author

Cynerd commented Feb 17, 2023

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.

@Cynerd Cynerd marked this pull request as ready for review February 18, 2023 16:15
@Cynerd Cynerd mentioned this pull request Mar 8, 2023
12 tasks
@Cynerd Cynerd force-pushed the setuptools-rust-hook branch 3 times, most recently from c0c3e77 to 03c2e42 Compare March 8, 2023 20:22
@ghost ghost self-requested a review June 1, 2023 10:15
ghost
ghost previously requested changes Jun 1, 2023
Copy link

@ghost ghost left a 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.

@Cynerd
Copy link
Contributor Author

Cynerd commented Jun 30, 2023

@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".

@Cynerd Cynerd force-pushed the setuptools-rust-hook branch 2 times, most recently from 03c2e42 to 97bdba9 Compare June 30, 2023 10:31
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.
@Cynerd Cynerd changed the base branch from master to staging June 30, 2023 12:52
@Cynerd
Copy link
Contributor Author

Cynerd commented Jun 30, 2023

there's a conflict and also needs to target staging, check CONTRIBUTING.md for how to rebase

Targeting now staging.

@ghost
Copy link

ghost commented Jul 26, 2023

This no longer works after #245475.

Details

@ghost ghost dismissed their stale review July 26, 2023 23:40

stale review; also, this PR needs rebased

@ghost
Copy link

ghost commented Jul 27, 2023

This no longer works after #245475.

I have reverted #245639 (well, I will once OfBorg wakes up).

Copy link

@ghost ghost left a 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.

@ghost
Copy link

ghost commented Jul 27, 2023

@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?

@ghost
Copy link

ghost commented Jul 28, 2023

@ofborg eval

@ghost ghost merged commit 52d3944 into NixOS:staging Jul 28, 2023
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jul 30, 2023

Is this supposed to have fixed cross compiling cryptography (#205807)?
I have just tried building pkgsCross.aarch64-multiplatform.python3.pkgs.cryptography and pkgsCross.raspberryPi.python3.pkgs.cryptography and both fails with:

    cargo:warning=In file included from /nix/store/2c7sgx69p6mmp76cvmi5j6c72dj76jj8-python3-3.10.12/include/python3.10/Python.h:50,
    cargo:warning=                 from /build/cryptography-41.0.2/src/rust/target/arm-unknown-linux-gnueabihf/release/build/cryptography-cffi-6fefc26573e7d979/out/_openssl.c:57:
    cargo:warning=/nix/store/2c7sgx69p6mmp76cvmi5j6c72dj76jj8-python3-3.10.12/include/python3.10/pyport.h:746:2: error: #error "LONG_BIT definition appears wrong for platform (bad gcc/glibc config?)."
    cargo:warning=  746 | #error "LONG_BIT definition appears wrong for platform (bad gcc/glibc config?)."
    cargo:warning=      |  ^~~~~
    exit status: 1

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jul 30, 2023

My mistake: it's only failing on armv6.

@kjeremy
Copy link
Contributor

kjeremy commented Oct 18, 2023

I'm seeing this on armv7 too:

warning: In file included from /nix/store/pzf6dnxg8gf04xazzjdwarm7s03cbrgz-python3-3.10.12/include/python3.10/Python.h:50,
warning:                  from /build/cryptography-41.0.3/src/rust/target/armv7-unknown-linux-gnueabihf/release/build/cryptography-cffi-1eecc4eb12ccf2f6/out/_opens>
warning: /nix/store/pzf6dnxg8gf04xazzjdwarm7s03cbrgz-python3-3.10.12/include/python3.10/pyport.h:746:2: error: #error "LONG_BIT definition appears wrong for platfo>
warning:   746 | #error "LONG_BIT definition appears wrong for platform (bad gcc/glibc config?)."
warning:       |  ^~~~~

error: failed to run custom build command for `cryptography-cffi v0.1.0 (/build/cryptography-41.0.3/src/rust/cryptography-cffi)`

@Artturin
Copy link
Member

Create a issue.

@kjeremy
Copy link
Contributor

kjeremy commented Oct 19, 2023

Create a issue.

#262073

@Cynerd Cynerd deleted the setuptools-rust-hook branch October 19, 2023 19:33
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants