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

buildPython*: allow providing a finalAttrs function #370232

Closed
wants to merge 1 commit into from

Conversation

PerchunPak
Copy link
Member

This adds support for finalAttrs function in python packages:

buildPythonPackage (finalAttrs: { # buildPythonApplication also supported
  # ...
})

just like

stdenv.mkDerivation (finalAttrs: {
  # ...
})

This implementation is alternative to #271387. The difference is that I don't depend on a stale PR, but #271387 should replace my code.

For now, I only updated a package that I maintain for testing. For treewide, #293452 would be a better place.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@nix-owners nix-owners bot requested review from mweinelt and natsukium January 2, 2025 13:28
@github-actions github-actions 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 labels Jan 2, 2025
@@ -40,21 +40,27 @@ let
result
);

allowFinalAttrs = f: lib.mirrorFunctionArgs f (args: f (lib.fix (lib.toFunction args)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there a lib function that does that?

If the process could be standardized that could be done to all builders and we could do a rec witch hunt

Copy link
Member Author

Choose a reason for hiding this comment

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

That's #234651 (for python #271387), but it is stale and unlikely to be merged soon. This PR implements a temporary solution

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I just think that it should be in lib on the final form. It'll likely be reused by other builder implementations.

@ShamrockLee
Copy link
Contributor

ShamrockLee commented Jan 2, 2025

This implementation fixes the fixed point arguments taken by mkPython* before passing down instead of leaving it to stdenv.mkDerivation. This makes the semantics completely different from that provided by stdenv.mkDerivation or overrideAttrs -- The finalAttrs here will be that of <pkg>.overridePythonAttrs (finalAttrs: previousAttrs: { }) (if we add such support) instead of <pkg>.overrideAttrs (finalAttrs: previousAttrs: { }).

One significant drawback is the lack of finalAttrs.finalPackage. While we can also re-implement the finalPackage here, that would make us depend more on custom overriders like overridePythonAttrs.

Update:

As @TomaSajt suggests, it's not even something like overridePythonAttrs (finalAttrs: previousAttrs: { }), since the finalAttrs here doesn't get updated after overriding.

@TomaSajt
Copy link
Contributor

TomaSajt commented Jan 2, 2025

As @ShamrockLee said, this is just a shorthad for lib.fix. Just using lib.fix is, IMO, perfectly acceptable if you don't want to use rec

@PerchunPak
Copy link
Member Author

I tried to avoid awful diffs, but... here we go. Rewritten everything to pass down finalAttrs to stdenv.mkDerivation

@PerchunPak PerchunPak marked this pull request as draft January 12, 2025 14:09
@PerchunPak PerchunPak force-pushed the python-finalattrs branch 2 times, most recently from 460100d to 94e35e4 Compare January 13, 2025 01:37
@PerchunPak
Copy link
Member Author

Rewritten almost everything to use overlays for handling args. The only issue I have right now is that I cannot call clearArgs inside an overlay. Maybe setting all those values to null? Doesn't sound right...

Copy link
Contributor

@TomaSajt TomaSajt left a comment

Choose a reason for hiding this comment

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

You could just not use lib.extends, but a custom function I have been calling "transforms". It takes in a transformation instead of an overlay.
If that transformation just does prevAttrs // at the beginning, it would act exactly like an overlay.
However, since we now explicitly // the prevAttrs you could remove some unneeded names from prevAttrs before //-ing it.

here's the definition of transforms btw:

transforms =
  transformation: rattrs: 
    final: transformation final (rattrs final)

And here's lib.extends to compare to

extends =
  overlay: rattrs:
    final: rattrs final // overlay final (rattrs final)

(Note the extra rattrs final //) (rattrs final is prev)

@PerchunPak PerchunPak force-pushed the python-finalattrs branch 4 times, most recently from 2326338 to fadc6d9 Compare January 14, 2025 12:08
@PerchunPak PerchunPak marked this pull request as ready for review January 14, 2025 12:59
@PerchunPak
Copy link
Member Author

BTW, @TomaSajt, you should totally add transforms to stdlib

@PerchunPak PerchunPak changed the title buildPython{Package,Application}: allow providing a finalAttrs function buildPython*: allow providing a finalAttrs function Jan 14, 2025
@TomaSajt
Copy link
Contributor

TomaSajt commented Jan 14, 2025

As @ShamrockLee mentioned previously it's very important to acknowledge is the following:

final in transformAttrs is the actual finalAttrs that's coming from inside mkDerivation
On the other hand, overridePythonAttrs is just a slap-on solution, so it only works in terms of the uppermost layer of attributes, so final in overridePythonAttrs means the fix-point of the args that will then be passed to transformAttrs.

Note:
to this day overridePythonAttrs doesn't know about whether the current package has been already overrideAttrs-ed, so it will gladly remove the effects of overrideAttrs, if done

@ShamrockLee
Copy link
Contributor

Note:
to this day overridePythonAttrs doesn't know about whether the current package has been already overrideAttrs-ed, so it will gladly remove the effects of overrideAttrs, if done

So that you know, #267296 fixes the issue. Such issues are a good example of how custom overriders are hard to maintain and are not a good pattern.

@ShamrockLee
Copy link
Contributor

You could just not use lib.extends, but a custom function I have been calling "transforms". It takes in a transformation instead of an overlay. If that transformation just does prevAttrs // at the beginning, it would act exactly like an overlay. However, since we now explicitly // the prevAttrs you could remove some unneeded names from prevAttrs before //-ing it.

here's the definition of transforms btw:

transforms =
  transformation: rattrs: 
    final: transformation final (rattrs final)

This function is similar to the lib.adaptMkDerivation in #234651 but without the functionArgs stuff.

origAttrs:
let
unfixed = (lib.toFunction origAttrs) { };
stdenv = unfixed.stdenv or python.stdenv;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stdenv = unfixed.stdenv or python.stdenv;
stdenv = origAttrs.stdenv or python.stdenv;

if origAttrs is { stdenv = ...; }, unfixed will be a callable equivalent to finalAttrs: { stdenv = "..."; }. When origAttrs is a function, it is impossible to specify stdenv as its arguments and get it out before fixing the arguments.

buildPythonPackage.override { stdenv = ...; } is the only possible and reasonable way to specify custom stdenv. That's what #271762 is for.

@PerchunPak
Copy link
Member Author

Closing in favor of #271387

@PerchunPak PerchunPak closed this Jan 22, 2025
@PerchunPak PerchunPak deleted the python-finalattrs branch January 22, 2025 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants