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

Make lib more unified and overridable #157056

Closed
wants to merge 5 commits into from
Closed

Conversation

ncfavier
Copy link
Member

@ncfavier ncfavier commented Jan 27, 2022

A broad attempt at fixing #156312-like issues. With this, the following flake works as expected:

{
  outputs = { nixpkgs, ... }: let
    lib = nixpkgs.lib.extend (final: prev: {
      licenses = prev.licenses // {
        sspl = prev.licenses.sspl // {
          shortName = "sspl";
        };
      };
    });
  in {
    nixosConfigurations.foo = lib.nixosSystem {
      system = "x86_64-linux";
      modules = [
        ./configuration.nix
        ({ lib, ... }: {
          nixpkgs.config.blocklistedLicenses = [ lib.licenses.sspl ];
        })
      ];
    };
  };
}

See each commit's description for details.

This will probably need release notes.

The type lazyAttrsOf unspecified for nixpkgs.lib is the most precise I could make it. With attrsOf we get warnings from trying to evaluate deprecated attributes, and with anything instead of unspecified there's a weird infinite recursion.

@Ericson2314
Copy link
Member

I don't think the lib should be overridable. The point of overriding is to be able to modify dependencies, but I don't know why anyone would be want do do that with the lib. If you want to merely add new stuff, you don't need overrides.

@ony
Copy link
Contributor

ony commented Jan 28, 2022

@Ericson2314, my real life case is #154031 related to blocklistedLicenses requiring all licenses to be from lib.licenses.

@ncfavier
Copy link
Member Author

The first step of the reasoning is that if you have a bunch of functions you want to use in your NixOS modules, extending the lib with those functions seems more natural (to me at least) than putting them in a separate myLib and having to remember which one to use (much like you add packages to pkgs using overlays instead of making your own myPkgs). The last commit allows this scenario to work without having to pass your modified lib to nixosSystem if you're already calling lib.nixosSystem.

But then we're left with an awkward situation where pkgs.lib != lib, and fixing this situation gives you the ability to implement quick workarounds such as the one illustrated with lib.licenses.sspl, just like you can quickly override a broken package without having to use a modified nixpkgs.

@Ericson2314
Copy link
Member

I think passing more parameters is good. People not liking passing individual parameters skirts the danger of state/entanglement. I still think the fundamental difference between overlays and this is and overlays are for overriding dependencies, and this is not.

Put another way, if we only extended package sets with downstream packages, and never modified upstream ones, I would not approve of overlays.

@ncfavier
Copy link
Member Author

ncfavier commented Feb 17, 2022

I see your point. I don't think that's the only thing overlays are for, though: they allow fixing broken packages (or in this case broken lib attributes), and they make everything available under a single namespace.

For nixpkgs, only the first thing applies since nixpkgs expressions won't be using our extensions, so I understand if you don't want to add a lib parameter just to be able to fix a broken lib.

For NixOS modules though, the second thing also applies, and I think this justifies the last commit, so I will split it into a separate PR (along with the first one which is orthogonal). EDIT: #160459

@infinisil
Copy link
Member

The type lazyAttrsOf unspecified for nixpkgs.lib is the most precise I could make it. With attrsOf we get warnings from trying to evaluate deprecated attributes, and with anything instead of unspecified there's a weird infinite recursion.

I believe with #160489, types.raw would be an appropriate type, or types.lazyAttrsOf types.raw if you want to make sure it's at least an attribute set

lib/fixed-points.nix Outdated Show resolved Hide resolved
@roberth
Copy link
Member

roberth commented Feb 17, 2022

The type lazyAttrsOf unspecified for nixpkgs.lib is the most precise I could make it. With attrsOf we get warnings from trying to evaluate deprecated attributes, and with anything instead of unspecified there's a weird infinite recursion.

I believe with #160489, types.raw would be an appropriate type, or types.lazyAttrsOf types.raw if you want to make sure it's at least an attribute set

It should be unique, to avoid combining methods of composition that do not interact well.

unique
  { message = "`lib` is not suitable for plain attrset merging, as it does not interact well with the `extend` function. Any fixes to `lib` should be applied by invoking `extend` on the original `lib` value."; }
  (lazyAttrsOf raw)

@roberth roberth mentioned this pull request Feb 17, 2022
2 tasks
@infinisil
Copy link
Member

