buildPythonPackage, buildPythonApplication: make doCheck default less confusing #308377

ShamrockLee · 2024-05-01T21:55:23Z

Description of changes

Make it clear that argument doCheck of buildPythonPackage and buildPythonApplication defaults to true instead of false.

The current implementation of mk-python-derivation.nix appears to default doCheck config.doCheckByDefault or false, but effectively default it true.

{
  stdenv,
  ...
}:
{
  doCheck ? config.doCheckByDefault or false,
  ...
}@attrs:
stdenv.mkDerivation (removeAttrs attrs [ ... ]) {
  doCheck = false;
  doInstallCheck = attrs.doCheck or true; # ???
}

This PR resolves such confusion:

{
  stdenv,
  ...
}:
{
  doCheck ? true,
  ...
}@attrs:
stdenv.mkDerivation (removeAttrs attrs [ ... ]) {
  doCheck = false;
  # The doCheck argument determines the doInstallCheck attribute.
  doInstallCheck = doCheck;
}

Another confusion caused by argument doCheck determining attribute doInstallCheck should be resolved by instructing people to specify doInstallCheck instead of doCheck and installCheckPhase instead of checkPhase. That's a change of interface that needs more discussion, and is excluded from the scope of this PR.

This PR is a back-portable fix that brings no interface changes and no rebuilds.

Cc: @FRidh @afh

Things done

Add a 👍 reaction to pull requests you find important.

afh · 2024-05-04T17:22:52Z

@ShamrockLee, thanks for this. Apologies if my confusion about around the doCheck default for regular and python packages still persist. Maybe you could help clear it up. What is the motivation for doCheck to be true for python packages / applications opposed to it being false for the (majority?) of regular packages?

Also: should this information about what (doCheck and/or doInstallCheck) to use for python packages / applications find its way to the Python-specific section of the Nixpkgs manual?

TomaSajt · 2024-05-04T17:53:05Z

I think this is the classic mistake of expecting the @attrs pattern to respect the ? pattern.

Example:

# a.nix
{ a ? "foo" }@attrs:
attrs.a or "bar"

When doing import ./a.nix {} this will evaluate to "bar" even though we told it that if a is unset set it to foo.
This is because attrs is actually just {}, exactly what we passed in.

TomaSajt · 2024-05-04T18:00:18Z

Here's the change that broke/changed the old default of false to true: f7e28bf

TomaSajt · 2024-05-04T18:10:18Z

We could also set config.doCheckByDefault or true as the default, so that it starts respecting the config value again.
This should not cause rebuilds, unless you have the config set to false, but then you're probably always expecting massive rebuilds anyway.

ShamrockLee · 2024-05-04T23:32:29Z

@afh

What is the motivation for doCheck to be true for python packages / applications opposed to it being false for the (majority?) of regular packages?

I haven't inspect into the reason why it is enabled by default. The design choice has been made years ago. What I'm doing in this PR is to uncover it.

Besides, defaulting doCheck/doInstallCheck true isn't uncommon for build helpers. Searching the codebase with regular expression do(Install)?Check = .* or true, we can find similar defaults in language-specific build helpers of Crystal, Go, Haskell, PHP, and Rust, aside from Python. Such defaults are sensible when there are some default checks or some arguments that, if set, would enable certain pre-defined checks.

Also: should this information about what (doCheck and/or doInstallCheck) to use for python packages / applications find its way to the Python-specific section of the Nixpkgs manual?

It would be great to explain them in the Nixpkgs Manual. However, I plan to propose setting it directly via doInstallCheck and installCheckPhase to make it less ambiguous. The documentation will be added there.

ShamrockLee · 2024-05-04T23:36:50Z

@TomaSajt

Here's the change that broke/changed the old default of false to true: f7e28bf

