lib: make extensible by infinisil · Pull Request #38522 · NixOS/nixpkgs

infinisil · 2018-04-06T17:22:05Z

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 = ...; }

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 a lib option. Stuff like with 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.

infinisil · 2018-04-06T17:23:55Z

lib/modules.nix

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.

Profpatsch

I’m not sure this is the correct way to implement overriding lib.
@edolstra, @nbp and maybe @shlevy and @aszlig should take a look.

Profpatsch · 2018-04-06T20:26:04Z

lib/default.nix

I’d keep the symmetry with other calls in this module.

Profpatsch · 2018-04-06T20:29:13Z

lib/default.nix

The indentation is broken from here.

How would you format it?

Don't use rec. Use with self; instead.

infinisil · 2018-04-07T14:38:51Z

Changed it to with self;

Ericson2314

Fix that and @Profpatsch's comment on symmetry, and this looks good to me. Thanks!

Sorry to not catch that before.

Ericson2314 · 2018-04-07T15:54:20Z

lib/default.nix

Do inherit (import ./fixed-points.nix {}) makeExtensible!.

Ah yeah that sounds better

Ericson2314 · 2018-04-07T15:57:19Z

@Profpatsch with our changes this does look indeed look correct to me.

infinisil · 2018-04-07T16:03:18Z

Updated

Ericson2314

Ah forgot to remove old inherit.

Ericson2314 · 2018-04-07T16:07:10Z

lib/default.nix

Ah this line still needs to go.

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 = ...; }

Ericson2314 · 2018-04-07T17:05:53Z

Great! I'll merge after of borg.

grahamc · 2018-04-07T18:05:31Z

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.

zimbatm · 2018-04-07T18:08:40Z

I was concerned about evaluation time. result: it doesn't affect time nix-env -qa >/dev/null (around 6s on my laptop).

infinisil · 2018-04-07T18:09:04Z

@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.

grahamc · 2018-04-07T18:13:24Z

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.

zimbatm · 2018-04-07T18:14:38Z

@infinisil why can't the overlay have a myoverlay.lib key with their own extensions?

infinisil · 2018-04-07T18:17:22Z

@grahamc What is the actual benefit of keeping it closed?
@zimbatm In case of modules, the overridden lib will then only be available in pkgs.lib, as opposed to the normal lib argument, which can give the same infinite recursion problems as mentioned initially with config.lib

shlevy · 2018-04-07T19:08:57Z

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 config.lib in NixOS to allow modules to give each other helper functions...

There is no real analogy with package set extensibility IMO.

shlevy · 2018-04-07T19:09:26Z

Re infinite recursion, we have mkIf etc. already there.

infinisil · 2018-04-07T19:18:52Z

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.

infinisil · 2018-04-07T19:25:47Z

An alternative, while not as elegant, is to add some functionality to the module system that allows you to set your own lib.

nbp

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.

rycee · 2018-04-08T20:06:12Z

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 :-)

infinisil · 2018-04-08T20:42:19Z

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.

Profpatsch · 2018-04-08T21:13:55Z

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.

Yes, I’d prefer that to making lib extensible.
Adding documentation how to do that might be the better solution here.

infinisil · 2018-04-08T21:40:12Z

I discussed other options for modifying the lib argument with @nbp on #nixos and the best thing we could come up with is to use evalModules specialArgs argument, see https://gist.github.com/Infinisil/aa97115ccec082ad192088b17b92f011#gistcomment-2427130. While specialArgs overriding normal arguments like config, options and lib doesn't really look like intended behaviour, it's what makes this work. While it's not as nice as the extensible lib, I'd almost be okay with using this instead. I really don't want all modules not in nixpkgs to have to use

{ lib, myLib, ... }:
with lib;
with myLib;

Profpatsch · 2018-04-08T22:53:37Z

I really don't want all modules not in nixpkgs to have to use

What’s so bad about that?

infinisil · 2018-04-08T23:07:36Z

@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

nbp · 2018-04-09T15:37:36Z

One last thing, using specialArgs or an extended lib (independently of how it is extended) will prevent using the home-manager as a sub-module, which I think is desirable for use cases where the home manager is used to configure some NixOS users.

Therefore, we should find a solution for the with problem described in #38522 (comment)

infinisil · 2018-04-15T17:27:52Z

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

👍 You like this change and don't want this to be reverted
👎 You dislike this change, nixpkgs is better off without it and it should be reverted.

oxij · 2018-04-18T00:32:47Z

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.

shlevy · 2018-04-18T01:45:36Z

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 lib, nor seen a compelling example of its usefulness.

oxij · 2018-04-18T02:12:32Z

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.

oxij · 2018-04-18T02:34:31Z

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.

infinisil · 2018-04-19T12:50:24Z

@oxij Interesting, very good point. Some excerpts from nixpkgs/lib:

nixpkgs/nixos/lib/build-vms.nix

Lines 1 to 6 in 12ce0db

    
           { system, minimal ? false, config ? {} }: 
        
           let pkgs = import ../.. { inherit system config; }; in 
        
           with pkgs.lib; 
        
           with import ../lib/qemu-flags.nix { inherit pkgs; };

nixpkgs/nixos/lib/eval-config.nix

Lines 25 to 27 in 12ce0db

    
           , prefix ? [] 
        
           , lib ? import ../../lib 
        
           }:

This is the whole file:

nixpkgs/nixos/lib/from-env.nix

Lines 1 to 4 in 12ce0db

    
           # TODO: remove this file. There is lib.maybeEnv now 
        
           name: default: 
        
           let value = builtins.getEnv name; in 
        
           if value == "" then default else value

nixpkgs/nixos/lib/utils.nix

Lines 1 to 3 in 12ce0db

    
           pkgs: with pkgs.lib; 
        
           rec {

I think all of these could be implemented by extending lib. I'm not 100% sure how to handle the pkgs argument though, since that's not really a thing in nixpkgs/lib.

nixos-discourse · 2023-05-15T01:03:30Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/completly-lost-with-errors-rror-attribute-extend-missing-at-nix-store-b7/28160/4

infinisil requested review from edolstra and nbp as code owners April 6, 2018 17:22

infinisil commented Apr 6, 2018

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Apr 6, 2018

Profpatsch reviewed Apr 6, 2018

View reviewed changes

infinisil force-pushed the extensible-lib branch from 4eee0ba to f8ce6b6 Compare April 7, 2018 14:38

Ericson2314 reviewed Apr 7, 2018

View reviewed changes

infinisil force-pushed the extensible-lib branch from f8ce6b6 to e95d8cc Compare April 7, 2018 16:02

Ericson2314 reviewed Apr 7, 2018

View reviewed changes

infinisil force-pushed the extensible-lib branch from e95d8cc to bbb2032 Compare April 7, 2018 16:09

Ericson2314 merged commit e1dee4e into NixOS:master Apr 7, 2018

nbp reviewed Apr 7, 2018

View reviewed changes

infinisil deleted the extensible-lib branch April 18, 2018 20:32

infinisil mentioned this pull request Sep 8, 2018

[RFC 0033] [WIP] Deprecation NixOS/rfcs#33

Closed

infinisil mentioned this pull request Dec 9, 2021

lib: remove the lib fix-point #149881

Closed

13 tasks

Uh oh!

Conversation

infinisil commented Apr 6, 2018

Motivation for this change

Things done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Profpatsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infinisil commented Apr 7, 2018

Uh oh!

Ericson2314 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Apr 7, 2018

Uh oh!

infinisil commented Apr 7, 2018

Uh oh!

Ericson2314 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Apr 7, 2018

Uh oh!

grahamc commented Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zimbatm commented Apr 7, 2018

Uh oh!

infinisil commented Apr 7, 2018

Uh oh!

grahamc commented Apr 7, 2018

Uh oh!

zimbatm commented Apr 7, 2018

Uh oh!

infinisil commented Apr 7, 2018

Uh oh!

shlevy commented Apr 7, 2018

Uh oh!

shlevy commented Apr 7, 2018

Uh oh!

infinisil commented Apr 7, 2018

Uh oh!

infinisil commented Apr 7, 2018

Uh oh!

nbp left a comment

Choose a reason for hiding this comment

Uh oh!

rycee commented Apr 8, 2018

Uh oh!

infinisil commented Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Profpatsch commented Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

infinisil commented Apr 8, 2018

Uh oh!

Profpatsch commented Apr 8, 2018

Uh oh!

infinisil commented Apr 8, 2018

Uh oh!

nbp commented Apr 9, 2018

grahamc commented Apr 7, 2018 •

edited

Loading

infinisil commented Apr 8, 2018 •

edited

Loading

Profpatsch commented Apr 8, 2018 •

edited

Loading