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

cpython: fix cross-compiling extension modules #98915

Merged
merged 1 commit into from Dec 28, 2020

Conversation

@lopsided98
Copy link
Contributor

@lopsided98 lopsided98 commented Sep 27, 2020

Motivation for this change

Fixes cross-compiling Python native extension modules to host platforms with a different word length than the build platform as well as in a few other cases.

Currently, cross-compiling Python extension modules generally works assuming the build and host platforms have the same word size. This leads to problems like those seen here: #50925 (comment), where x86_64 -> aarch64 works, but x86_64 -> armv7l doesn't. Unfortunately, this PR doesn't fix btrfs-progs (#50925) and talloc (#63043), because they do not use setuptools/distutils.

This happens because the build Python's headers are used, rather than host Python's headers. The header location and other data used for compiling extension modules are available in the _sysconfigdata__<kernel>_<platform> module. distutils.sysconfig can be patched to use this data and then Python needs to be told to use sysconfigdata from host Python using the _PYTHON_SYSCONFIGDATA_NAME environment variable. I believe the patch should be upstreamable.

There was an ill-fated upstream PR to improve and document Python cross-compilation, avoiding the need to manually construct _PYTHON_SYSCONFIGDATA_NAME, but based on my quick review I don't think it would have worked for us as is: python/cpython#17420

Setting _PYTHON_HOST_PLATFORM and _PYTHON_SYSCONFIGDATA_NAME appropriately also fixes some other issues, such as adding the correct platform suffix to extension module file names. This allows cryptography to build after a few minor fixes.

I have tested this PR by cross-compiling a number of Python packages for armv6l, including requests and netifaces, both of which failed before.

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.
@lopsided98 lopsided98 requested review from FRidh and jonringer as code owners Sep 27, 2020
@lopsided98
Copy link
Contributor Author

@lopsided98 lopsided98 commented Sep 27, 2020

@@ -31,6 +32,7 @@ buildPythonPackage rec {

outputs = [ "out" "dev" ];

nativeBuildInputs = stdenv.lib.optional (!isPyPy) python.pythonForBuild.pkgs.cffi;
Copy link
Contributor Author

@lopsided98 lopsided98 Sep 27, 2020

Splicing doesn't seem to work here. Anyone know why?

Copy link
Member

@FRidh FRidh Sep 27, 2020

Copy link
Member

@FRidh FRidh Oct 6, 2020

If this is always the case we are going to have a bit of a problem, because all the nativeBuildInputs in case of Python packages are then incorrect.

@@ -31,6 +32,7 @@ buildPythonPackage rec {

outputs = [ "out" "dev" ];

nativeBuildInputs = stdenv.lib.optional (!isPyPy) python.pythonForBuild.pkgs.cffi;
Copy link
Member

@FRidh FRidh Sep 27, 2020

@lopsided98
Copy link
Contributor Author

@lopsided98 lopsided98 commented Sep 28, 2020

I created an upstream PR: python/cpython#22440

@lopsided98 lopsided98 force-pushed the python-cross-extensions branch from 2f56545 to 4571e76 Sep 29, 2020
@ofborg ofborg bot requested a review from FRidh Sep 29, 2020
@lopsided98 lopsided98 force-pushed the python-cross-extensions branch from 4571e76 to 82c7ed5 Oct 1, 2020
@lopsided98
Copy link
Contributor Author

@lopsided98 lopsided98 commented Oct 1, 2020

I have now implemented this PR as a setup hook. I did not add it to the common Python setup-hook.sh because the environment variables are cpython specific and because I needed access to cpython's $out, which cannot be done easily if the setup hook is defined in a separate derivation. To get the sysconfigdata module on PYTHONPATH, I moved it to site-packages.

Also, this PR now fixes #50925.

@lopsided98
Copy link
Contributor Author

@lopsided98 lopsided98 commented Oct 1, 2020

After looking into it some more, this PR also helps solve #63043. Normally talloc uses python-config, but it will fall back to using the sysconfig module if python-config is not found (apparently this is to support Windows, but it seems to work fine on Linux). You can force this to happen by setting PYTHON_CONFIG=/invalid in the environment. This also applies to samba and tdb, which all use the same build system.

@Thra11
Copy link
Contributor

@Thra11 Thra11 commented Oct 2, 2020

This doesn't fix #63043 for me. Running nix-build '<nixpkgs>' -A pkgsCross.armv7l-hf-multiplatform.talloc I still get:

Checking if compiler accepts -fstack-clash-protection                             : yes
Checking linker accepts -Wl,-no-undefined                                         : yes
Checking linker accepts ['-undefined', 'dynamic_lookup']                          : no
Checking for program 'python3'                                                    : /nix/store/0w5g6pmsb0i7f7vklbg2snyj3wfalq7y-python3-3.8.5/bin/python3
Checking for program 'python'                                                     : /nix/store/0w5g6pmsb0i7f7vklbg2snyj3wfalq7y-python3-3.8.5/bin/python3
Checking for program 'python3'                                                    : /nix/store/0w5g6pmsb0i7f7vklbg2snyj3wfalq7y-python3-3.8.5/bin/python3
Checking for python version >= 3.5.0                                              : 3.8.5
python-config                                                                     : /nix/store/0w5g6pmsb0i7f7vklbg2snyj3wfalq7y-python3-3.8.5/bin/python3-config
Asking python-config for pyembed '--cflags --libs --ldflags --embed' flags        : yes
Testing pyembed configuration                                                     : Could not build a python embedded interpreter
Testing pyembed configuration                                                     : Could not build a python embedded interpreter
The configuration failed
(complete log in /build/talloc-2.3.1/bin/config.log)
builder for '/nix/store/5bn9ss7a1al5js5l0ncz0n4q5i3hqgc9-talloc-2.3.1-armv7l-unknown-linux-gnueabihf.drv' failed with exit code 1

@Thra11
Copy link
Contributor

@Thra11 Thra11 commented Oct 2, 2020

Unfortunately, --keep-failed doesn't seem to work any more, so I can't check config.log.

@lopsided98
Copy link
Contributor Author

@lopsided98 lopsided98 commented Oct 2, 2020

Yes, it doesn't solve #63043 by itself, you also need to set PYTHON_CONFIG="/invalid" in talloc.

@Thra11
Copy link
Contributor

@Thra11 Thra11 commented Oct 2, 2020

Ah. Missed that bit, sorry!

@Thra11
Copy link
Contributor

@Thra11 Thra11 commented Oct 2, 2020

Still doesn't quite work for me:

Checking for python version >= 3.5.0                                              : 3.8.5
python-config                                                                     : /invalid
Checking for library python3.8 in LIBPATH_PYEMBED                                 : not found
Checking for library python3.8 in LIBDIR                                          : not found
Checking for library python3.8 in python_LIBPL                                    : yes
Checking for header Python.h                                                      : Distutils not installed? Broken python installation? Get python-config now!
The configuration failed
(complete log in /build/talloc-2.3.1/bin/config.log)
builder for '/nix/store/8jf92la0x7x00wfaffka361s1qipggcb-talloc-2.3.1-armv7l-unknown-linux-gnueabihf.drv' failed with exit code 1

@lopsided98
Copy link
Contributor Author

@lopsided98 lopsided98 commented Oct 2, 2020

Oh right, you also have to add both host and build Python as dependencies. I've added the full fix to this PR.

@lopsided98
Copy link
Contributor Author

@lopsided98 lopsided98 commented Nov 29, 2020

I fixed the older Python versions as well now.

@ofborg build python36.tests python37.tests python38.tests python39.tests python310.tests

@lopsided98
Copy link
Contributor Author

@lopsided98 lopsided98 commented Nov 29, 2020

3.10 was already broken.

@ofborg build python36.tests python37.tests python38.tests python39.tests

@FRidh
Copy link
Member

@FRidh FRidh commented Nov 29, 2020

Currently we're working on updating the Python packages set. When that is done, I'll push this job to that branch for further testing. That will be about a week from now.

@Atemu
Copy link
Member

@Atemu Atemu commented Dec 14, 2020

Hi, it's been two weeks; any update?

@flokli
Copy link
Contributor

@flokli flokli commented Dec 26, 2020

I can confirm this fixes #107578.

I cherry-picked this on top of currently master locally and will run some more tests.

@FRidh is this pushed somewhere to a jobset?

@flokli
Copy link
Contributor

@flokli flokli commented Dec 26, 2020

I pushed the cherry-pick to cpython-headers-cross.

This reproducibly breaks the test of libuv (on x86_64-linux), which is a dependency of cmake:

not ok 72 - fs_fstat
# exit code 134
# Output from process `fs_fstat`:
# Assertion failed in test/test-fs.c on line 1389: s->st_dev == (uint64_t) t.st_dev

pkgs.pkgsCross.raspberryPi.libuv builds, however.

Edit: Ouch, ignore that. Apparently, this seems to be have been a bitflip.

@flokli
Copy link
Contributor

@flokli flokli commented Dec 27, 2020

I was able to successfully build -A python36.tests -A python37.tests -A python38.tests -A python39.tests.

python310.tests failed before and after this.

@FRidh
Copy link
Member

@FRidh FRidh commented Dec 27, 2020

Pushed 0959552 to python-unstable. Should make it easier to test wider. https://hydra.nixos.org/eval/1637790

@FRidh
Copy link
Member

@FRidh FRidh commented Dec 28, 2020

https://hydra.nixos.org/eval/1637790 looks good. This now needs a rebase and then it can go in staging.

@flokli flokli force-pushed the python-cross-extensions branch from cf2bb6e to 1a65c5d Dec 28, 2020
@flokli
Copy link
Contributor

@flokli flokli commented Dec 28, 2020

I rebased this to latest staging and pushed again.

The only conflict was the addition of the # Backport a fix for ctypes.util.find_library. patch in pkgs/development/interpreters/python/cpython/default.nix in staging, which was causing git to not find the exact place to add the patches introduced here.

@flokli
Copy link
Contributor

@flokli flokli commented Dec 28, 2020

I successfully built all of python3{6,7,8,9}.tests and confirmed this still fixes pkgsCross.raspberryPi.libselinux, so it doesn't look like there's any conflicts due to other changes in staging in the meantime. Let's get this in :-)

@flokli flokli merged commit 093632a into NixOS:staging Dec 28, 2020
4 of 7 checks passed
Python batch upgrade automation moved this from To do to Done Dec 28, 2020
@matthewbauer
Copy link
Member

@matthewbauer matthewbauer commented Dec 29, 2020

Would it be possible to apply this to 20.09? I think we may be able to avoid the mass-rebuild...

@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 29, 2020

as long it doesn't have any breakages. I'm fine with back-porting it

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Dec 29, 2020

@matthewbauer While the first version of this was made independently, it's possible the final version relies on #104201 in some ways, too.

@FRidh
Copy link
Member

@FRidh FRidh commented Dec 29, 2020

@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 29, 2020

A backport would need extensive testing as well first.

This is my biggest concern. I would prefer not to backport if the scope of everything needed to backport is ill-defined, and likely to introduce regressions. Unstable works great for iteration, but the purpose of the release channel is to allow people to update without fear.

@FRidh FRidh mentioned this pull request Jan 7, 2021
10 tasks
@pjjw
Copy link
Contributor

@pjjw pjjw commented Jan 21, 2021

hey just FYI i think this PR broke python extensions under musl libc, have a PR out to fix at #110293. looks like you all are talking about backporting this and it'd be a bummer to backport that breakage too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment