@aneeshusa, thanks for your PR! By analyzing the annotation information on this pull request, we identified @urkud, @oxij and @edolstra to be potential reviewers

Looks useful!

@abbradar I'm happy to run some benchmarks, but would appreciate some more concrete instructions on how to do so.

I have completely forgot about this, sorry. I've found an old issue about this with associated benchmarks: #10721 (comment) Let's see if things are better now...

@abbradar This PR just adds a new function but doesn't use it anywhere yet, (unlike the old PR which changes overrideDerivation), so those benchmarks will just report the same thing both times. A blind search/replace doesn't seem like a good idea as the new overrideAttrs has slightly different semantics and will act differently in some cases, so I think using a few selected test cases would be best. Can you help me pick a few (important) packages that currently use overrideDerivation that I can bench after switching to overrideAttrs?

I think the main performance impact of the old patch (and probably of your new one) was adding things to makeOverridable, therefore effectively growing closure of every function wrapped in it. I'm not sure however, so the first thing to do is probably run the same tests and see if there is any slowdown.

I have updated this with some documentation, please review and let me know how it can be improved.

@abbradar there is a trailing comma on your last comment, is part of it missing? I'm not sure what you're looking for in terms of benchmarking.

You are right, fixed; I was going to bed around this time and was sleepy -- sorry! Disclosure: personally I have urges to just merge this because IMO it's better to have a sane overrideDerivation even at a performance cost. However, others may not be so passionate about it, so we need numbers to present.

Also notice that there're very few overrideDerivation calls in nixpkgs and it's generally considered slow, so I don't think performance penalty of actual overrideAttrs calls need to be measured (I don't think it has much more impact that just a callPackage from what I see) -- rather we are interested only in added overhead for makeOverridable.

I wrote a quick benchmarking script. Here are some sample results; note that it varies between runs.

cowsay
                     before     after     delta     change
Elapsed time          0.109     0.094    -0.015  -13.7615%
Max RSS (Kbytes)    27663.6   27684.8     +21.2   +0.0766%

diffoscope
                     before     after     delta     change
Elapsed time           0.26      0.26      +0.0   +0.0000%
Max RSS (Kbytes)    51450.4   51534.0     +83.6   +0.1625%

firefox
                     before     after     delta     change
Elapsed time          0.288      0.27    -0.018   -6.2500%
Max RSS (Kbytes)    52466.8   52707.6    +240.8   +0.4590%

gnome3
                     before     after     delta     change
Elapsed time          0.596     0.595    -0.001   -0.1678%
Max RSS (Kbytes)    95241.6   95707.6    +466.0   +0.4893%

I don't think this patch will cause any significant performance problems.

Seems good to me; cc @edolstra

/cc @domenkozar

I think right after the 16.09 release is a good time to get this in since it's a replacement for (most uses of) overrideDerivation, allowing time before the next release to update references in the code and docs, etc.

I'm also looking for feedback on the documentation I added.

I'll review this shortly.

@domenkozar, just a kindly reminder :)

Clean PR with docs and simple implementation. Merging.

Nice idea! I also see very little use for overrideDerivation, now that we have this, as the derive primitive is hidden too deep in typical expressions.

Thanks @domenkozar! Can I request that this be cherry-picked to 16.09/stable?

This is great; wanted for a long time! But I wish we could call this overrideDerivation, and the old one overrideRawDerivation, or something like that.

Also nice if this could use the self: super: ... ideom.

I thought of overrideMkDerivation, especially if we also changed override to overrideCallPackage, but the names would be very long.

Oh, I didn't recall lib.extends. We still don't have a way for the two-argument style on most places. Perhaps we should introduce some new parallel naming for using this more powerful style.

I don't think silently changing what overrideDerivation does is a good idea, and I never liked that name anyways.

Also, what's the benefit of the self: super: idiom here? I've never felt the urge to use a self argument when doing overrides, usually just oldAttrs: rec { ... } works for me.

@aneeshusa check out the agda infa, which does an adhoc mkDerivation arguments self: super:

@Ericson2314 I took a look and think you're talking about the code at the bottom of pkgs/build-support/agda/default.nix. I can't yet understand what's going on or what the purpose of the extension arg is (which seems to be the raison d'être for the self/super distinction). I can't find docs in-tree, in the manual, in commit messages or PR comments, and I see only extension = self: super: {} being passed for that arg. Can you please provide some more explanation and/or concrete examples?

Well, self is supposed to refer to the final version, i.e. even after some other overrides were (potentially) applied.

It's not terribly useful, but it can come handy in some complex cases... out of the top of my head, say the package/override (conditionally) applies a patch that should be gone by version x and after that you want to additionally apply override of the version to a newer one – in that case it's better to use optional self.version than just optional version.

@aneeshusa sorry for being slow to respond. The simple answer is mkDerivation rec { .. } is fairly common, and now suspect. Each case probably ought to be probably be a mkDerivation (self: with self; { .. }), because one probably wants other arguments to be updated based on overrides.

rec { .. } in general is kind of a smell for this reason.