python3Packages: don't depend on setup hooks' bash eval#352976
python3Packages: don't depend on setup hooks' bash eval#352976emilazy merged 3 commits intoNixOS:stagingfrom
Conversation
github-actions
bot
added
the
6.topic: python
ofborg
bot
added
the
2.status: merge conflict
wolfgangwalther
force-pushed
the
no-python-eval-staging
branch
from
0457651 to
2a63643
Compare
|
I see On a serious note, my opinion is we should not be changing the semantics of such an old variable, we need a new name, and maybe we deprecate the old with a warning. I'm slightly concerned that with this and with the previous |
Yes, that's the plan. The idea is to create a This PR is just a preparation to make the change from Regarding
Uhm, isn't that the case with every change to stdenv / any hook etc.? I think the expectation to be able to put an expression into a different nixpkgs version and have it work is unreasonable in the general case. Although... external overrides are exactly that, right? Hm.... |
My thinking too. Overlays can access
Yes. I'd reckon it's OK because we're rather conservative about touching stdenv and hooks (EDIT: isn't this how structuredAttrs still aren't the default?..). Which I reckon we are at least partly because we don't know how people might be (ab)using nixpkgs "internals" out-of-tree. We could know a tad more if we advertised a more explicit interface, e.g. versioning |
ofborg
bot
removed
the
2.status: merge conflict
ofborg
bot
added
10.rebuild-darwin-stdenv
Aleksanaa
changed the title
wolfgangwalther
force-pushed
the
no-python-eval-staging
branch
from
2a63643 to
76a35f3
Compare
|
@ofborg build picard Test pytest-xdist |
|
Oops! I didn't mean to DoS our CI 😓 |
|
but you explicitly asked for DoS‐age! |
|
python3Packages.certbot-dns-{cloudflare,ovh,rfc2136,route53} fails to build on master, complaining I filed an issue in Nixpkgs (#354054). Such issue might also need to be upstreamed. |
I think those could be fixed on staging already, but I might mis-remember. |
We must check if they failed to build on master or staging since they failed after this change. |
Yeah, I didn't want to rebase while all the ofborg stuff was running, so might have been after where the branch is currently on. |
Oops! I switched to the wrong commit. Now I'm waiting for the correct one to build. |
|
Since this is causing a lot of rebuilds and thus is hard to test, I am thinking of splitting it up in two pieces - one going to master with everything that causes fewer rebuilds, and one part staying here, causing the big number of rebuilds, but possibly easier to review alone. That is.. unless someone is already convinced that this PR is good to go? Then I'd just rebase over the trivial conflict in numcodecs. |
Sounds great!
These changes looks good for me. Considering those extra flags and pre/post steps contain package-specific tricks to make them build, it's still safer to at least test if they build successfully.
|
…ldFlags We'd like to remove bash eval for setupByBuildFlags, thus use appendToVar in the hook instead. Tested that linuxPackages.chipsec still builds, including the driver in the right place.
ofborg
bot
requested review from
JohnAZoidberg,
domenkozar,
doronbehar and
erdnaxe
ofborg
bot
added
10.rebuild-darwin: 101-500
wegank
added
12.approvals: 1
|
@ShamrockLee After the other PRs have been merged, shall we continue with this one? I think this should be good to go, do you agree? |
|
LGTM!
We could also re-target it to the `master` branch if the number of
rebuilds are low enough.
On 2024-11-15 04:23, Wolfgang Walther wrote:
@ShamrockLee [1] After the other PRs have been merged, shall we continue with this one? I think this should be good to go, do you agree?
--
Reply to this email directly, view it on GitHub [2], or unsubscribe [3].
You are receiving this because you were mentioned.Message ID: ***@***.***>
Links:
------
[1] https://github.com/ShamrockLee
[2] #352976 (comment)
[3]
https://github.com/notifications/unsubscribe-auth/AKQF2MZNNKVU7IU33A5ZB3D2AUBCXAVCNFSM6AAAAABRA5CBEGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINZXGMZTSMJVGM
|
I guess we could, but I think it's not urgent and we can just as well merge it into staging as well. So I'd keep it here. @emilazy this should be good to go. |
|
Has this been backported into 24.11 yet? |
|
No, it hasn't and I don't think that's the plan. |
|
Why? Packages are broken and need to be fixed anyway. |
|
This PR was not about fixing any broken packages. It's more of a refactor to enable other changes in the future. |
|
@wolfgangwalther I re-built from the PR, and the broken packages now build. More specifically, it fixes #354054. |
|
@aviallon My two cents: We could consider backporting changes that don't add additional working directory changes. As for other changes with additional |
I don't know how you come to that conclusion. My observations:
Nothing indicates that any of the changes here fix the build for those packages anywhere. |
|
@wolfgangwalther, I'm rebuilding from master right now. I may have built from a commit that is slightly too old. |
7 participants
This came up in #335110 and then in #347194.
The python ecosystem currently uses bash
evalin some setup hooks - and of course some packages depend on that. Mostly inpytestFlagsArray, but also insetupPyBuildFlagsandsetupPyGlobalFlags.This PR deals with all those cases, replacing it with more adequate solutions. This will allow us to replace
pytestFlagsArraywith a structuredAttrs-supportingpytestFlagsalternative more easily later on.@ShamrockLee @SomeoneSerge @emilazy @philiptaron
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.