lib.makeOverridable: Propagate function arguments #67809

infinisil · 2019-08-31T04:39:31Z

Motivation for this change

Propagate the arguments of f to (makeOverridable f args).override

This allows people to query what arguments they can override for
derivations without having to look at the nix expression with

nix-repl> lib.functionArgs pkgs.hello.override
{ fetchurl = false; stdenv = false; }

If f args is a function, propagates its arguments to
makeOverridable f args

This allows querying function arguments even after it's been
makeOverridable'd. E.g.

nix-repl> lib.functionArgs pkgs.fetchgit
{ branchName = true; deepClone = true; fetchSubmodules = true; ... }

This is a continuation of #47535, this time with correct behavior even when .overrideDerivation or .overrideAttrs is used.

I took the liberty to remove the overrideDerivation = throw "overrideDerivation not yet supported for functors"; while I'm at it.

Ping @nbp @Profpatsch

Things done

Double-checked to make sure the code doesn't change any behavior

infinisil · 2019-08-31T04:55:05Z

pkgs/applications/graphics/gimp/wrapper.nix

@@ -1,7 +1,7 @@
 { stdenv, lib, symlinkJoin, gimp, makeWrapper, gimpPlugins, gnome3, plugins ? null}:

 let
-allPlugins = lib.filter (pkg: builtins.isAttrs pkg && pkg.type == "derivation" && !pkg.meta.broken or false) (lib.attrValues gimpPlugins);
+allPlugins = lib.filter (pkg: lib.isDerivation pkg && !pkg.meta.broken or false) (lib.attrValues gimpPlugins);


This change is necessary because gimpPlugins was makeOverride'd, meaning it has a .override, which previously was a function, but now is a functor attrset (but without a .type), which this function wasn't able to handle before. The lib.isDerivation function properly checks for that.

FRidh · 2019-08-31T13:03:02Z

Interesting change. Does this impact memory usage?

grahamc · 2019-08-31T13:23:45Z

@GrahamcOfBorg eval

it should have posted the performance report ...

infinisil · 2019-08-31T14:44:57Z

Hm that doesn't look very good, why would eval time go up this much? I call bad luck, let's try again

@GrahamcOfBorg eval

This was the result for reference:

stat	before	after	Δ	Δ%
cpuTime	189.76	225.71	↗ 35.95	18.94%
envs-bytes	3,013,206,344	3,022,604,696	↗ 9,398,352	0.31%
envs-elements	162,003,077	162,976,653	↗ 973,576	0.60%
envs-number	107,323,858	107,424,467	↗ 100,609	0.09%
gc-heapSize	11,828,391,936	11,878,723,584	↗ 50,331,648	0.43%
gc-totalBytes	23,316,522,048	23,352,482,976	↗ 35,960,928	0.15%
list-bytes	1,591,287,664	1,591,287,664	0
list-concats	7,288,187	7,288,187	0
list-elements	198,910,958	198,910,958	0
nrAvoided	151,146,250	151,485,068	↗ 338,818	0.22%
nrFunctionCalls	93,631,170	93,731,779	↗ 100,609	0.11%
nrLookups	75,657,344	75,682,341	↗ 24,997	0.03%
nrOpUpdateValuesCopied	230,487,746	230,496,380	↗ 8,634	0.00%
nrOpUpdates	12,981,495	12,984,373	↗ 2,878	0.02%
nrPrimOpCalls	66,132,513	66,132,513	0
nrThunks	155,344,587	155,998,851	↗ 654,264	0.42%
sets-bytes	7,771,229,080	7,772,720,896	↗ 1,491,816	0.02%
sets-elements	314,516,021	314,568,889	↗ 52,868	0.02%
sets-number	27,855,572	27,883,445	↗ 27,873	0.10%
sizes-Attr	24	24	0
sizes-Bindings	8	8	0
sizes-Env	16	16	0
sizes-Value	24	24	0
symbols-bytes	15,190,949	15,190,922	↘ 27	-0.00%
symbols-number	334,133	334,134	↗ 1	0.00%
values-bytes	5,461,096,464	5,477,392,272	↗ 16,295,808	0.30%
values-number	227,545,686	228,224,678	↗ 678,992	0.30%

infinisil · 2019-08-31T15:07:55Z

Yeah looking much better now (only 3.5% slower), not sure how much I can trust the eval time now.

Profpatsch · 2019-09-03T17:55:34Z

Can you split this PR into two/three/n commits, the implementation changes first and then any renaming/refactoring afterwards?

This is just a block of stuff and it’s very hard to see what is renaming, what is actual changes to the logic and what is reformatting/reflowing of the code.

And call it overrideArgs in the let binding because that's what it does

This allows querying the arguments you can .override: nix-repl> lib.functionArgs pkgs.hello.override { fetchurl = false; stdenv = false; }

…ction This allows querying function arguments of things like fetchFromGitHub: nix-repl> lib.functionArgs pkgs.fetchFromGitHub { fetchSubmodules = true; githubBase = true; ... }

…tions - Apparently nobody ever needed this - We already have enough ways to override things - Using overrideDerivation is discouraged

- Rename ff to result because that's what it is - Better indentation - Less parens - Comment what overrideWith does

infinisil · 2019-09-04T23:02:26Z

@Profpatsch Yeah good point, did that, now best reviewable by commit

infinisil · 2019-09-04T23:05:39Z

@jonringer Got any concerns about this change?

jonringer · 2019-09-04T23:57:34Z

No concerns code-wise. The inner-workings of nix looks like voodoo to me :)

For commit hygience, maybe squash a few of the minor ones together. Otherwise LGTM

infinisil · 2019-09-05T00:19:04Z

@jonringer (asking because you put a 😕 on the PR)

jonringer · 2019-09-05T00:31:39Z

oh, I think i meant to comment about how I couldnt make heads or tails of what magic you were doing. Sorry for the confusion :)

infinisil · 2019-09-05T13:37:51Z

Hehe, this time in the performance report:

stat	before	after	Δ	Δ%
cpuTime	192.73	192.73	↗ 0.00	0.00%

Profpatsch

I found a few things.

lib/customisation.nix

Profpatsch · 2019-09-21T21:36:34Z

lib/customisation.nix

+          override = overrideArgs;
+          overrideDerivation = fdrv: overrideResult (x: overrideDerivation x fdrv);
+          ${if result ? overrideAttrs then "overrideAttrs" else null} = fdrv:
+            overrideResult (x: x.overrideAttrs fdrv);


I’d like to see a deprecation of overrideDerivation, by throwing a warning and removing it next release (or the one after).

The attribute has been throwing an error already forever, so removing it is already the next step

lib/customisation.nix

infinisil · 2019-09-30T14:42:14Z

@Profpatsch Can you take another look?

Profpatsch

Sorry for the wait, was on vacation for a week.

lib/trivial.nix

infinisil · 2019-10-03T12:01:32Z

@GrahamcOfBorg eval

Not sure why it failed evaluation, seems unrelated

lib.makeOverridable: Propagate function arguments (cherry picked from commit 1230fc8)

infinisil requested review from edolstra and nbp as code owners August 31, 2019 04:39

gimp: Fix derivation detection

a41af6a

infinisil force-pushed the propagate-override-args branch from 699feab to 68d0fed Compare August 31, 2019 04:52

infinisil commented Aug 31, 2019

View reviewed changes

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Aug 31, 2019

infinisil added 6 commits September 5, 2019 00:54

lib/makeOverridable: Deduplicate override definition

a75080f

And call it overrideArgs in the let binding because that's what it does

lib/makeOverridable: Abstract result overriding

e140d70

lib/makeOverridable: Propagate function arguments to override functions

d54bdf5

This allows querying the arguments you can .override: nix-repl> lib.functionArgs pkgs.hello.override { fetchurl = false; stdenv = false; }

lib/makeOverridable: Propagate function args of the callPackage'd fun…

c638dac

…ction This allows querying function arguments of things like fetchFromGitHub: nix-repl> lib.functionArgs pkgs.fetchFromGitHub { fetchSubmodules = true; githubBase = true; ... }

lib/makeOverridable: Remove unimplemented overrideDerivation for func…

23e72ef

…tions - Apparently nobody ever needed this - We already have enough ways to override things - Using overrideDerivation is discouraged

lib/makeOverridable: Refactor

a4896cb

- Rename ff to result because that's what it is - Better indentation - Less parens - Comment what overrideWith does

infinisil force-pushed the propagate-override-args branch from 68d0fed to a4896cb Compare September 4, 2019 23:01

Profpatsch requested changes Sep 21, 2019

View reviewed changes

Profpatsch requested changes Oct 2, 2019

View reviewed changes

lib/trivial.nix Outdated Show resolved Hide resolved

infinisil force-pushed the propagate-override-args branch from 999a5ab to a4896cb Compare October 2, 2019 21:02

Profpatsch approved these changes Oct 3, 2019

View reviewed changes

infinisil merged commit 1230fc8 into NixOS:master Oct 22, 2019

infinisil deleted the propagate-override-args branch October 22, 2019 12:37

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Oct 22, 2019

Merge pull request NixOS#67809 from Infinisil/propagate-override-args

9c12e6b

lib.makeOverridable: Propagate function arguments (cherry picked from commit 1230fc8)

ShamrockLee mentioned this pull request Dec 14, 2023

lib.extendMkDerivation: init #234651

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib.makeOverridable: Propagate function arguments #67809

lib.makeOverridable: Propagate function arguments #67809

infinisil commented Aug 31, 2019

infinisil Aug 31, 2019

FRidh commented Aug 31, 2019

grahamc commented Aug 31, 2019

infinisil commented Aug 31, 2019 •

edited

Loading

infinisil commented Aug 31, 2019

Profpatsch commented Sep 3, 2019

infinisil commented Sep 4, 2019

infinisil commented Sep 4, 2019

jonringer commented Sep 4, 2019

infinisil commented Sep 5, 2019

jonringer commented Sep 5, 2019

infinisil commented Sep 5, 2019

Profpatsch left a comment

Profpatsch Sep 21, 2019

infinisil Sep 22, 2019

infinisil commented Sep 30, 2019

Profpatsch left a comment

infinisil commented Oct 3, 2019

lib.makeOverridable: Propagate function arguments #67809

lib.makeOverridable: Propagate function arguments #67809

Conversation

infinisil commented Aug 31, 2019

Motivation for this change

Things done

infinisil Aug 31, 2019

Choose a reason for hiding this comment

FRidh commented Aug 31, 2019

grahamc commented Aug 31, 2019

infinisil commented Aug 31, 2019 • edited Loading

infinisil commented Aug 31, 2019

Profpatsch commented Sep 3, 2019

infinisil commented Sep 4, 2019

infinisil commented Sep 4, 2019

jonringer commented Sep 4, 2019

infinisil commented Sep 5, 2019

jonringer commented Sep 5, 2019

infinisil commented Sep 5, 2019

Profpatsch left a comment

Choose a reason for hiding this comment

Profpatsch Sep 21, 2019

Choose a reason for hiding this comment

infinisil Sep 22, 2019

Choose a reason for hiding this comment

infinisil commented Sep 30, 2019

Profpatsch left a comment

Choose a reason for hiding this comment

infinisil commented Oct 3, 2019

infinisil commented Aug 31, 2019 •

edited

Loading