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

Revert "Revert "python3: splice python within python3Packages.callPackage"" #247245

Merged
1 commit merged into from
Aug 5, 2023

Conversation

Artturin
Copy link
Member

@Artturin Artturin commented Aug 4, 2023

This reverts commit 2ffca30.

793cc9d982415b71cdba729cf779bfc49e9d2ae7 python3: splice python within python3Packages.callPackage Was reverted because it broke
pkgsCross.aarch64-multiplatform.python3Packages.cryptography

But by reverting the revert pkgsCross.aarch64-multiplatform.python3Packages.cryptography still builds.

I tried reverting the 2 cryptography update commits, and it still worked, so I do not know why this would work now.

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/)
  • 23.11 Release Notes (or backporting 23.05 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
  • Fits CONTRIBUTING.md.

…kage""

This reverts commit 2ffca30.

`793cc9d982415b71cdba729cf779bfc49e9d2ae7` `python3: splice python within python3Packages.callPackage`
Was reverted because it broke
`pkgsCross.aarch64-multiplatform.python3Packages.cryptography`

But by reverting the revert `pkgsCross.aarch64-multiplatform.python3Packages.cryptography` still builds.

I tried reverting the 2 cryptography update commits, and it still
worked, so I do not know why this would work now.
@Artturin
Copy link
Member Author

Artturin commented Aug 4, 2023

@amjoseph-nixpkgs Did we overlook something? Why is this working now??

nix-diff $(nix eval --raw ".?ref=staging#pkgsCross.aarch64-multiplatform.python3Packages.cryptography.drvPath") $(nix eval --raw ".#pkgsCross.aarch64-multiplatform.python3Packages.cryptography.drvPath") looks completely fine

@ghost
Copy link

ghost commented Aug 5, 2023

nix-diff $(nix eval --raw ".?ref=staging#pkgsCross.aarch64-multiplatform.python3Packages.cryptography.drvPath") $(nix eval --raw ".#pkgsCross.aarch64-multiplatform.python3Packages.cryptography.drvPath") looks completely fine

What do you mean by "completely fine"? I still see the same diff that I posted here:

#245475 (comment)

I'm rebuilding to see if something somewhere else got fixed. If so I'll bisect to find out exactly what.

@Artturin
Copy link
Member Author

Artturin commented Aug 5, 2023

nix-diff $(nix eval --raw ".?ref=staging#pkgsCross.aarch64-multiplatform.python3Packages.cryptography.drvPath") $(nix eval --raw ".#pkgsCross.aarch64-multiplatform.python3Packages.cryptography.drvPath") looks completely fine

What do you mean by "completely fine"? I still see the same diff that I posted here:

#245475 (comment)

image

yep looks like it should

@Artturin
Copy link
Member Author

Artturin commented Aug 5, 2023

git checkout 793cc9d982415b71cdba729cf779bfc49e9d2ae7
793cc9d
then
git apply - <<< $(curl "https://patch-diff.githubusercontent.com/raw/NixOS/nixpkgs/pull/212795.patch")
#212795
then
nix-build -A pkgsCross.aarch64-multiplatform.python3Packages.cryptography

builds OK

Maybe you messed up a merge or something?

@ghost
Copy link

ghost commented Aug 5, 2023

Still waiting for my build to catch up.

1/368/378 built because, you know, LLVM. 😾

@ghost
Copy link

ghost commented Aug 5, 2023

FWIW, this is the failure that led me to ask for a revert:

Yes, I should have put that in the commit message, not just the in the PR.

Still waiting for LLVM.

@ghost
Copy link

ghost commented Aug 5, 2023

Ah, from #245475 (comment)

"aarch64-unknown-linux-gnu-gcc\" \"-O3\" .. \"-fPIC\" \"-m64\"
 ^^^^^^^                                                ^^^^

one of these things is not like the other. I've seen this before: buildPlatform CFLAGS being passed to the hostPlatform's compiler.

When I tried troubleshooting #245475 I was so focused on trying to minimize the change to see what part of it caused the issue that I failed to, you know, look closely at the error message.

80% done building LLVM.

@ghost
Copy link

ghost commented Aug 5, 2023

Yeah, here's where I saw that before:

#226081 (comment)

I have a theory on what's going on here. Will test theory once I have a caught-up build. LLVM is running its lovely 48,200-case testsuite.

@ghost
Copy link

ghost commented Aug 5, 2023

Hah! #225360 (comment)

@ghost
Copy link

ghost commented Aug 5, 2023

I was able to build pkgsCross.aarch64-multiplatform.python3Packages.cryptography at this commit.

This PR is targeted at staging which is a jillion commits ahead of the revert-which-it-reverts, which was targeted at master. Obviously something in that jillion commits fixed this. The jillion commits includes two version bumps of python-cryptography, a patch to openssl, and a bunch of other stuff.

Rather than try to bisect through all those commits on staging (each of which is a full stdenv rebuild), I'm just going to build everything at this commit overnight.

Building on:

  • x86_64-linux workstation packageset
  • powerpc64le-linux workstation packageset
  • aarch64-linux laptop packageset (cross from x86_64-linux)
  • mips64el-linux mixed n32/64 router packageset (cross from x86_64-linux)

And:

  • pkgs.tests.cross.sanity

@Artturin
Copy link
Member Author

Artturin commented Aug 5, 2023

This PR is targeted at staging which is a jillion commits ahead of the revert-which-it-reverts, which was targeted at master. Obviously something in that jillion commits fixed this. The jillion commits includes two version bumps of python-cryptography, a patch to openssl, and a bunch of other stuff.

#247245 (comment)

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.

Built (all but chromium+qutebrowser, which are still building) on:

  • x86_64-linux workstation packageset
  • powerpc64le-linux workstation packageset
  • aarch64-linux laptop packageset (cross from x86_64-linux)
  • mips64el-linux mixed n32/64 router packageset (cross from x86_64-linux)

I'm still not sure what caused the build failures I saw before. I had a very slightly different version of #212795 in my local tree (which is 112 commits diverged from master); maybe it was that.

@ghost ghost marked this pull request as ready for review August 5, 2023 21:33
@ghost ghost requested review from FRidh and jonringer as code owners August 5, 2023 21:33
@ghost ghost merged commit e67ff66 into NixOS:staging Aug 5, 2023
6 checks passed
@@ -1,9 +1,9 @@
self: super: with self;
self: dontUse: with self;
Copy link
Member

Choose a reason for hiding this comment

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

Could we use _ here instead of dontUse?

Copy link
Member Author

@Artturin Artturin Aug 7, 2023

Choose a reason for hiding this comment

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

I want it to be noticeable without evalling that it should not be used, It could be changed to _dontUse if one wanted to quieten deadnix.

Copy link

Choose a reason for hiding this comment

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

BTW, why exactly will this never be useful or usable at any time in the future?

I can see why it isn't useful right now, but it seemed non-obvious to me that it would never be useful. If that's true there probably ought to be a comment explaining it.

Copy link
Member Author

@Artturin Artturin Aug 7, 2023

Choose a reason for hiding this comment

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

If super had to be used then it won't be possible to deduplicate and inherits would have to be done manually

      keep = self: {
        inherit (self)
          condaInstallHook
          condaUnpackHook
          eggBuildHook
          eggInstallHook
          eggUnpackHook
          flitBuildHook
          pipBuildHook
          pipInstallHook
          pytestCheckHook
          pythonCatchConflictsHook
          pythonImportsCheckHook
          pythonNamespacesHook
          pythonOutputDistHook
          pythonRecompileBytecodeHook
          pythonRelaxDepsHook
          pythonRemoveBinBytecodeHook
          pythonRemoveTestsDirHook
          setuptoolsBuildHook
          setuptoolsCheckHook
          setuptoolsRustBuildHook
          sphinxHook
          unittestCheckHook
          venvShellHook
          wheelUnpackHook
          wrapPython
        ;
      };

calling twice with self probably won't work when the package actually has a difference between self and super, I don't know when that would be the case here or why using super would be necessary here

Copy link
Member Author

@Artturin Artturin Aug 7, 2023

Choose a reason for hiding this comment

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

The best thing to solve this issue would be #228139 and to deprecate python3.pkgs
but it's not urgent

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course that is assuming I debugged correctly and there's no other issues.

Copy link

@ghost ghost Aug 16, 2023

Choose a reason for hiding this comment

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

tjni added a commit that referenced this pull request Aug 20, 2023
github-actions bot pushed a commit to arcnmx/nixpkgs-lib that referenced this pull request Aug 22, 2023
@takeda
Copy link
Contributor

takeda commented Sep 3, 2023

This revert did break musl build for me, I see this.

I'm getting this error when building pydantic with crosssystem set to musl, and arrived to this PR by bisecting:

┃ error: output '/nix/store/bi0795dnsdbib2jqpc9hx16mn68sny36-python3.11-pydantic-1.10.12-x86_64-unknown-linux-musl' is not allowed to refer to the following paths:
┃          /nix/store/b5w9gndxsm7gakddgl6f2ma6lk8x8ivf-python3-3.11.4

@Artturin
Copy link
Member Author

Artturin commented Sep 3, 2023

This revert did break musl build for me, I see this.

I'm getting this error when building pydantic with crosssystem set to musl, and arrived to this PR by bisecting:

┃ error: output '/nix/store/bi0795dnsdbib2jqpc9hx16mn68sny36-python3.11-pydantic-1.10.12-x86_64-unknown-linux-musl' is not allowed to refer to the following paths:
┃          /nix/store/b5w9gndxsm7gakddgl6f2ma6lk8x8ivf-python3-3.11.4

can't repro with pkgsMusl.python310Packages.pydantic and pkgsMusl.python311Packages.pydantic fails at flit-core

was able to repro with p311 and

diff --git a/pkgs/top-level/stage.nix b/pkgs/top-level/stage.nix
index 3886ae04e492..b35d82d31dc5 100644
--- a/pkgs/top-level/stage.nix
+++ b/pkgs/top-level/stage.nix
@@ -204,8 +204,7 @@ let
       overlays = [ (self': super': {
         pkgsMusl = super';
       })] ++ overlays;
-      ${if stdenv.hostPlatform == stdenv.buildPlatform
-        then "localSystem" else "crossSystem"} = {
+        crossSystem = {
         parsed = makeMuslParsedPlatform stdenv.hostPlatform.parsed;
       };
     } else throw "Musl libc only supports 64-bit Linux systems.";

@Artturin
Copy link
Member Author

Artturin commented Sep 3, 2023

What in the output is referring to python3, it's likely a bug that this pr exposed.

$ nix why-depends $(chase result) "/nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4" --all --precise
/nix/store/fp40l5zz2f1idc2a7na5cvrxb6vjnbvw-python3.11-pydantic-1.10.9-x86_64-unknown-linux-musl
└───lib/python3.11/site-packages/pydantic/__init__.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/_hypothesis_plugin.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/annotated_types.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/class_validators.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/color.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/config.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/dataclasses.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/datetime_parse.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/decorator.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/env_settings.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/error_wrappers.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/errors.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/fields.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/json.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/main.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/mypy.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/networks.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/parse.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/schema.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/tools.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/types.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/st    → /nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4
ore/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/typing.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/utils.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/validators.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…
    lib/python3.11/site-packages/pydantic/version.cpython-311-x86_64-linux-gnu.so: …preter per process.../nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.…

image

Interpreter change detected - this module can only be loaded into one interpreter per process.
/nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.11/cpython/unicodeobject.h
/nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.11/cpython/listobject.h
from-import-* object has no __dict__ and no __all__
PyTuple_Check(((((void)((PyType_HasFeature(Py_TYPE(((PyObject*)((list)))), (1UL << 25))) || (__assert_fail("PyList_Check(list)", "pydantic/__
init__.c", 1576, __func__),0))), ((PyListObject*)((list))))->ob_item[i]))
/nix/store/rfrlbr8h85rlzwrccaqbqilsggs0n9ig-python3-3.11.4/include/python3.11/cpython/bytesobject.h
Module 'pydantic' has already been imported. Re-initialisation is not supported.
compiletime version %s of module '%.100s' does not match runtime version %s

Weird that this isn't an issue in pkgsCross.aarch64-multiplatform.python311Packages.pydantic

@Artturin
Copy link
Member Author

Artturin commented Sep 4, 2023

I suppose the first thing to try is cython_3 and or updating pydantic

ipadic and uamqp don't have this issue

@takeda
Copy link
Contributor

takeda commented Sep 4, 2023

I managed to create a repo where it could be reproduced: https://github.com/takeda/test-app

But I see you also were successful at it, so probably isn't that helpful.

Thank you for looking into it.

@takeda
Copy link
Contributor

takeda commented Sep 4, 2023

@Artturin so I just tried using cython_3 by adding this override:

    overrides = self: super: {
      pydantic = super.pydantic.overridePythonAttrs (old: {
        nativeBuildInputs = (builtins.filter (x: (x.pname or "") != "cython") old.nativeBuildInputs) ++ [
          self.cython_3
        ];
      });
    };

And it worked, but I also observed that no compilation took place. Looks like pydantic fallen back to the interpreted code.

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.

3 participants