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

python3: fix overriding of interpreters, closes #163639 #169475

Merged
merged 2 commits into from
Apr 21, 2022

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Apr 20, 2022

Overriding the interpreters did not work correctly. When overriding
packages would end up twice in the build time closure: one corresponding
to the overridden interpreter and one corresponding to the original
interpreter. The reason is that the override was not applied to the
interpreters in the spliced package sets.

Description of changes
Things done
  • 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/)
  • 22.05 Release Notes (or backporting 21.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.

@FRidh
Copy link
Member Author

FRidh commented Apr 20, 2022

@Ericson2314 work-around needed for lib.makeScopeWithSplicing.

@FRidh
Copy link
Member Author

FRidh commented Apr 20, 2022

Note that while this "fixes" the usage of .override, .overrideAttrs would I think still not function.

Overriding the interpreters did not work correctly. When overriding
packages would end up twice in the build time closure: one corresponding
to the overridden interpreter and one corresponding to the original
interpreter. The reason is that the override was not applied to the
interpreters in the spliced package sets.
@FRidh FRidh requested review from jonringer and mweinelt April 20, 2022 19:37
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 20, 2022
@Artturin Artturin requested a review from roberth April 21, 2022 00:44
adisbladis
adisbladis previously approved these changes Apr 21, 2022
@adisbladis adisbladis dismissed their stale review April 21, 2022 06:01

Noticed that it breaks cross

@adisbladis
Copy link
Member

This breaks cross compilation:

$ nix-instantiate ./. -A pkgsCross.aarch64-multiplatform.python3
error: stack overflow (possible infinite recursion)

@adisbladis
Copy link
Member

adisbladis commented Apr 21, 2022

I pushed a commit fixing the cross issue in adisbladis@b4fbe2e
I'm not a huge fan of the solution as it's a bit crude and adds maintenance burden (any function options would have to be kept in sync with the inputs attrset), but it's the only thing I could come up with.

@FRidh
Copy link
Member Author

FRidh commented Apr 21, 2022

I pushed a commit fixing the cross issue in adisbladis@b4fbe2e
I'm not a huge fan of the solution as it's a bit crude and adds maintenance burden (any function options would have to be kept in sync with the inputs attrset), but it's the only thing I could come up with.

That makes sense. I suppose we could filter derivations out of inputs.

@adisbladis
Copy link
Member

That makes sense. I suppose we could filter derivations out of inputs.

Yeah, that's a far more palatable solution.
Additionally that would have the benefit of actually behaving as expected when overriding a dependency as null, but it wouldn't work as expected when a dependency is overriden with overrideattrs.

Tried that out in adisbladis@9203631 and it also works just fine to build a cross compiled environment.

In the case of cross compilation we don't want pass through build
inputs from the cross platform, but we do want to pass on config options.
@roberth roberth removed their request for review April 21, 2022 09:19
@FRidh FRidh merged commit 881ea51 into NixOS:master Apr 21, 2022
@FRidh FRidh deleted the python-spliced branch April 21, 2022 12:50
@lopsided98
Copy link
Contributor

lopsided98 commented May 18, 2022

This breaks nativeBuildInputs when cross-compiling (not sure why yet, I identified this through bisecting). buildInputs of nativeBuildInputs of Python packages end up being for the host platform. For example, this occurs with any package that uses cffi, such as brotlicffi:

gcc -shared -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-zlib-1.2.12/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-bzip2-1.0.6.0.2/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-expat-2.4.8/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeee>
/nix/store/rs684lgm8k7akkgbisb49z4vpxxc2zns-binutils-2.38/bin/ld: skipping incompatible /nix/store/b7p34l2l1w21b6kvpv31kr65v0pdfyzh-libffi-armv6l-unknown-linux-gnueabihf-3.4.2/lib/libffi.so when searching for -lffi
/nix/store/rs684lgm8k7akkgbisb49z4vpxxc2zns-binutils-2.38/bin/ld: skipping incompatible /nix/store/b7p34l2l1w21b6kvpv31kr65v0pdfyzh-libffi-armv6l-unknown-linux-gnueabihf-3.4.2/lib/libffi.so when searching for -lffi
/nix/store/rs684lgm8k7akkgbisb49z4vpxxc2zns-binutils-2.38/bin/ld: cannot find -lffi: No such file or directory
/nix/store/rs684lgm8k7akkgbisb49z4vpxxc2zns-binutils-2.38/bin/ld: skipping incompatible /nix/store/b7p34l2l1w21b6kvpv31kr65v0pdfyzh-libffi-armv6l-unknown-linux-gnueabihf-3.4.2/lib/libffi.so when searching for -lffi
/nix/store/rs684lgm8k7akkgbisb49z4vpxxc2zns-binutils-2.38/bin/ld: skipping incompatible /nix/store/b7p34l2l1w21b6kvpv31kr65v0pdfyzh-libffi-armv6l-unknown-linux-gnueabihf-3.4.2/lib/libffi.so when searching for -lffi
collect2: error: ld returned 1 exit status
error: command '/nix/store/bg35nfwn6zd616facdywiysgpprfvsji-gcc-wrapper-11.3.0/bin/gcc' failed with exit code 1

I used the following Nix code to bisect this:

{ lib, buildPythonPackage, cffi }:

let
  self = buildPythonPackage {
    name = "test-drv";

    nativeBuildInputs = [ cffi ];
  };

  cffiDep = lib.elemAt self.nativeBuildInputs (lib.length self.nativeBuildInputs - 1);
  libffiDep = lib.elemAt cffiDep.buildInputs (lib.length cffiDep.buildInputs - 1);
in
assert cffiDep.pname == "cffi";
assert libffiDep.pname == "libffi";
assert libffiDep.stdenv.hostPlatform == libffiDep.stdenv.buildPlatform;
self

The assertion will fail after this PR (run with nix-build -E 'with import ./. {}; pkgsCross.raspberryPi.python3Packages.callPackage ./test.nix {}'). Note that if you actually want to cross-compile cffi, you need #173435

@FRidh
Copy link
Member Author

FRidh commented May 18, 2022

That's unfortunate. Did you test with multiple packages? Seems like we need to revert this then.

@takeda
Copy link
Contributor

takeda commented May 18, 2022

@FRidh my original problem was that when overriding python, the old packages were used from the original version.

I'm wondering maybe there is a bug in another place and the old behavior was just hiding it?

Alternatively, maybe overrides shouldn't be applied for the Build version? Although I'm less convinced of this one.

@FRidh
Copy link
Member Author

FRidh commented May 18, 2022

No, the issue originated exactly because the build Python was not overridden. That is solved now. Unfortunately it is not possible to pass in derivations to the spliced sets because then they're for the wrong purpose (build/host/target).

I think it is really difficult to override a build derivation in a spliced package set without globally overriding that derivation (in this case the Python interpreter) with an overlay.

@lopsided98
Copy link
Contributor

lopsided98 commented May 18, 2022

Did you test with multiple packages?

Yes, the same issue occurs if you replace cffi in my example with any other Python package that has non-Python buildInputs, such as numpy, pyaudio or pygobject3.

@Mindavi Mindavi added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label May 18, 2022
@adisbladis
Copy link
Member

I think the fix to this is as simple as:

diff --git a/pkgs/development/interpreters/python/cpython/default.nix b/pkgs/development/interpreters/python/cpython/default.nix
index 4463dc8e9ba..47f9d9a3775 100644
--- a/pkgs/development/interpreters/python/cpython/default.nix
+++ b/pkgs/development/interpreters/python/cpython/default.nix
@@ -85,7 +85,7 @@ let
 
   passthru = let
     # When we override the interpreter we also need to override the spliced versions of the interpreter
-    inputs' = lib.filterAttrs (_: v: ! lib.isDerivation v) inputs;
+    inputs' = lib.filterAttrs (n: v: ! lib.isDerivation v && n != "passthruFun") inputs;
     override = attr: let python = attr.override (inputs' // { self = python; }); in python;
   in passthruFun rec {
     inherit self sourceVersion packageOverrides;

@lopsided98
Copy link
Contributor

That fix seems to work.

Artturin added a commit to Artturin/nixpkgs that referenced this pull request Jun 16, 2022
github-actions bot pushed a commit that referenced this pull request Jun 16, 2022
see #169475 (comment)

patch by adisbladis

Co-authored-by: adisbladis <[email protected]>
(cherry picked from commit 843b988)
@con-f-use
Copy link
Contributor

con-f-use commented Jun 19, 2023

Sorry for asking here, but how is this supposed to work? Because I still end up with two versions of the same package, when I do something like:

  pythonOverlay = final: prev: {
    weakPython = final.python311.override {
      openssl = true;
      openssl_legacy = final.weakOpenssl;
      packageOverrides = pyfinal: pyprev: {
        pyopenssl = pyprev.pyopenssl.override { openssl = final.weakOpenssl; };
        cryptography = pyprev.cryptography.override { openssl = final.weakOpenssl; };
      };
      self = final.weakPython;
    }

e.g. building weakPython.pkgs.beautifulsoup4

Found duplicated packages in closure for dependency 'packaging': 
  packaging 23.0 (/nix/store/2y2542j56m4zk5c5bz4wiyp9wdanfnj1-python3.11-packaging-23.0/lib/python3.11/site-packages)
  packaging 23.0 (/nix/store/47jcryfc1gn7d52nvrwzdxg5bvqr1zgs-python3.11-packaging-23.0/lib/python3.11/site-packages)
Found duplicated packages in closure for dependency 'six': 
  six 1.16.0 (/nix/store/41cwsgbqz23lr8pz73y91nabclgd31gb-python3.11-six-1.16.0/lib/python3.11/site-packages)
  six 1.16.0 (/nix/store/skpaa2kjggn9ycxi842rpj4izydvbfb8-python3.11-six-1.16.0/lib/python3.11/site-packages)

@domenkozar
Copy link
Member

It seems like it's broken again.

@takeda
Copy link
Contributor

takeda commented Jul 24, 2023

Yes, I'm also seeing it once again:

Found duplicated packages in closure for dependency 'cffi':
  cffi 1.15.1 (/nix/store/p8kj8w41hsqcs48l4bqwhw8mzwlj449s-python3.11-cffi-1.15.1/lib/python3.11/site-packages)
  cffi 1.15.1 (/nix/store/fxm8r4p3p6kv5rky6sfpbkknq9gcfkf2-python3.11-cffi-1.15.1/lib/python3.11/site-packages)
Found duplicated packages in closure for dependency 'pycparser':
  pycparser 2.21 (/nix/store/vvjz5fbi04cd9q1nnmvc76hrcy8s64g3-python3.11-pycparser-2.21/lib/python3.11/site-packages)
  pycparser 2.21 (/nix/store/9dbwyd7h1y1f0d37zhlrllannkqlmfdi-python3.11-pycparser-2.21/lib/python3.11/site-packages)

@mweinelt
Copy link
Member

This change made evaluating cross packages very memory expensive, likely because there is no memoization on the override. I'm inclined to go for a revert, but I have no good idea about how to make the interpreter overridable any other way.

In the meantime, if you want to run out of memory, try nix-instantiate ./. -A pkgsCross.aarch64-multiplatform.home-assistant.

@adisbladis
Copy link
Member

but I have no good idea about how to make the interpreter overridable any other way.

We should probably break out the Python packages factory function and stop conflating overriding the interpreter with overriding the package set.

@kjeremy
Copy link
Contributor

kjeremy commented Sep 19, 2024

@adisbladis I'm not really familiar with python development but the memory usage is killing me. Maybe I could tackle this if I had some guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: python 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants