stdenv.mkDerivation: overlay style overridable recursive attributes

[roberth]

Motivation for this change

• Allows to get rid of confusing rec { } in mkDerivation calls. Overriding an attribute in this style causes derived attributes to be overridden as expected.

• Allows passthru attributes to reference the outputs

• Fixes passthru.tests is wrong after overrides #119407, the problem where if you use overrideAttrs, the tests in passthru still use the old package.

• Should allow cleanup of python and rust packaging functions by using "overlay" layers on a prototype derivation, rather than the current workarounds.

See updated manual for usage explanation.

Update: this problem was also brought up in this discussion: #119731 (review)

Update: "overlay" part minus public was also discovered and implemented by lilyball #94198

Things done

• Tested using sandboxing not applicable: changes are evaluation-only (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[davidak]

I see how this can be useful in the case of using overrideAttrs, but i don't like that we would have to add this overhead to every package.

It is a common use case to use the current package in passthru.tests, so could we achieve this with a more elegant implementation? I don't have to manually specify something to be able to use $out in buildPhase, why do i have to for passthru.tests?

When thinking this further, what if i override a package and also test another package that depend on it in it's passthru.tests?

[roberth]

what if i override a package and also test another package that depend on it in it's passthru.tests?

If you don't use this in a tests attribute, you're not testing the final package. In such a case you should override that package to use this. You could use pkgs.extend for this purpose.

I don't have to manually specify something to be able to use $out in buildPhase, why do i have to for passthru.tests?

$out is a shell variable that only exists inside the derivation. When you write a test in a separate derivation, its $out will not equal the $out of the original derivation, so you'll need to use the Nix expression language to be able to reference it. Variables in the Nix expression are mostly lexically scoped and immutable, unlike bash's global-by-default mutable scope. The only exception is the with keyword, which is only a fallback for variable references that would be unresolvable otherwise.

[Ericson2314]

Also, I would check out the saga of #27319 PR, which had similar goals long before, but ended up being reverted for perf reasons (IIRC).

[roberth]

I've updated the docs. It now includes this example which I think is particularly nice:

    passthru.appendPackages = packages':
      self.overrideAttrs (newSelf: super: {
        packages = super.packages ++ packages';
      });

Also, I would check out the saga of #27319 PR, which had similar goals long before, but ended up being reverted for perf reasons (IIRC).

This is quite different and I've checked performance with bench aka criterion on an idle machine.

bench -L 1800 'nix-instantiate -A hello -A pandoc -A cassandra -A rabbitmq-server'

master:

benchmarking nix-instantiate -A hello -A pandoc -A cassandra -A rabbitmq-server
time                 2.206 s    (2.205 s .. 2.207 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 2.205 s    (2.204 s .. 2.206 s)
std dev              2.875 ms   (1.878 ms .. 4.684 ms)

overridable:

benchmarking nix-instantiate -A hello -A pandoc -A cassandra -A rabbitmq-server
time                 2.216 s    (2.215 s .. 2.217 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 2.216 s    (2.215 s .. 2.216 s)
std dev              1.822 ms   (1.462 ms .. 2.350 ms)

So the slowdown is only 0.45%, which seems like a fair price to pay for correctness.

Memory usage has increased slightly as well.

--- master.stats        2021-05-02 23:46:59.409895694 +0200
+++ overridable.stats   2021-05-02 23:47:12.699873034 +0200
@@ -1,27 +1,27 @@
 {
-  "cpuTime": 1.72167,
+  "cpuTime": 1.70415, # noisy; ignore; see proper stats above
   "envs": {
-    "number": 565107,
-    "elements": 1036066,
-    "bytes": 17330240
+    "number": 587547,
+    "elements": 1063062,
+    "bytes": 17905248
   },
   "list": {
-    "elements": 364943,
-    "bytes": 2919544,
+    "elements": 373857,
+    "bytes": 2990856,
     "concats": 53359
   },
   "values": {
-    "number": 2043552,
-    "bytes": 49045248
+    "number": 2058343,
+    "bytes": 49400232
   },
   "symbols": {
-    "number": 92751,
-    "bytes": 2812685
+    "number": 92755,
+    "bytes": 2812733
   },
   "sets": {
-    "number": 189351,
-    "bytes": 175026936,
-    "elements": 7229672
+    "number": 198270,
+    "bytes": 177275760,
+    "elements": 7320400
   },
   "sizes": {
     "Env": 16,
@@ -29,15 +29,15 @@
     "Bindings": 8,
     "Attr": 24
   },
-  "nrOpUpdates": 105296,
-  "nrOpUpdateValuesCopied": 6123991,
-  "nrThunks": 1545207,
-  "nrAvoided": 963446,
-  "nrLookups": 596064,
-  "nrPrimOpCalls": 382588,
-  "nrFunctionCalls": 501801,
+  "nrOpUpdates": 109755,
+  "nrOpUpdateValuesCopied": 6205698,
+  "nrThunks": 1559996,
+  "nrAvoided": 989027,
+  "nrLookups": 601181,
+  "nrPrimOpCalls": 387046,
+  "nrFunctionCalls": 517808,
   "gc": {
     "heapSize": 402915328,
-    "totalBytes": 287925024
+    "totalBytes": 292058624
   }
 }