treewide: finalAttrs.doCheck -> finalAttrs.finalPackage.doCheck

[pbsds]

Description of changes

This will respect doCheck = false; overrides, common for cross.

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.05 Release Notes (or backporting 23.05 and 23.11 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.

Priorities

Add a 👍 reaction to pull requests you find important.

[roberth]

This will respect doCheck = false; overrides

Doesn't it already?

nix-repl> hello.doCheck                                      
true

nix-repl> (hello.overrideAttrs { doCheck = true; }).doCheck
true

nix-repl> (hello.overrideAttrs { doCheck = false; }).doCheck
false

nix-repl> hello                                    
«derivation /nix/store/nvl9ic0pj1fpyln3zaqrf4cclbqdfn1j-hello-2.12.1.drv»

nix-repl> hello.overrideAttrs { doCheck = true; }            
«derivation /nix/store/nvl9ic0pj1fpyln3zaqrf4cclbqdfn1j-hello-2.12.1.drv»

nix-repl> hello.overrideAttrs { doCheck = false; }
«derivation /nix/store/6n4az7cnydvgj1ixk4bmg50vx7nih3xm-hello-2.12.1.drv»

Fwiw, it wouldn't work with cleanAttrs when going through finalPackage.
Also theoretically you could "fake it" with passthru then.

[pbsds]

Consider:

nixpkgs/pkgs/development/libraries/libspatialindex/default.nix

Lines 25 to 27 in ad5e744

	cmakeFlags = [
	"-DSIDX_BUILD_TESTS=${if finalAttrs.finalPackage.doCheck then "ON" else "OFF"}"
	];

On master, without finalPackage:

nix-repl> libspatialindex.cmakeFlags                                 
[ "-DSIDX_BUILD_TESTS=ON" ]

nix-repl> pkgsCross.aarch64-multiplatform.libspatialindex.cmakeFlags
[ "-DSIDX_BUILD_TESTS=ON" "-DCMAKE_SYSTEM_NAME=Linux" "-DCMAKE_SYSTEM_PROCESSOR=aarch64" "-DCMAKE_HOST_SYSTEM_NAME=Linux" "-DCMAKE_HOST_SYSTEM_PROCESSOR=x86_64" ]

with this PR:

nix-repl> libspatialindex.cmakeFlags
[ "-DSIDX_BUILD_TESTS=ON" ]

nix-repl> pkgsCross.aarch64-multiplatform.libspatialindex.cmakeFlags
[ "-DSIDX_BUILD_TESTS=OFF" "-DCMAKE_SYSTEM_NAME=Linux" "-DCMAKE_SYSTEM_PROCESSOR=aarch64" "-DCMAKE_HOST_SYSTEM_NAME=Linux" "-DCMAKE_HOST_SYSTEM_PROCESSOR=x86_64" ]

[roberth]

Right, I forgot that mkDerivation applies some logic between doCheck coming in and doCheck going out.
Makes sense. It's unfortunate that we don't have a more direct way to access that outgoing doCheck, but I don't see a good enough alternative that doesn't massively rewrite make-derivation.nix, other than yours!

[SuperSandro2000]

This is super unintuitive and I expect that you can create such a PR every 6 months. Can we somehow prevent this or do something to prevent this error or fix the underlying issue?

[pbsds]

Either the cross logic needs to take place during the finalAttrs fixed-point, or we do this everywhere.

[SuperSandro2000]

It would be a bad idea to set this by default in mkDerivation, would it?

[pbsds]

doCheck is by default false as i understand.

[roberth]

fix the underlying issue?

A redesign of mkDerivation, under a new name. This is a hard problem because of the requirements

• stay as close as possible to the mkDerivation
• share as much implementation as possible with mkDerivation. A piece of the puzzle is to expose the currently (badly) encapsulated logic by refactoring mkDerivation into
  • a simple transformation of attributes (mkDerivation arguments to derivation arguments)
  • a mkDerivationOverridable that deals with adding the (legacy?) overrideAttrs and wrapping the outputs to resemble the expected package
  • a short and sweet mkDerivation that pulls it all together
• try to be future proof with RFC 92, and multi-derivation packages, both of which change the concept of a package attrset to be properly distinct from the derivations that build it
• not use the module system because the Nixpkgs graph is big enough that we care about performance penalties, even if they're only constant-factor
• incorporate module system ideas without the feature creep of wanting to do all the things we know we could do because of the module system
  • must have: a hierarchy of approximately two levels, corresponding to the attribute sets that exist at each "stage" of package construction. As an example, see e.g. __cleanAttrs: Clean packages that don't expose all internals #217243 (comment)

Note to self: put these two comments under a new issue to further discuss such a design, because this is starting to go off-topic, especially possible responses, and ping the Nixpkgs architecture team.

[pbsds]

We could maybe lift most cross logic out of stdenv.mkDerivation and rather compose it in using overrideAttrs.
EDIT: i re-read your second point, which basically states this

[pbsds]

But onto something more immediately actionable: are we good to merge this?

[roberth]

Yes, thank you for reminding me. Architecture improvements (like I suggested) should follow from things we find problematic in the code, not the opposite where we wait for beautiful elegant whatever before we solve our actual problems.