Thanks a lot for finding it out! (#68244)

We could also set config.doCheckByDefault or true as the default, so that it starts respecting the config value again. This should not cause rebuilds, unless you have the config set to false, but then you're probably always expecting massive rebuilds anyway.

According to the documentation, config.doCheckByDefault is "default to false", and "changing the default may cause a mass rebuild." AFAIK, it is unexpected for doCheckByDefault = false to cause rebuild.

TomaSajt · 2024-05-04T23:44:00Z

@TomaSajt

Here's the change that broke/changed the old default of false to true: f7e28bf

Thanks a lot for finding it out! (#68244)

We could also set config.doCheckByDefault or true as the default, so that it starts respecting the config value again. This should not cause rebuilds, unless you have the config set to false, but then you're probably always expecting massive rebuilds anyway.

According to the documentation, config.doCheckByDefault is "default to false", and "changing the default may cause a mass rebuild." AFAIK, it is unexpected for doCheckByDefault = false to cause rebuild.

Oh, I see. I didn't see that it was set by default.
I thought the docs would be something like: "either set this to true/false to force it to be true/false, or don't set it to leave the choice to the builder helpers".

I expected it to be this way, because every place that mentions this config option has an or false after it.

FRidh · 2024-05-05T08:49:21Z

Python tests have always been enabled because it is otherwise very hard to judge during build time whether the package will work at all.

…

On Sun, May 5, 2024, 01:44 Toma ***@***.***> wrote: @TomaSajt <https://github.com/TomaSajt> Here's the change that broke/changed the old default of false to true: f7e28bf <f7e28bf> Thanks a lot for finding it out! (#68244 <#68244>) We could also set config.doCheckByDefault or true as the default, so that it starts respecting the config value again. This should not cause rebuilds, unless you have the config set to false, but then you're probably always expecting massive rebuilds anyway. According to the documentation <https://nixos.org/manual/nixpkgs/unstable/#opt-doCheckByDefault>, config.doCheckByDefault is "default to false", and "changing the default may cause a mass rebuild." AFAIK, it is unexpected for doCheckByDefault = false to cause rebuild. Oh, I see. I didn't see that it was set by default. I thought the docs would be something like: "either set this to true/false to force it to be true/false, or don't set it to leave the choice to the builder helpers". — Reply to this email directly, view it on GitHub <#308377 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQHZ3Y22OESB5N55DNN4CLZAVXFNAVCNFSM6AAAAABHCTZE5CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJUGUYDGMJZGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

afh

With the information provided and my current understanding, I support this change. Someone more knowledgable may disagree and hopefully they decide to speak up and share their insight.

Thank you for putting this together, @ShamrockLee, much appreciated! And thank you everyone else for chiming in with your expertise.

nixos-discourse · 2024-06-17T13:21:22Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1756

afh · 2024-06-27T12:42:48Z

@JohnRTitor would you be willing to lend your expertise for review and if acceptable merge this PR?

natsukium · 2024-06-28T01:48:31Z

pkgs/development/interpreters/python/mk-python-derivation.nix

@@ -309,7 +309,8 @@ let

    # Python packages don't have a checkPhase, only an installCheckPhase
    doCheck = false;
-    doInstallCheck = attrs.doCheck or true;
+    # The doCheck argument determines the doInstallCheck attribute.
+    doInstallCheck = doCheck;


Suggested change

doInstallCheck = doCheck;

doInstallCheck = attrs.doCheck;

Could you clarify this with attrs?
I think it is confusing since there is a doCheck = false right above it.

Can't test it right now, but IIRC if we use the @attrs pattern, the defaults set with ? will not be applied when accessing through attrs.

Could you clarify this with attrs?

attrs does not consider the default value given by the set pattern of the build helper. It would be confusing to set a default value in the set pattern ({ doCheck ? true, ... }) but shadow it with another default when accessing (doInstallCheck = attrs.doCheck or true).

Hmmm, indeed, attrs seems to be empty, as @TomaSajt pointed out at the beginning.
Then I think attrs.doCheck or true would be easier to understand.

The confusion was originally caused by the fact that the argument, doCheck, actually evaluates to true even though it defaults to false, right?
Do we need to change this here?

Hmmm, indeed, attrs seems empty, as @TomaSajt pointed out at the beginning. Then I think attrs.doCheck or true would be easier to understand.

It still shadows the default set by the set pattern, but it sounds like an acceptable trade-off, considering the increased readability.

IMO, the root cause of the readability issue is the choice to specify the doInstallCheck attribute with the doCheck argument. In subsequent PR, I propose unifying the name (as doInstallCheck).

natsukium · 2024-06-28T01:56:13Z

pkgs/development/interpreters/python/mk-python-derivation.nix

@@ -309,7 +309,8 @@ let

    # Python packages don't have a checkPhase, only an installCheckPhase
    doCheck = false;
-    doInstallCheck = attrs.doCheck or true;
+    # The doCheck argument determines the doInstallCheck attribute.


This comment explains what it does, but I don't think it's necessary because it's just what the code says.

Modify the set pattern to hint developers that the doInstallCheck attribute of Python packages, specified by the doCheck argument of buildPythonPackage and buildPythonApplication, defaults to true instead of false.

ShamrockLee · 2024-07-02T18:44:37Z

I just rebased it onto the tip of the master branch.

natsukium

Thanks for improving this. LGTM

ShamrockLee · 2024-07-03T15:18:08Z

Thanks for merging.

I created another PR #324124 proposing directly taking the installCheck-related arguments. Please take a look and leave some feedback if you are interested.

TomaSajt · 2024-07-03T21:28:10Z

pkgs/development/interpreters/python/mk-python-derivation.nix

@@ -144,7 +144,7 @@ in

 , meta ? {}

-, doCheck ? config.doCheckByDefault or false
+, doCheck ? true


It's a bit late now, but I think this fix was not the one that needed to happen, since doCheck never gets used.
Only attrs.doCheck is ever referenced, which doesn't need this line.

Suggested change

, doCheck ? true

Should we open another PR that just removes this line?

Ah, yes. Removing this line from the set pattern is a better solution to such confusion.

github-actions bot added the 6.topic: python label May 1, 2024

ofborg 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 May 1, 2024

ShamrockLee marked this pull request as ready for review May 2, 2024 10:21

afh approved these changes May 5, 2024

View reviewed changes

TomaSajt approved these changes May 5, 2024

View reviewed changes

wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label May 5, 2024

kirillrdy requested a review from natsukium June 27, 2024 20:40

natsukium reviewed Jun 28, 2024

View reviewed changes

ShamrockLee force-pushed the build-python-docheck-quick branch from ced9eeb to 60cfc1e Compare July 2, 2024 15:41

buildPython*: make doCheck default less confusing

455314a

Modify the set pattern to hint developers that the doInstallCheck attribute of Python packages, specified by the doCheck argument of buildPythonPackage and buildPythonApplication, defaults to true instead of false.

ShamrockLee force-pushed the build-python-docheck-quick branch from 60cfc1e to 455314a Compare July 2, 2024 18:44

ShamrockLee requested a review from natsukium July 2, 2024 18:44

ShamrockLee mentioned this pull request Jul 2, 2024

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

Closed

13 tasks

natsukium approved these changes Jul 3, 2024

View reviewed changes

natsukium merged commit df6ca30 into NixOS:master Jul 3, 2024
21 checks passed

ShamrockLee deleted the build-python-docheck-quick branch July 3, 2024 19:48

TomaSajt reviewed Jul 3, 2024

View reviewed changes

ShamrockLee mentioned this pull request Jul 28, 2024

emacs: teach elisp builders the finalAttrs pattern #330589

Merged

13 tasks

jian-lin mentioned this pull request Jul 30, 2024

[Discussion] Should "unused code" that are "useful" in a file be removed (or commented out)? #331085

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buildPythonPackage, buildPythonApplication: make doCheck default less confusing #308377

buildPythonPackage, buildPythonApplication: make doCheck default less confusing #308377

ShamrockLee commented May 1, 2024 •

edited

Loading

afh commented May 4, 2024

TomaSajt commented May 4, 2024

TomaSajt commented May 4, 2024

TomaSajt commented May 4, 2024

ShamrockLee commented May 4, 2024

ShamrockLee commented May 4, 2024 •

edited

Loading

TomaSajt commented May 4, 2024 •

edited

Loading

FRidh commented May 5, 2024 via email

afh left a comment

nixos-discourse commented Jun 17, 2024

afh commented Jun 27, 2024

natsukium Jun 28, 2024

TomaSajt Jun 28, 2024

ShamrockLee Jun 29, 2024

natsukium Jul 2, 2024

ShamrockLee Jul 2, 2024 •

edited

Loading

ShamrockLee Jul 2, 2024

natsukium Jun 28, 2024

ShamrockLee Jul 2, 2024

ShamrockLee commented Jul 2, 2024

natsukium left a comment

ShamrockLee commented Jul 3, 2024

TomaSajt Jul 3, 2024

ShamrockLee Jul 3, 2024

buildPythonPackage, buildPythonApplication: make doCheck default less confusing #308377

buildPythonPackage, buildPythonApplication: make doCheck default less confusing #308377

Conversation

ShamrockLee commented May 1, 2024 • edited Loading

Description of changes

Things done

afh commented May 4, 2024

TomaSajt commented May 4, 2024

TomaSajt commented May 4, 2024

TomaSajt commented May 4, 2024

ShamrockLee commented May 4, 2024

ShamrockLee commented May 4, 2024 • edited Loading

TomaSajt commented May 4, 2024 • edited Loading

FRidh commented May 5, 2024 via email

afh left a comment

Choose a reason for hiding this comment

nixos-discourse commented Jun 17, 2024

afh commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShamrockLee Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShamrockLee commented Jul 2, 2024

natsukium left a comment

Choose a reason for hiding this comment

ShamrockLee commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShamrockLee commented May 1, 2024 •

edited

Loading

ShamrockLee commented May 4, 2024 •

edited

Loading

TomaSajt commented May 4, 2024 •

edited

Loading

ShamrockLee Jul 2, 2024 •

edited

Loading