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
lib: make extensible #38522
lib: make extensible #38522
Conversation
@@ -59,7 +59,7 @@ rec { | |||
}; | |||
}; | |||
|
|||
closed = closeModules (modules ++ [ internalModule ]) ({ inherit config options; lib = import ./.; } // specialArgs); | |||
closed = closeModules (modules ++ [ internalModule ]) ({ inherit config options lib; } // specialArgs); |
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.
I'm not really sure why lib
was not taken from the fixed point before here, seems like a logical thing to do and is necessary for getting the module system benefits out of this change.
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.
lib/default.nix
Outdated
|
||
# often used, or depending on very little | ||
trivial = callLibs ./trivial.nix; | ||
fixedPoints = callLibs ./fixed-points.nix; | ||
inherit fixedPoints; |
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.
I’d keep the symmetry with other calls in this module.
lib/default.nix
Outdated
|
||
lib = fixedPoints.makeExtensible (self: let | ||
callLibs = file: import file { lib = self; }; | ||
in rec { |
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.
The indentation is broken from here.
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.
How would you format it?
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.
Don't use rec
. Use with self;
instead.
4eee0ba
to
f8ce6b6
Compare
Changed it to |
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.
Fix that and @Profpatsch's comment on symmetry, and this looks good to me. Thanks!
Sorry to not catch that before.
lib/default.nix
Outdated
@@ -5,13 +5,16 @@ | |||
*/ | |||
let | |||
|
|||
callLibs = file: import file { inherit lib; }; | |||
fixedPoints = import ./fixed-points.nix {}; |
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.
Do inherit (import ./fixed-points.nix {}) makeExtensible!
.
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.
Ah yeah that sounds better
@Profpatsch with our changes this does look indeed look correct to me. |
f8ce6b6
to
e95d8cc
Compare
Updated |
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.
Ah forgot to remove old inherit.
lib/default.nix
Outdated
|
||
# often used, or depending on very little | ||
trivial = callLibs ./trivial.nix; | ||
fixedPoints = callLibs ./fixed-points.nix; | ||
inherit fixedPoints; |
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.
Ah this line still needs to go.
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.
I excuse my sluggishness
This allows the lib fixed point to be extended with myLib = lib.extend (self: super: { foo = "foo"; }) With this it's possible to have the new modified lib attrset available to all modules when using evalModules myLib.evalModules { modules = [ ({ lib, ... }: { options.bar = lib.mkOption { default = lib.foo; }; }) ]; } => { config = { bar = "foo"; ... }; options = ...; }
e95d8cc
to
bbb2032
Compare
Great! I'll merge after of borg. |
I'm not sure lib should be so easy to extend, since it sort of obscures what is in lib, and possible collisions when new functions are added later on. Maybe not a big concern though? It is sort of like monkeypatching new functions in to core libs for other programming langs, a generally frowned upon thing. |
I was concerned about evaluation time. result: it doesn't affect |
@grahamc The same could be said for pkgs overlays though, but these are doing just fine with a much larger attribute count. Also this is no problem of nixpkgs, because extending lib is mostly useful outside of it. If you need to extend lib within nixpkgs you can just add the function you need in nixpkgs/lib as it's been done all this time. |
I guess I don't see the benefit of making it extensible, but I can see the benefits of keeping it closed -- preferring explicitness in what is contained in the nixpkgs lib. pkgs being an extensible fixed point isn't a good comparison, because the fixed point is required for dependency resolution and replacing packages -- a desirable set of features which I don't think translate in to eval-time library code. |
@infinisil why can't the overlay have a |
I'm with @grahamc here. I think we need concrete use cases to add more configuration knobs and increase complexity, and I just don't see why we need this. Note that we already have There is no real analogy with package set extensibility IMO. |
Re infinite recursion, we have |
Well we can undo this of course and discuss further, i didn't expect this to get merged so quickly anyways. I think it makes sense to be able to extend the base library like this. When we look at the default arguments to a module they are: config, options and lib. config and options are completely customizable on the module level. lib however is fixed in stone, there's no way to change it prior to this PR. And this change seems like the logical step to get this to be possible. Note that I'm mostly talking about non-nixpkgs uses of the module system. I stumbled upon this when I wanted to use the module system for my emacs config. |
An alternative, while not as elegant, is to add some functionality to the module system that allows you to set your own lib. |
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.
The changes made in lib/modules.nix
sounds fine to me.
However, I will note that unless you are extending the module system you should probably not do that.
Instead you can set the _module.args.hmLib
attribute to whatever new set of library function you want to be added. The reason of having a lib
attribute is only for getting the module system library functions without the pkgs
attribute.
Since this (apparently) has no negative impact on today's use in NixOS and Nixpkgs I'd say that its an interesting change that might be well worth experimenting with in non-Nixpkgs projects that use the NixOS module system. Of course, I'm personally thinking about Home Manager here. I'm also liking nbp's idea of adding a new attribute for the library set but would like the ability to test both variants. I should also add that I'm partial to this change also on purely esthetic grounds; the symmetry it adds is quite nice :-) |
Also, this change allows the module system itself to be modified/extended, which would've not been possible without forking nixpkgs before. And I think it makes sense to enable users of nixpkgs (Edit: I mean nixpkgs users that can't modify it directly) to do that. And as said by @rycee, I don't see any negative impact on nixpkgs/NixOS with this. |
Yes, I’d prefer that to making |
I discussed other options for modifying the { lib, myLib, ... }:
with lib;
with myLib; |
What’s so bad about that? |
@Profpatsch If I didn't know the internals of nixpkgs and saw that code I'd ask myself why there are two libs and why they couldn't be combined |
One last thing, using Therefore, we should find a solution for the |
Now it's been some time since this was (a bit prematurely) merged and while there were a lot of voices against it, I feel like nobody's gonna undo the change themselves. While I'd love to use this new functionality, I'm gonna hold back until I know this is gonna stay. To get an idea of amount of agreement I'm asking everybody here to vote on this comment with
|
My two cents.
At first I was somewhere between "meh, whatever" to 👎 on this. But after thinking about this for a while, now I feel like nixpkgs should follow GHC's approach of "it's always ok to remove a limitation that has no technical justification". Hence, now I'm completely 👍 on this until somebody can demonstrate technical, not stylistic, justifications against this.
I feel like if you want this to be reverted it's only fair to ask for overlays to be reverted too as they add a similar amount of complexity.
|
Again, there is no real comparison to overlays here. There have been use cases with a strong need to modify or add to the base package set for years before overlays, and they have been served very well since overlays were introduced. On the other hand, I've literally never felt the need to either override or extend |
I get your stylistic argument, overriding lib is uncommon. But what is the technical argument against this?
And, for the record, I do extend the `lib` (with stuff I should probably just PR, but let's assume it's very my-configs specific for the sake of the argument).
Can I just import a file whenever I use those things? Sure. Can I use module arguments? Sure.
But what if I want to use those functions in package expressions in my overlay?
Can I just import a file there? Well, sure, but what is the technical difference with overlays, again?
Personally, I don't even use overlays, I just dump everything into branches (because I prefer to fix merge conflicts as opposed to debugging why things unexpectedly silently broke with overlays), but if I did use them for my overrides I would see the same benefits with this PR.
|
Actually, I think I just came up with a use for this (or a bit modified version of this) directly in nixpkgs.
Why not make `nixos/lib/*` into such an extension of the `lib`? That would save a lot of imports in nixos sources. Some duplicated code would be easier to move there too if it were a lib (I wanted to implement `nixoslib` or something eventually, but something like this seems fine too). And you could still evaluate `nixpkgs` against the default `lib` as before with some minimal modifications.
|
@oxij Interesting, very good point. Some excerpts from nixpkgs/lib: nixpkgs/nixos/lib/build-vms.nix Lines 1 to 6 in 12ce0db
nixpkgs/nixos/lib/eval-config.nix Lines 25 to 27 in 12ce0db
This is the whole file: nixpkgs/nixos/lib/from-env.nix Lines 1 to 4 in 12ce0db
Lines 1 to 3 in 12ce0db
I think all of these could be implemented by extending lib. I'm not 100% sure how to handle the |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This allows the lib fixed point to be extended with
With this it's possible to have the new modified lib attrset available to all
modules when using evalModules
=>
Motivation for this change
This will be useful for projects like @rycee home-manager that use the module system themselves without the ability to modify
nixpkgs/lib
directly. Currently home-manager needs to use the prone to infinite recursion lib module that just adds alib
option. Stuff likewith config.lib;
at the top of the file will give you infinite recursion in general though.Another instance is @LnL7's nix-darwin which currently imports its lib directly in every file that needs it (which is what home-manager used previously).
@LnL7 @rycee @Profpatsch @edolstra @shlevy
Things done
Other than running a few very basic commands like the one above I haven't tested this very well. But I'm 99% sure this can't break anything and is fully backwards compatible. Will test it more in a bit.