Skip to content

buildPython*: restructure in preparation for fixed-point argument support and overridePythonAttrs deprecation#375921

Merged
ShamrockLee merged 4 commits intoNixOS:masterfrom
ShamrockLee:build-python-restructure
Feb 2, 2025
Merged

buildPython*: restructure in preparation for fixed-point argument support and overridePythonAttrs deprecation#375921
ShamrockLee merged 4 commits intoNixOS:masterfrom
ShamrockLee:build-python-restructure

Conversation

@ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Jan 22, 2025

This PR restructures the expression layout to prepare for implementing the fixed-point attribute support (#271387) and the transition from overridePythonAttrs to overrideAttrs (#376372). #271387 depends on #271762, which might need additional discussion and consensus. This PR makes it easier to proceed with #376372 simultaneously.

The only part of this PR that touches the programming logic is to use drv.disabled (from <pkg>.passthru) instead of attrs.disabled (from the buildPython* arguments) to determine whether to throw the interpreter-unsupported error. This is necessary for transferring to lib.extendMkDerivation, as transformDrv must be independent of the build-helper arguments.

Please consider reviewing this PR commit-by-commit, as the total diff might not be intuitive to read after formatting.

How the layout changes

Before:

# buildPython*.override set pattern
{
  lib,
  python,
  hooks,
}:

let
  argument-independent-utils = "..."; # <---

  cleanAttrs = lib.flip removeAttrs [
    "..."
  ];
in

# buildPython* arguments
{
  name,
  pyproject,
  ...
}@attrs:

let
  argument-dependent-utils = "...";

  # The result Python package
  self = toPythonModule (
    stdenv.mkDerivation (
      cleanAttrs attrs
      // {
        # Phases
      }
    )
  );
in
extendDerivation (
  disabled -> throw "${name} not supported for interpreter ${python.executable}"
) { } self

After this PR:

# buildPython*.override set pattern
{
  lib,
  python,
  hooks,
}:

let
  argument-independent-utils = "...";

  cleanAttrs = lib.flip removeAttrs [
    "..."
  ];
in

# buildPython* arguments
{
  name,
  pyproject,
  ...
}@attrs:

let
  # The result Python package
  self = stdenv.mkDerivation (
    finalAttrs:
    let
      argument-dependent-utils = "..."; # <---
    in
    cleanAttrs attrs
    // {
      # Phases
    }
  )
);

  # This derivation transformation function must be independent to `attrs`
  # for fixed-point arguments support in the future.
  transformDrv =
    drv:
    extendDerivation (
      drv.disabled
      -> throw "${lib.removePrefix namePrefix drv.name} not supported for interpreter ${python.executable}"
    ) { } (toPythonModule drv);
in
transformDrv self

After adopting lib.extendMkDerivation (#271387)

# buildPython*.override set pattern
{
  lib,
  python,
  stdenv,
  hooks,
}:

let
  argument-independent-utils = "...";
in

lib.extendMkDerivation {
  constructDrv = stdenv.mkDerivation;

  excludeDrvArgNames = [
    "..."
  ];

  extendDrvArgs =
    finalAttrs:
    # buildPython* arguments
    {
      name,
      pyproject,
      ...
    }@attrs:

    let
      argument-dependent-utils = "..."; # <---
    in
    {
      # Phases
    };

  transformDrv =
    drv:
    extendDerivation (
      drv.disabled
      -> throw "${lib.removePrefix namePrefix drv.name} not supported for interpreter ${python.executable}"
    ) { } (toPythonModule drv);
}

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.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Jan 22, 2025
@ShamrockLee ShamrockLee force-pushed the build-python-restructure branch from 22007a8 to b2644e7 Compare January 22, 2025 21:09
@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 22, 2025
@ShamrockLee
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 375921

@ShamrockLee
Copy link
Contributor Author

I split the restructuring step into four commits for easier reviewing and merging.

@ShamrockLee
Copy link
Contributor Author

I split the miscelaneous changes before restructuring as a separate PR #378566. Let's merge it first.

…alAttrs

This commit is intentionally unformatted
for smoother merging and rebasing experience.
This commit is intentionally unformatted
for smoother merging and rebasing experience.
@ShamrockLee ShamrockLee force-pushed the build-python-restructure branch from 761d2df to 3678c2e Compare February 2, 2025 06:27
@ShamrockLee
Copy link
Contributor Author

#378566 has been merged, and I have rebased this feature branch.

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

After staring at it for a while and going through the commits one by one as suggested, this change makes sense to me and has no rebuilds.

@ShamrockLee
Copy link
Contributor Author

@philiptaron Thank you for review and approval.

As this PR might not be an easy merge into staging (due to the reformatting here and the __structuredAttrs refactoring on staging), and that it conflicts with #376419. How about merging #376419 first and rebasing this one to make it easier for the staging people?

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 2, 2025
@ShamrockLee
Copy link
Contributor Author

Update: Merging this first and rebasing #376419 doesn't add extra friction when merging into staging. The only drawback is that #376419 would be unable to backport automatically.

I'll merge this one first to push dependent changes forward.

@ShamrockLee ShamrockLee merged commit cfd4b92 into NixOS:master Feb 2, 2025
50 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 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

Development

Successfully merging this pull request may close these issues.

4 participants