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.

Sign up for GitHub

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

Jump to bottom

buildPythonPackage , buildPythonApplication: take installCheck-related attributes directly #324124

Closed
wants to merge 5 commits into from
Closed

buildPythonPackage , buildPythonApplication: take installCheck-related attributes directly #324124

wants to merge 5 commits into from

Conversation

ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Jul 2, 2024 •
edited
Loading

Description of changes

This PR enables buildPythonPackage and buildPythonApplicaiton to directly take the installCheck- related attribute instead of specifying it using check- associated arguments.

This is the first and backward-compatible step toward simplifying the installCheck-related attribute specification for Python packages and applications.

In the long run, we should deprecate the use of checkPhase to specify installCheckPhase, and use installCheckPhase directly, as stated in the mk-python-derivation comment four years ago.

Cc: @FRidh

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Jul 2, 2024
@ShamrockLee ShamrockLee changed the title Build python install check rename buildPythonPackage , buildPythonApplication: take installCheck-related attributes directly Jul 2, 2024
@ofborg ofborg bot requested a review from fabaff July 2, 2024 20:41
@ShamrockLee ShamrockLee marked this pull request as ready for review July 3, 2024 15:01
@ShamrockLee ShamrockLee mentioned this pull request Jul 3, 2024
Merged
13 tasks
afh
afh reviewed Jul 3, 2024
View reviewed changes
Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Thanks for continuing the work on this, @ShamrockLee, please find below some minor suggestions…

doc/languages-frameworks/python.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/python.section.md Outdated Show resolved Hide resolved
@ShamrockLee ShamrockLee force-pushed the build-python-install-check-rename branch from 055d61d to 089cbc9 Compare July 3, 2024 19:48
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 6, 2024
@ShamrockLee ShamrockLee force-pushed the build-python-install-check-rename branch 2 times, most recently from 167f116 to b922bfb Compare July 7, 2024 23:53
@mweinelt
Copy link
Member

mweinelt commented Jul 8, 2024 •
edited
Loading

I believe to understand the distinction between checkPhase and installCheckPhase in the meaning of the stdenv chapter in the manual. What I don't understand is why Python adopted one over the other, and what the motivation and benefit of rolling that decision back is.

This decision seems to date back to 2009.

@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Jul 8, 2024 •
edited
Loading

@mweinelt Thank you for finding out the history of the decision.

The issue is that we are

  • specifying the installCherkPhase attributes using the checkPhase arguments of buildPython* and
  • pretending that we are using checkPhase in the corresponding section of the Nixpkgs Manual.

It is not about choosing between chcekPhase and installCheckPhase but about informing users of the build helpers about this choice.

@mweinelt
Copy link
Member

mweinelt commented Jul 8, 2024

Why then not move us into checkPhase if it is all the same? I don't see the value in adopting another lingo when a whole bunch of packages have already adopted it.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 8, 2024
@ShamrockLee
Copy link
Contributor Author

That's indeed another way to go.

The decision to implement doCheck with doInstallCheck dates back to 2016, in commit 3e05cce made by @FRidh in #18143. The comment mentioning the long-term goal to move to installCheckPhase was added in 2019 in commit f7e28bf.

@natsukium
Copy link
Member

The issue is that we are

  • specifying the installCherkPhase attributes using the checkPhase arguments of buildPython* and
  • pretending that we are using checkPhase in the corresponding section of the Nixpkgs Manual.

It is not about choosing between chcekPhase and installCheckPhase but about informing users of the build helpers about this choice.

The manual notes that checkPhase is used instead of installCheckPhase.
However, I do not know why we adopted checkPhase and not installCheckPhase as it was.
Perhaps because installCheckPhase was rarely used and unfamiliar to most people?

The checkPhase for python maps to the installCheckPhase on a normal derivation. This is due to many python packages not behaving well to the pre-installed version of the package. Version info, and natively compiled extensions generally only exist in the install directory, and thus can cause issues when a test suite asserts on that behavior.

https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/python.section.md#testing-python-packages-testing-python-packages

@mweinelt
Copy link
Member

mweinelt commented Jul 8, 2024

I get that installCheckPhase runs post-install and is more useful for the reasons mentioned.

The frustrating thing here is, that we simplified the lingo to build-system and dependencies, only to move from checkInputs to nativeCheckInputs, and now to nativeInstallCheckInputs. I get that all of these bits and pieces have meaning, but we'll be dealing with the resulting churn for the next two releases at least.

And then there is the matter of nix-community/nix-init#419 🫠

@FRidh
Copy link
Member

FRidh commented Jul 8, 2024

I get that installCheckPhase runs post-install and is more useful for the reasons mentioned.

The frustrating thing here is, that we simplified the lingo to build-system and dependencies, only to move from checkInputs to nativeCheckInputs, and now to nativeInstallCheckInputs. I get that all of these bits and pieces have meaning, but we'll be dealing with the resulting churn for the next two releases at least.

And then there is the matter of nix-community/nix-init#419 🫠

Build system and dependencies are essentially another layer on top. The underlying stdenv atttributes will always be needed for all other cases.

I think motivation at the time was consistency. Probably it was not given much thought.

@ShamrockLee @mweinelt
buildPythonPackage, buildPythonApplication: take installCheck* directly
e366630 
Take attributes doInstallCheck, installCheckPhase, installCheckInputs,
and their native companions directly.

Make arguments doCheck, checkPhase, checkInputs, and their native
companions, alias to the installCheck arguments.

Co-authored-by: Martin Weinelt <[email protected]>
ShamrockLee and others added 4 commits July 11, 2024 15:44
@ShamrockLee @afh
doc python.section.md: specify installCheck-related attributes directly
920188c 
Change checkPhase to installCehckPhase, doCheck to doInstallCheck, ...,
to reflect the fact that the specification goes to the
installCheck-related attributes and that buildPython* build helpers now
takes the installCheck-related attributes directly in addition to
indirectly specify them thourgh the check-related arguments.

Documentation about the preCheck and postCheck phases of *CheckHook's
(pytestCheckHook, setuptoolsCheckHook, and unittestCheckHook) are
deliberately kept unchanged, as they are not renamed to preInstallCheck
and postInstallCheck yet.

Co-authored-by: Alexis Hildebrandt <[email protected]>
@ShamrockLee
erosmb: fix do-test specification
c420a2e 
In the previous implementation of buildPythonApplication, the
doInstallCheck attribute was always specified by the doCheck argument.
as `doCheck = false` was specified by this package, there's no effect to
specify `doInstallCheck = true`, and the `installCheckPhase` specified
doesn't get executed during build.

After buildPythonApplication takes `doInstallCheck` directly,
`doInstallCheck = true` starts to take effect. The `doCheck` argument,
as an alias to `doInstallCheck`, lost its effect when `doInstallCheck`
is presented.

The setuphookCheckPhase are still supressed due to the fact that
installCheckPhase is specified non-empty.
@ShamrockLee
glasgow: remove irrelevent attribute specification
f406efd 
In the previous implementation of buildPythonApplication, the
doInstallCheck attribute is always specified by the doCheck argument,
which defaults to true. The doInstallCheck = false specification didn't
take effect. It is the non-empty installCheckPhase attribute, specified
by the checkPhase argument, that supress the setuphookCheckPhase.

After `buildPythonPackage` starts taking doInstallCheck directly, the
doInstallCheck = false specification starts to take effect, supressing
the execution of the installCheckPhase attribute specified by the
checkPhase argument.

This patch removes the irrelevant doInstallCheck = false specifcation,
keeping the attribute value true and installCheckPhase executed.
@ShamrockLee @mweinelt
Add release note entry about buildPython* taking installCheck-relatd …
1ca8ebf 
…attributes

Co-authored-by: Martin Weinelt <[email protected]>
@ShamrockLee ShamrockLee force-pushed the build-python-install-check-rename branch from 8091f9c to 1ca8ebf Compare July 11, 2024 07:45
@natsukium
Copy link
Member

After some thought, I'm personally against the migration to installCheckPhase for now.
It has been over six months since we started the switch to build-system/dependencies, and this migration is painful.

Indeed, this change is correct and should be done to ensure consistency with the phase definition.
However, it will be more difficult to encourage contributors to migrate just for that reason, even if it is backwards compatible.

Regarding installCheckPhase, I feel that many of its functions have delegated passthru.tests, such as tester.testVersion.
It is no exception in python, where #272177 tries to separate build and test.
I don't know how it will finally be implemented, but it seems that it would be less burdensome if this PR change were adopted at the same time as the separation.

@ShamrockLee
Copy link
Contributor Author

After some thought, I'm personally against the migration to installCheckPhase for now.
It has been over six months since we started the switch to build-system/dependencies, and this migration is painful.

Indeed, this change is correct and should be done to ensure consistency with the phase definition.
However, it will be more difficult to encourage contributors to migrate just for that reason, even if it is backwards compatible.

Thank you for sharing your precious experiences about attribute renaming.

Your right. The change wouldn't be helpful to the community if the pain caused by the mass renaming outweighs the benefits of the name correctness.

Regarding installCheckPhase, I feel that many of its functions have delegated passthru.tests, such as tester.testVersion.
It is no exception in python, where #272177 tries to separate build and test.
I don't know how it will finally be implemented, but it would be less burdensome if this PR change were adopted at the same time as the separation.

Delegating checks to passthru.test (as package tests) sounds like the right way to go. Let's push that one first.

If this is not the change we want to make now, should we close this pull request or mark it as a draft?

@ShamrockLee ShamrockLee marked this pull request as draft July 18, 2024 20:30
@ShamrockLee ShamrockLee mentioned this pull request Oct 27, 2024
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mweinelt mweinelt mweinelt left review comments

@afh afh afh left review comments

@fabaff fabaff Awaiting requested review from fabaff

@natsukium natsukium Awaiting requested review from natsukium

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: changelog 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ShamrockLee @mweinelt @natsukium @FRidh @afh @wegank