-
-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mkDerivation: add overrideAttrs function #18660
Conversation
@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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an awesome change. I've stumbled upon this several times before, and this also confuses newcomers (especially buildInputs
vs nativeBuildInputs
). I'm very much in favor of this (the only possible problem that I can think of may be memory consumption or evaluation performance -- we should probably measure it).
@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 |
I think the main performance impact of the old patch (and probably of your new one) was adding things to |
c9b6ead
to
50569c2
Compare
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 Also notice that there're very few |
This is similar to `overrideDerivation`, but overrides the arguments to `mkDerivation` instead of the underlying `derivation` call. Also update `makeOverridable` so that uses of `overrideAttrs` can be followed by `override` and `overrideDerivation`, i.e. they can be mix-and-matched.
50569c2
to
39b64b5
Compare
I wrote a quick benchmarking script. Here are some sample results; note that it varies between runs.
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) 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 |
Thanks @domenkozar! Can I request that this be cherry-picked to |
This is great; wanted for a long time! But I wish we could call this |
Also nice if this could use the |
I thought of Oh, I didn't recall |
I don't think silently changing what Also, what's the benefit of the |
@aneeshusa check out the agda infa, which does an adhoc |
@Ericson2314 I took a look and think you're talking about the code at the bottom of |
Well, 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 |
@aneeshusa sorry for being slow to respond. The simple answer is
|
Motivation for this change
This is similar to
overrideDerivation
, but overrides the arguments tomkDerivation
instead of the underlyingderivation
call.Also update
makeOverridable
so that uses ofoverrideAttrs
can befollowed by
override
andoverrideDerivation
, i.e. they can bemix-and-matched.
Extracted out of #17886, as this seems to have existing support (e.g. see #10721), so there is no need to keep it with the
cmakeFlags
changes as a motivator.Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)