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

maturinBuildHook: specify python interpreter #116001

Closed
wants to merge 1 commit into from

Conversation

rmcgibbo
Copy link
Contributor

@rmcgibbo rmcgibbo commented Mar 11, 2021

Motivation for this change

Pass absolute path to python interpreter to maturin, to work around a cross compilation issue under qemu user-mode emulation, fixes #115601

#115601 (comment)

Can be tested by running something like:

OPTIONS="--option system aarch64-linux
         --option sandbox false
         --extra-platforms aarch64-linux"
nix build $OPTIONS -f . python38Packages.adblock

from an x86 machine with boot.binfmt.emulatedSystems = [ "aarch64-linux" ];

cc @danieldk

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@r-rmcgibbo
Copy link

r-rmcgibbo commented Mar 11, 2021

Result of nixpkgs-review pr 116001 at 060c68e run on aarch64-linux 1

7 packages failed to build (new failures):
10 packages skipped due to time constraints:
  • python38Packages.fastapi
  • python38Packages.johnnycanencrypt
  • python38Packages.qiskit
  • python38Packages.qiskit-aqua
  • python38Packages.tumpa
  • python39Packages.adblock
  • python39Packages.qiskit
  • python39Packages.qiskit-aqua
  • python39Packages.qiskit-ibmq-provider
  • python39Packages.starlette
15 packages built successfully:

Result of nixpkgs-review pr 116001 at 060c68e run on x86_64-linux 1

5 packages skipped due to time constraints:
  • python39Packages.adblock
  • python39Packages.johnnycanencrypt
  • python39Packages.qiskit
  • python39Packages.qiskit-aqua
  • python39Packages.tumpa
27 packages built successfully:

@rmcgibbo
Copy link
Contributor Author

(the qiskit failures reported by r-rmcgibbo are pre-existing, despite what the comment says. e.g. https://hydra.nixos.org/job/nixpkgs/trunk/python38Packages.cvxpy.aarch64-linux)

@@ -19,6 +19,7 @@ maturinBuildHook() {
--target @rustTargetPlatformSpec@ \
--manylinux off \
--strip \
--interpreter $(command -v python) \
Copy link
Contributor

@danieldk danieldk Mar 12, 2021

Choose a reason for hiding this comment

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

I am not familiar enough with the Python machinery. @FRidh is python on PATH the interpreter of the build platform or the host platform in cross-compilation?

@rmcgibbo I mean true cross-compilation here, not compilation under AArch64 emulation, which is strictly not cross-compilation (I think).

Copy link
Member

Choose a reason for hiding this comment

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

strictDeps is used, so its the interpreter of the build platform.

Copy link
Member

Choose a reason for hiding this comment

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

However, its probably better to hardcode the interpreter with a substitution, because there are alternative Python interpreters, such as pypy.

Copy link
Contributor

@danieldk danieldk Mar 12, 2021

Choose a reason for hiding this comment

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

However, its probably better to hardcode the interpreter with a substitution, because there are alternative Python interpreters, such as pypy.

Yeah, that would be even better.

There are some variables that can be set for providing the correct header and library paths:

https://pyo3.rs/v0.13.2/building_and_distribution.html#cross-compiling

But that would only work for bindings that use PyO3. However, since PyO3 is the most commonly used crate for binding Python, I guess the hook could detect if a package is using PyO3 and then set these variables in the expected way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strictDeps is used, so its the interpreter of the build platform.

maturin does run the interpreter in order to extract some metadata about it, so if it's the build platform's rather than the host platform's python, I'm not sure that's going to work without emulation...

However, its probably better to hardcode the interpreter with a substitution, because there are alternative Python interpreters, such as pypy.

This was my first thought as well, but it wasn't clear to me how to accomplish it within the hook / nix code.

Copy link
Member

Choose a reason for hiding this comment

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

...however, I am not sure that that would be good either, isn't the Python interpreter metadata supposed to be for the platform you are building for?

A separate build interpreter is built. This interpreter adds the host metadata to sysconfig.

Copy link
Member

Choose a reason for hiding this comment

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

In CPython cross-compilation does not really get much attention, so many downstreams such as Nixpkgs and Yocto have their patches.

Copy link
Member

Choose a reason for hiding this comment

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

So, this will get messy if we use a substitution, because then the hook will depend on Python, and we need to rebuild the hook for each Python package set as well, resulting in a matrix of hooks. Not very nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, this will get messy if we use a substitution, because then the hook will depend on Python, and we need to rebuild the hook for each Python package set as well, resulting in a matrix of hooks. Not very nice.

Agreed. It seems like it's going to be tricky to both:

(a) have this work for pypy and other alternative python interpreters, and;
(b) not need to rebuild the hook for each Python package set.

Unfortunately my PR against maturin (PyO3/maturin#471) did not get a lot of traction, and filing a bug against rustlang or qemu (it's probably a qemu bug, but I'm just guessing) is daunting.

I suppose I can just change this PR to something ugly like

get_python() {
    for name in "pypy" "python"; do
        path=$(command -v "$name" || echo "")
        if [ -f "$path" ]; then
            echo $path;
            break
        fi
    done
}

...

maturin ... \
  --interpreter $(get_python)

Copy link
Member

Choose a reason for hiding this comment

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

maybe the interpreters should export a setup hook setting an alias python.

@stale
Copy link

stale bot commented Sep 14, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 14, 2021
@rmcgibbo rmcgibbo closed this Jan 9, 2022
@rmcgibbo rmcgibbo deleted the maturinBuildHook branch January 9, 2022 16:51
@kczulko
Copy link

kczulko commented Mar 6, 2024

Hello,

I came to this issue while cross-compiling from host x86_64-linux to target aarch64-multiplatform. This patch more less helped. However, I had to adjust it in the following way:

+        --interpreter python3.11 \

otherwise I was getting this:

Caused by: Python interpreter should be a kind of interpreter (e.g. 'python3.8' or 'pypy3.9') when cross-compiling, got path to interpreter: /nix/store/y027d3bvlaizbri04c1bzh28hqd6lj01-python3-3.11.7/bin/python

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: rust 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with maturinBuildHook during cross-compilation
5 participants