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
Copy Markdown
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
Copy Markdown
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 375921

@ShamrockLee
Copy link
Copy Markdown
Contributor Author

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

@ShamrockLee
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

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