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
python3Minimal: disable optimizations #89489
Conversation
@GrahamcOfBorg eval |
need to use fetchurl instead of fetchpatch |
@GrahamcOfBorg eval |
ofborg does not like fetching patches when the derivation is used during bootstrapping. This reverts commit 480c8d1.
f7c2c2c
to
7532a05
Compare
@GrahamcOfBorg eval |
I fail to see how this can still be an issue. |
No point for the bootstrapping.
( | ||
if pythonAtLeast "3.8" then | ||
fetchpatch { | ||
url = "https://salsa.debian.org/cpython-team/python3/-/raw/3.8.3rc1-1/debian/patches/link-opt.diff"; |
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.
All of these are files in git, not generated files so fetchurl
should be sufficient.
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.
yes, it should be, yet ofborg complains.
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 see the error even on a337c44 and it points to the removed line 🤷♀️
@GrahamcOfBorg eval |
Let's see how it behaves with other PR's. |
Sorry these changes caused things to fail! I think I don't yet follow what the errors were. Is there a place I can see what ofborg said about this version, or a way I can reproduce it locally? It looks like with this PR, the PGO (worth a ~16% speedup in my measurements) is disabled for python3Minimal but still active for the other builds, and the linker optimization (worth ~6%) is reverted for all builds. If |
I also didn't understand what went on here. @FRidh you wrote:
But in this PR you removed the patches themselves - you didn't change the way they were fetched. |
@doronbehar Yes, they can be added back. I needed a way to get staging building again asap, and as I wrote
it turned out to be two issues at the same time (solution 913bee3) and ofborg giving the same error while that was not possible (#89489 (comment)) . So, reverting this can be undone as long as @gnprice actually the PR was fine, the issue was that I merged it after I made Python 3.8 the default interpreter, and did not let ofborg check it again. |
No point for the bootstrapping, and not possible because patches are
fetched.
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)