Or alternatively have the type of lib be types.raw directly. That type also only allows a single definition (but you can override it with mkForce). Then it won't be checked to be an attribute set, but that's not terrible imo

@roberth
Copy link
Member

roberth commented Feb 17, 2022

Or alternatively have the type of lib be types.raw directly. That type also only allows a single definition (but you can override it with mkForce). Then it won't be checked to be an attribute set, but that's not terrible imo

It won't produce an explanation like unique does and it doesn't complain when it's accidentally a function (forgotten arg) or a path (forgotten import) etc. #157056 (comment) seems better.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Looking good to me!

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

types...raw is available now, so the type can be improved.

nixos/modules/misc/nixpkgs.nix Outdated Show resolved Hide resolved
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

I agree with @edolstra and @Ericson2314's intent. In a perfect world, their argument would block this change, but in our messy one, we have to deal with stuff like incompatibilities between nixpkgs versions and potentially other use cases. lib.extend already exists and continues to exist for such reasons. Just that it shouldn't be the preferred choice, doesn't mean that it should be crippled.

@edolstra
Copy link
Member

edolstra commented Mar 3, 2022

I think extending the library is just a weird idea in general. It's confusing and bad for UX, because it means that you can't count on a function lib.foo to actually be part of the standard library. Indeed, you can't really have any expectations about lib, since any overlay can make arbitrary changes to it. By contrast, if I'm hacking on a Rust project, I can count on a symbol std::foo to be something from the standard library and to be documented in the corresponding Rust crate.

What exactly is the problem with putting one's own functions in my-lib or whatever, instead of globally injecting it into lib?

@roberth
Copy link
Member

roberth commented Mar 3, 2022

What exactly is the problem with putting one's own functions in my-lib or whatever, instead of globally injecting it into lib?

There rarely is, unless you're polyfilling missing functionality in the stable release for example. Like the "incompatibilities" link in my previous comment.

@ncfavier
Copy link
Member Author

ncfavier commented Mar 3, 2022

It's confusing and bad for UX

It's supposed to be an end user thing!

What exactly is the problem with putting one's own functions in my-lib or whatever, instead of globally injecting it into lib?

{ lib, my-lib, ... }: with lib; with my-lib; ... is just awful

@edolstra
Copy link
Member

It's supposed to be an end user thing!

The "end user" here can also be a colleague who think it's a good idea to extend lib in their organisation's NixOS modules, leaving others scratching their heads where lib.<some-name> comes from.

{ lib, my-lib, ... }: with lib; with my-lib; ... is just awful

Why? That's what basically every other language does: if you want something from my-lib, you have to import my-lib.

@ncfavier
Copy link
Member Author

ncfavier commented Mar 11, 2022

I think I would agree with you if it was named nixpkgs-lib or somesuch. The fact that it has such a generic name tells me that it's a generic bag of functions to be extended at will, not something specific to nixpkgs.

I can see the problem with figuring out where things are coming from, but I think this is outweighed by the benefit of having a relatively unified boilerplate for modules, plus the practical uses @roberth and I mentioned.

For a programming language analogy, think of Haskell, where the dependency plumbing happens in the Cabal file (i.e. flake.nix) and modules can simply import other modules without worrying about what package they come from (i.e. { lib }: lib.my.module.foobar).

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 21, 2022
@RaitoBezarius
Copy link
Member

I am very much in favor of this, @edolstra @Ericson2314, do you oppose the merging of this PR?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 1, 2022
This is necessary for `lib` to still be extensible in the modules
evaluated by `evalModules`.
Also inline the call to `fix'` for performance.
Makes it easy to override the lib used across pkgs, for example to stay
in sync with the lib used in NixOS modules.
Unify the lib used in stdenv/generic with the one passed to pkgs/top-level
@infinisil
Copy link
Member

We talked about this on the NAT Matrix channel today, see here

Allows overriding the lib passed to the default nixpkgs invocation.
Defaults to the `lib` argument to modules.
Makes lib extensions available in modules by default
@RossComputerGuy
Copy link
Member

This is a PR I really want to see go through. I have a lot of stuff where I override things in nixpkgs and this PR will make things easier.

@RossComputerGuy
Copy link
Member

Rebase is needed.

@bew
Copy link
Contributor

bew commented Nov 5, 2023

Why close? 🤔

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.