Skip to content
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

nixos: allow overriding lib #51109

Closed
wants to merge 1 commit into from
Closed

nixos: allow overriding lib #51109

wants to merge 1 commit into from

Conversation

4z3
Copy link
Contributor

@4z3 4z3 commented Nov 27, 2018

Motivation for this change

Allows overriding the lib argument used in configurations, which in turn allows using library functions while working with imports, like e.g.:

{ lib, ... }: {
  imports = lib.my-imports-generator;
}

The same wouldn't be possible with pkgs.lib, as it would cause an infinite recursion.

I'm not sure this is the best way to implement this feature.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Allows overriding the `lib` argument used in configurations, which in
turn allows using library functions while working with imports, like
e.g.:

```
{ lib, ... }: {
  imports = lib.my-imports-generator;
}
```

The same wouldn't be possible with `pkgs.lib`, as it would cause an
infinite recursion.
@@ -7,12 +16,18 @@ let
eval = import ./lib/eval-config.nix {
inherit system;
modules = [ configuration ];
specialArgs = {
inherit lib;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eval-config.nix takes a lib argument which you can use instead.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except that lib would then not propagate to the module arguments like you might expect. That argument is there as an optimization. In my lib overlays prototype I made that argument work as you expect, but this requires changes to lib/modules.nix, e.g. like this: { inherit config options; lib = args.lib or lib; } // specialArgs.

{ configuration ? import ./lib/from-env.nix "NIXOS_CONFIG" <nixos-config>
, lib ? if builtins.any looks-like-nixos-lib builtins.nixPath
then import <nixos-lib>
else import ../lib
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually, we can just use lib ? import <nixpkgs/lib> here. Then users can override it by setting

NIX_PATH=nixpkgs/lib=/path/to/lib:nixpkgs=/path/to/nixpkgs

This is then a single-line change pretty much accomplishing the same :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than setting lib like this, I think they would prefer to use the modular way (as mentioned in various comments), setting _module.args.lib. This requires some changes to lib/modules.nix to not make lib a special case any more, but I think this would be more in line with what we already have for e.g. pkgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware nixpkgs/lib can be overridden ❤️

@ottidmes
Copy link

ottidmes commented Dec 8, 2018

I was planning on writing a PR to extend lib myself when @infinisil mentioned this PR. I experimented with making overlays for lib as we have for nixpkgs, which works, but like you mentioned they would have to be defined outside of your configuration, because most modules tend to include lib inside the scope at the top of the file (i.e. with lib;), causing recursion, just like pkgs.lib would.

I was using <nixpkgs-lib-overlays> and ~/.config/nixpkgs/lib/overlays{,.nix}, but I was not particular happy with that approach. Seeing your approach made me realize that it indeed could just be simplified to just pointing to a custom lib. This would still leave open the option to implement that custom lib via overlays, but leave that option completely open to the implementer. I however do not like the coupling between NixOS and lib. For example, I also sometimes use just with import <nixpkgs/lib>;, which would then not have the custom lib. I like the suggestion made by @infinisil instead, to use <nixpkgs/lib> itself to point to the custom lib if you so want to, but for it to work with any existing direct import of nixpkgs/lib/default.nix, I would suggest we change that file to check for <nixpkgs/lib> and use that if it does not refer to itself.

I have some other suggestions, but I better place them at the source code directly.

@4z3
Copy link
Contributor Author

4z3 commented Dec 8, 2018

would suggest we change that file to check for <nixpkgs/lib> and use that if it does not refer to itself.

@ottidmes, I haven't tested the redirection, but I think d145173 implements what you're suggesting. That said, I'm not really sure we should redirect direct imports of nixpkgs/lib/default.nix. Can you elaborate on why we would want to do the redirection?

@4z3
Copy link
Contributor Author

4z3 commented Dec 8, 2018

One downside I'm seeing with overriding nixpkgs/lib is that it's not possible to extend <nixpkgs/lib> easily anymore. The earlier approach, i.e. using <nixos-lib>, allowed to extend <nixpkgs/lib> like this:

import <nixpkgs/lib> // {
  mystuff = "...";
}

Now this causes an infinite recursion, and the only solution I'm seeing is to reimplement <nixpkgs/lib/default.nix> it the custom lib.

EDIT
Without d145173, the above can be accomplished with following code, which would be good enough for me:

import <nixpkgs/nixos/../lib> // {
  mystuff = "...";
}

I'm going to drop d145173 from this PR unless there's a reason to keep the redirection in <nixpkgs/lib/default.nix>.

@ottidmes
Copy link

ottidmes commented Dec 8, 2018

The idea behind the redirection was that there are still many places in nixpkgs that references nixpkgs/lib directly via path imports (search nixpkgs with this regex: \./lib[^0-9a-zA-Z/\.\-|"]), and outside nixpkgs there might be people having such references too, but its also fair to say that in those cases you probably do not want to use the custom lib anyway. Its just if you want the custom lib be the default everywhere, redirection is the only sure way I could think of.

@4z3
Copy link
Contributor Author

4z3 commented Dec 8, 2018

Now, after trying to actually use an overridden nixpkgs/lib in my setup, I have to say that I'm not happy with it anymore for following reasons:

  1. the above mentioned import <nixpkgs/nixos/../lib> doesn't look particularly good to me, and I wouldn't like to promote such hacks as the conventional way to do things
  2. and more importantly, it's not possible to use just NIX_PATH=/somepath anymore, because nixpkgs/lib has to be overridden explicitly.

Therefore I'm reverting this PR to the original state again. I'm not sure the implementation is the best possible, but having a separate path like <nixos-lib> feels cleaner to me.

@ottidmes
Copy link

ottidmes commented Dec 9, 2018

I get the issues you have with nixpkgs/lib as a prefix, like (1) you mentioned, but I do not quite understand the issue described in (2), could you elaborate on that? It is already impossible to have just NIX_PATH=/path/to/nixpkgs, it needs to be at least NIX_PATH=nixpkgs=/path/to/nixpkgs and probably even NIX_PATH=nixpkgs=/path/to/nixpkgs:/path/to/nixpkgs. And how would using nixos-lib as a prefix address the issue in (2)?

@@ -1,4 +1,13 @@
let
looks-like-nixos-lib = { path, prefix }:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

camelCase please.

@edolstra
Copy link
Member

edolstra commented Dec 9, 2018

Not really in favor of adding another ad-hoc override mechanism. Also, in the future, the Nix search path is likely to be deprecated in favor of flakes, where the Nixpkgs library may well be factored out into a separate flake, which would allow it to be overriden easily.

@ottidmes ottidmes mentioned this pull request Dec 9, 2018
10 tasks
@ottidmes
Copy link

ottidmes commented Dec 9, 2018

I will eagerly await more news about these flakes! In the meantime I made a different proposal (#51797) on how to extend lib, which while still ad hoc, tries to address the problems I had with this proposal.

@4z3
Copy link
Contributor Author

4z3 commented Dec 10, 2018

Looking forward to seeing flakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants