lib.makeOverridable: observe <result>.__overriders#454997
lib.makeOverridable: observe <result>.__overriders#454997ShamrockLee wants to merge 3 commits intoNixOS:masterfrom
<result>.__overriders#454997Conversation
f13f741 to
8fa180d
Compare
<result>.__overriders
8fa180d to
8c3729e
Compare
8c3729e to
f109f51
Compare
|
I added a test case in |
f109f51 to
47424c0
Compare
emilazy
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
FWIW, I think this shouldn’t block #445668 because llvmPackages.overrideScope didn’t previously exist – so its result not having .override is not a critical issue, just an unfortunate one. My review comments there were only about the need to ensure the Darwin stdenv’s use of it don’t clobber it, and to ensure that we can provide an .override that actually works there, but it seems that can be achieved by hand for now.
However, solving this problem in general is of course important. I hope you don’t mind me wanting to take some time to think about this approach, as this area of things is relevant to the by-name work I’m doing with @wolfgangwalther, and I worry a little about adding this to 25.11 and then potentially wanting to break it shortly after.
(To be clear, though, I don’t want to punt on this indefinitely, and although #273815 has been relevant to my thinking, I would not want to couple the kinds of solutions I’m thinking about to a meaningful breaking change in the stdenv.mkDerivation interface – I am hoping we can achieve something relatively general that preserves good backwards compatibility with our existing APIs, but I don’t object to smaller changes that work well.)
| genAttrs (filter ( | ||
| x: x != "override" && x != "overrideAttrs" && x != "overrideDerivation" && result ? ${x} | ||
| ) result.__overriders) (name: fdrv: overrideResult (x: x.${name} fdrv)) |
There was a problem hiding this comment.
An interesting option here would be to explicitly allow override here, which could potentially be used to allow migrating foo = bar.override { … }; definitions into by-name without breaking override interfaces. However, there are other complications there, with regards to the mirrorArgs stuff. (I got something to allow composing override interfaces working locally, but wasn’t entirely happy with the trade-offs.)
There was a problem hiding this comment.
An interesting option here would be to explicitly allow
overridehere
Could you please elaborate?
We cannot implement the override attribute with overrideResult in lib.makeOverridable. It will always get overwritten.
As for overrideAttrs, we could do it here instead of defining explicitly above, but I'm worried about the potential performance implication.
There was a problem hiding this comment.
I can’t directly recall what I was thinking at the time, but I believe that what I meant was that we could allow packages to expose their own .override field rather than always overriding it, which lets you callPackage/.override some other package in by-name without having the usual callPackage override interface clobber the one you’d like to inherit. (Hence why we have some overrides directly in all-packages.nix still.) Though in further thinking about this whole class of problem I decided it was more complicated than that, because sometimes you don’t want to clobber the callPackage override interface in that way.
Thank you for explaining and clarifying it! The discussion thread is quite long under that PR, and I only have a vague impression about it.
This change affects the whole |
47424c0 to
cbdd103
Compare
This PR solves
lib.makeOverridable's not handling existing overriders besideoverrideAttrsby introducing the__overridersattribute.Such attribute contains the names of existing overriders of the attribute set.
When a
lib.makeOverridable-like function adds a new overrider, it can checkresult.__overriders, decorates existing overriders, and add the name of the overriders it provide to the__overriderslist.This is an extensible solution/workaround to issues like
<set>.overridelost after<set>.overrideScopeoverriding (#447012) without substantial infrastructural changes (like what #273815 proposes).This PR helps
unblockimprove PR #445668. Cc: @LunNova @bb010gThings done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.