Skip to content

buildPythonPackage: redirect pre/postCheck to pre/postInstallCheck#379637

Open
ShamrockLee wants to merge 3 commits intoNixOS:stagingfrom
ShamrockLee:build-python-precheck
Open

buildPythonPackage: redirect pre/postCheck to pre/postInstallCheck#379637
ShamrockLee wants to merge 3 commits intoNixOS:stagingfrom
ShamrockLee:build-python-precheck

Conversation

@ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Feb 5, 2025 •
edited
Loading

This PR unifies the .overrideAttrs interface for Python packages regarding preInstallCheck and postInstallCheck in a compatible way.

  • Specify preInstallCheck/postInstallCheck as preCheck/postCheck if the latter is not specified.
  • Use preInstallCheck/postInstallCheck in buildPython* setup hooks.
  • When the checkPhase argument is specified, replace runHook pre/postCheck with runHook pre/postInstallCheck before assigning it to installCheckPhase.

After this change, pytestCheckPhase now precedes sphinxBuildPhase if both invoked, and locale changes left by pytestCheckPhase could trigger Sphinx's failure (sphinx-doc/sphinx#11739) (despite the topic of the linked issue, the error also occurs on Linux). As a result, this PR depends on a treewide locale cleanup:

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 Feb 5, 2025
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Feb 5, 2025

@ofborg build python3Packages.aggdraw
@ofborg build python3Packages.accelerate

@github-actions github-actions bot added 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels Feb 5, 2025
@ShamrockLee ShamrockLee mentioned this pull request Feb 5, 2025
Open
@ShamrockLee ShamrockLee force-pushed the build-python-precheck branch from 06891d7 to 2d76e4a Compare February 14, 2025 07:12
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Feb 14, 2025 •
edited
Loading

I rebased the feature branch onto the merge base of master and staging to shorten the local build time (on my quad-core tablet).

@ShamrockLee ShamrockLee force-pushed the build-python-precheck branch from 2d76e4a to 72b6cda Compare February 14, 2025 08:57
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Feb 14, 2025

@ofborg build python3Packages.accelerate

@ShamrockLee ShamrockLee force-pushed the build-python-precheck branch from 72b6cda to 1993221 Compare March 9, 2025 22:21
@ShamrockLee ShamrockLee changed the title buildPython*: redirect pre/postCheck to pre/postInstallCheck buildPythonPackage: redirect pre/postCheck to pre/postInstallCheck Mar 9, 2025
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 6.topic: games Gaming on NixOS 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: documentation This PR adds or changes documentation labels Mar 9, 2025
@ShamrockLee ShamrockLee force-pushed the build-python-precheck branch from 1993221 to 84b7c28 Compare March 9, 2025 22:21
@github-actions github-actions bot removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 6.topic: games Gaming on NixOS 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Mar 9, 2025
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Mar 9, 2025

@ofborg build python3Packages.termcolor

@ShamrockLee ShamrockLee marked this pull request as ready for review March 23, 2025 14:45
@nix-owners nix-owners bot requested a review from natsukium March 23, 2025 14:47
@nix-owners nix-owners bot requested a review from mweinelt March 23, 2025 14:47
@ShamrockLee ShamrockLee force-pushed the build-python-precheck branch from 84b7c28 to 1cd1c85 Compare July 22, 2025 22:26
@nixpkgs-ci nixpkgs-ci bot added the 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. label Jul 22, 2025
@ShamrockLee ShamrockLee force-pushed the build-python-precheck branch from 1cd1c85 to 1920589 Compare October 23, 2025 12:24
@ShamrockLee ShamrockLee mentioned this pull request Oct 23, 2025
Draft
13 tasks
MattSturgeon
MattSturgeon approved these changes Dec 11, 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.

It feels like there may be a way to use the finalAttrs fixpoint to keep checkPhase and installCheckPhase in sync... Maybe in combination with using phases to exclude checkPhase? That way, it'd be possible to use/override finalAttrs.checkPhase via overrideAttrs (etc). That may be an alternative solution to #324124 too. That said, I think it'd only be possible as a one-way binding; a two-way binding would suffer infinite recursion.

Regardless, it feels correct that the installCheckPhase would run the {pre,post}InstallCheck hooks - so this PR SGTM in concept. The diff also looks good overall, but I've not tested anything.

pkgs/development/interpreters/python/mk-python-derivation.nix
Comment on lines +440 to +442
lib.replaceStrings
[ "runHook preCheck\n" "runHook postCheck\n" ]
[ "runHook preInstallCheck\n" "runHook postInstallCheck\n" ]
Copy link
Contributor

@MattSturgeon MattSturgeon Dec 11, 2025

Choose a reason for hiding this comment

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

This doesn't feel particularly robust, but it should cover 99% of cases.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Dec 11, 2025
@ShamrockLee
python3Packages.buildPythonPackage: redirect pre/postCheck to pre/pos…
cd199e2 
…tInstallCheck

Goal: Unify the <pkg>.overrideAttrs interface for Python packages.

Specify preInstallCheck/postInstallCheck as preCheck/postCheck
if the latter is not specified.

Use preInstallCheck/postInstallCheck in buildPython* setup hooks.

When the checkPhase argument is specified,
replace `runHook pre/postCheck` with `runHook pre/postInstallCheck`
before assigning it to `installCheckPhase`.
@ShamrockLee
python3Packages.pytestCheckHook: assign pytestCheckPhase to installCh…
af2d8a4 
…eckPhase

Simplify implementation and ensure that
pre/postInstallCheck only run once.
@ShamrockLee
python3Packages.unittestCheckHook: assign unittestCheckPhase to insta…
e55131e 
…llCheckPhase

Simplify implementation and ensure that
pre/postInstallCheck only run once.
@ShamrockLee ShamrockLee force-pushed the build-python-precheck branch from 1920589 to e55131e Compare December 11, 2025 03:57
@ShamrockLee ShamrockLee mentioned this pull request Dec 11, 2025
Draft
13 tasks
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Dec 11, 2025

It feels like there may be a way to use the finalAttrs fixpoint to keep checkPhase and installCheckPhase in sync...

buildPythonPackage has been passing checkPhase to installCheckPhase once received and drop checkPhase immediately long before this PR, and I'm a bit reluctant to change it.

How about teaching overridePythonAttrs to keep checkPhase and installCheckPhase in sync? I made this change at PR #469804 , which should cause few or no rebuilds after this PR is merged.

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

@natsukium natsukium Awaiting requested review from natsukium

@mweinelt mweinelt Awaiting requested review from mweinelt

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 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.

2 participants

@ShamrockLee @MattSturgeon