Skip to content

buildPython*: unconditionally add .override #455362

Merged
MattSturgeon merged 1 commit intoNixOS:masterfrom
sternenseemann:build-python-lazy
Oct 24, 2025
Merged

buildPython*: unconditionally add .override #455362
MattSturgeon merged 1 commit intoNixOS:masterfrom
sternenseemann:build-python-lazy

Conversation

@sternenseemann
Copy link
Member

@sternenseemann sternenseemann commented Oct 24, 2025 •
edited by MattSturgeon
Loading

Preserve laziness of buildPython* build-helper functions, by adding override unconditionally instead of via optionalAttrs (f ? override).

Checking if f contains override causes it to be strictly evaluated. The check is also uneccessary because we know makeOverridablePythonPackage is always supplied a callPackage-result (either callPackage ./mk-python-derivation.nix { } or the python2 equivalent), so override should always be present in f.

This should resolve issues like pkgsCross.ghcjs.buildPackages.nixpkgs-openjdk-updater on staging right now (ff5098e).

Original description

This change fixes two kinds of issues:

  1. Fixes evaluation if result is neither a function nor an attribute set (which the code at least allowed before buildPython*: bring back buildPython*.override #366593).

  2. Fix evaluation of buildPython* based packages in certain splicing situations by being lazier. An example of this is pkgsCross.ghcjs.buildPackages.nixpkgs-openjdk-updater on staging right now (ff5098e). Apparently, a change that is not part of master, caused this to manifest on staging.

    By moving the addition of the set with .override inwards, we are able generating the outer __spliced set without forcing result, so we can actually access splices that don't throw.

cc @wolfgangwalther @alexfmpe

This was referenced Oct 24, 2025
Merged
Merged
MattSturgeon
MattSturgeon previously requested changes Oct 24, 2025
View reviewed changes
Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

With these changes, override is being added to the result, not the function.

I don't think this particularly changes how lazily f is evaluated; rather it just moves the override attribute to the wrong place.

github-actions[bot]

This comment was marked as resolved.

Sign in to view

@sternenseemann
Copy link
Member Author

sternenseemann commented Oct 24, 2025

With these changes, override is being added to the result, not the function.

Indeed, resolved.

@sternenseemann sternenseemann changed the title buildPython*: pull .override as much inwards as possible buildPython*: unconditionally add .override Oct 24, 2025
@nixpkgs-ci nixpkgs-ci 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. 6.topic: python Python is a high-level, general-purpose programming language. labels Oct 24, 2025
@nix-owners nix-owners bot requested review from mweinelt and natsukium October 24, 2025 21:57
MattSturgeon
MattSturgeon reviewed Oct 24, 2025
View reviewed changes
Copy link
Contributor

@MattSturgeon MattSturgeon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is a much better approach. Avoiding (f ? override) does indeed make f lazier.

If !(f ? override) accessing .override of the returned function will[...]

We shouldn't need to worry about that edge-case, because makeOverridablePythonPackage is a private function that is always applied with a callPackage result, so ? override will always be true.

With this in mind, the commit message could be shortened and put less emphasis on "we can't do (f?override)" and more emphasis on "we don't actually need to do it anyway"?

(I edited the PR description to match your revised approach)

SGTM overall, thanks!

@MattSturgeon MattSturgeon dismissed their stale review October 24, 2025 22:02

Blockers resolved

@MattSturgeon MattSturgeon requested a review from alexfmpe October 24, 2025 22:14
@sternenseemann
buildPython*: unconditionally add .override
0443b98 
To be able to splice, we can't fail to eval before returning an
attribute set. By checking for f ? override, we need to force f which
isn't always possible, whereas mirrorFunctionArgs serves as an
indirection which wraps the inner thunks in an attribute set that can
always be returned. Fortunately, there should be no case when f
evaluates successfully and does not have the override attribute, so we
can just remove the condition.

This fixes evaluation of buildPython* based packages in certain splicing
situations by being lazier. An example of this is
pkgsCross.ghcjs.buildPackages.nixpkgs-openjdk-updater on staging right
now (ff5098e).
@sternenseemann
Copy link
Member Author

sternenseemann commented Oct 24, 2025

We shouldn't need to worry about that edge-case

Done.

MattSturgeon
MattSturgeon approved these changes Oct 24, 2025
View reviewed changes
Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

An example of this is pkgsCross.ghcjs.buildPackages.nixpkgs-openjdk-updater on staging right now (ff5098e).

Is this still accurate? #366593 (comment) seems to contradict.

Either way, this is just nitpicking the commit wording; the change itself is good regardless of whether it addresses a currently-present issue.

LGTM, thanks!

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Oct 24, 2025
@alexfmpe
Copy link
Member

alexfmpe commented Oct 24, 2025 •
edited
Loading

Is this still accurate? #366593 (comment) seems to contradict.

staging contains the breaking commit but not yet the fixing one

$ git checkout staging
$ git rev-parse HEAD
ed14c53d9e2c95dc22a6283d3537699d643a8d26
$ git log | grep -i buildPython | head -n 8
    buildPython*: bring back buildPython*.override (#366593)
    buildPython*: bring back buildPython*.override
    and unshadow `buildPython*.override`.
    This makes it possible to override the dependencies of buildPython*.
    E.g., `buildPythonPackage.override { unzip = unzip-custom; }`
    returns a derived version of `buildPythonPackage` with
    buildPythonPackage: Enable direct attribute overriding via overrideAttrs (the non-staging ones) (#376372)
    python3Packages.buildPythonPackage: remove ineffective outputs cleanAttrs

$ git checkout master
$ git rev-parse HEAD
a811a240963a40a979ec7fcdbf75305d35a3f20b
$ git log | grep -i buildPython | head -n 8
    Enable `stdenv` customization through `(buildPython*.override { inherit stdenv; })` (#271762)
    buildPython*: warn about deprecated argument stdenv
    warn about the use of buildPython*'s deprecated argument `stdenv`.
    doc: document buildPython* stdenv overriding via <function>.override
    python3Packages.torch: specify custom stdenv by overriding buildPythonPackage
    buildPython*: allow stdenv customization through <function>.override
    buildPython*: bring back buildPython*.override (#366593)
    buildPython*: bring back buildPython*.override

@MattSturgeon
Copy link
Contributor

MattSturgeon commented Oct 24, 2025

However, the very first commit of #271762, which this PR unblocked, fixes the eval issue again and was merged soon after.

staging contains the breaking commit but not yet the fixing one

c4e3b67 adds an earlier // { override }, but keeps the problematic one outer // optionalAttrs (f ? override) { override } fixed by this PR. I assume that the earlier fixed // { override } is giving nix a way to optimize away the strictness, or at least avoid the symptoms caused by the strictness. So it makes sense that c4e3b67 would "resolve" or at least "mask" the issue.

Thanks both for your thorough investigation, analysis, fixes, improvements, etc!

I'll merge this now, as it is an objectively good patch. Even if c4e3b67 means we won't see any changes in practice with modern nix implementations, it's still good to remove unnecessary potential strictness from the code.

@MattSturgeon MattSturgeon added this pull request to the merge queue Oct 24, 2025
Merged via the queue into NixOS:master with commit 19144d4 Oct 24, 2025
27 of 31 checks passed
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented Oct 24, 2025

Successfully created backport PR for release-25.05:

@nixpkgs-ci nixpkgs-ci bot mentioned this pull request Oct 24, 2025
Merged
1 task
@github-actions github-actions bot added the 8.has: port to stable This PR already has a backport to the stable release. label Oct 24, 2025
@sternenseemann sternenseemann deleted the build-python-lazy branch October 25, 2025 09:30
@ShamrockLee ShamrockLee added the backport release-25.11 Backport PR automatically label Jan 5, 2026
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented Jan 5, 2026

Backport failed for release-25.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-25.11
git worktree add -d .worktree/backport-455362-to-release-25.11 origin/release-25.11
cd .worktree/backport-455362-to-release-25.11
git switch --create backport-455362-to-release-25.11
git cherry-pick -x 0443b98ab3aa88d5eb7ff231781d679be89868e8

@ShamrockLee
Copy link
Contributor

ShamrockLee commented Jan 5, 2026

Ah! It's already inside release-25.11.

@ShamrockLee ShamrockLee removed the backport release-25.11 Backport PR automatically label Jan 5, 2026
@ShamrockLee ShamrockLee mentioned this pull request Jan 5, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MattSturgeon MattSturgeon MattSturgeon approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

@ShamrockLee ShamrockLee Awaiting requested review from ShamrockLee

@natsukium natsukium Awaiting requested review from natsukium

@mweinelt mweinelt Awaiting requested review from mweinelt

@alexfmpe alexfmpe Awaiting requested review from alexfmpe

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 8.has: port to stable This PR already has a backport to the stable release. 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. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sternenseemann @alexfmpe @MattSturgeon @ShamrockLee