treewide: Fix finalAttrs.doCheck by explicitly setting doCheck to the conditional in mkDerivation

[Artturin]

finalAttrs.doCheck does not include the conditionals in mkDerivation which disable doCheck on !canExecute cross.

This is better than changing all finalAttrs.doCheck to
finalAttrs.finalPackage.doCheck because it works without conditional
outputs and it addresses the comment in mkDerivation about not using
doCheck = true

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[ConnorBaker]

finalAttrs.doCheck does not include the conditionals in mkDerivation which disable doCheck on !canExecute cross.

Is it a longer term goal to update the implementation of mkDerivation to provide consistent handling of doCheck across different supported arguments (like attribute set and fixed point)?

This is better than changing all finalAttrs.doCheck to finalAttrs.finalPackage.doCheck because it works without conditional outputs and it addresses the comment in mkDerivation about not using doCheck = true

By conditional outputs, do you mean entries in outputs which depend on finalAttrs or something else?

I wasn’t aware of the comment about doCheck:

nixpkgs/pkgs/stdenv/generic/make-derivation.nix

Lines 320 to 322 in 145aee8

	# TODO(@oxij, @Ericson2314): This is here to keep the old semantics, remove when
	# no package has `doCheck = true`.
	doCheck' = doCheck && stdenv.buildPlatform.canExecute stdenv.hostPlatform;

I’m not sure I understand it either. Is removal referring to doCheck (so tests would happen so long as canExecute) or the whole line (so doCheck' is gone and doCheck is used directly or potentially removed as well under the assumption we should always run tests, per

nixpkgs/pkgs/stdenv/generic/make-derivation.nix

Lines 271 to 273 in 145aee8

	# TODO(@Ericson2314): Make unconditional / resolve #33599
	# Check phase
	doCheck ? config.doCheckByDefault or false,

).

At any rate, if doCheck = true is an anti-pattern and people should be using the (far less ergonomic) canExecute pattern, is there a way to prevent usage of the anti-pattern or make it easier to do the right thing?

[Artturin]

By conditional outputs, do you mean entries in outputs which depend on finalAttrs or something else?

There's a few attrs where a finalAttrs.finalPackage.... conditional can't be used because it'll cause infinite recursion, IIRC for example the env attrset(env = lib.optional finalAttrs.finalPackage...)

I’m not sure I understand it either. Is removal referring to doCheck (so tests would happen so long as canExecute) or the whole line (so doCheck' is gone and doCheck is used directly

Removal of the doCheck' internal conditional

At any rate, if doCheck = true is an anti-pattern and people should be using the (far less ergonomic) canExecute pattern, is there a way to prevent usage of the anti-pattern or make it easier to do the right thing?

It can't be seen during eval so it would have to be a lint.

[ConnorBaker]

It can't be seen during eval so it would have to be a lint.

For the short term, is it worth documenting this somewhere? Perhaps in the Nixpkgs reference manual? If not, then I think this is good to merge!