-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Conversation
…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.
@amjoseph-nixpkgs Did we overlook something? Why is this working now??
|
What do you mean by "completely fine"? I still see the same diff that I posted here: I'm rebuilding to see if something somewhere else got fixed. If so I'll bisect to find out exactly what. |
yep looks like it should |
builds OK Maybe you messed up a merge or something? |
Still waiting for my build to catch up.
|
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. |
Ah, from #245475 (comment)
one of these things is not like the other. I've seen this before: 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. |
Yeah, here's where I saw that before: 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. |
Hah! #225360 (comment) |
I was able to build This PR is targeted at Rather than try to bisect through all those commits on Building on:
And:
|
|
There was a problem hiding this 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 fromx86_64-linux
) -
mips64el-linux
mixedn32
/64
router packageset (cross fromx86_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.
@@ -1,9 +1,9 @@ | |||
self: super: with self; | |||
self: dontUse: with self; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
|
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."; |
Weird that this isn't an issue in |
I suppose the first thing to try is
|
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. |
@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 reverts commit 2ffca30.
793cc9d982415b71cdba729cf779bfc49e9d2ae7
python3: splice python within python3Packages.callPackage
Was reverted because it brokepkgsCross.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
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)