gytis-ivaskevicius / flake-utils-plus

Use Nix flakes without any fluff.
MIT License
494 stars 54 forks source link

`overlayNames` errors; can't necessarily pass `null` to an overlay #98

Closed Radvendii closed 2 years ago

Radvendii commented 3 years ago

overlayNames calls the overlay attribute with null null for the arguments final and prev. This works if the overlay attribute looks like:

{
  overlay = final: prev: {
    blah = prev.callPackage ./blah {};
    foo = prev.callPackage ./foo {};
  };
}

because nix is lazy, so it will not even evaluate the attribute values.

However, if the overlay looks something like:

{
  overlay = final: prev:
    prev.lib.genAttrs 
      (builtins.readDir ./packages) 
      (pkgName: _: prev.callPackage ./packages + "/${pkgName}");
}

Then the overlay needs access to prev.lib before the set of names is known, and so overlayNames errors with value is null while a set was expected

For reference, this is where overlayNames is defined: https://github.com/gytis-ivaskevicius/flake-utils-plus/blob/master/lib/exportOverlays.nix#L63

Radvendii commented 3 years ago

That code also assumes overlays is a set. overlays is also allowed to be a list, right?

Pacman99 commented 3 years ago

Yeah the code very much relies on the lazy eval of overlays. I'm not really sure how to get around that for a function that needs to know the names of the packages the overlay creates.

That code also assumes overlays is a set. overlays is also allowed to be a list, right?

I don't think thats true for the overlays flake output.

gytis-ivaskevicius commented 3 years ago

I can't think of an actual solution to overlays evaluation issue, do you have anything in mind in terms of a fix? I doubt that it is even possible :/ But by looking at your provided example wouldn't something like this fix your particular issue?

    nixpkgs.lib.genAttrs # nixpkgs instead of prev
      (builtins.readDir ./packages) 
      (pkgName: _: prev.callPackage ./packages + "/${pkgName}");

Also as far as I know overlays must evaluate to a set

Radvendii commented 3 years ago

I don't think thats true for the overlays flake output.

Ah, okay. I stand corrected then.

I can't think of an actual solution to overlays evaluation issue, do you have anything in mind in terms of a fix? I doubt that it is even possible :/

It may not be possible to find a fix. It's a rather nasty surprise though, if someone makes a valid overlay, and someone else includes it in their config, and it breaks because flake-utils-plus is making assumptions about the structure of that overlay. How essential is knowing the names of the packages? Is it something we can take out? Is there a way to catch the error somehow and just ignore the packages for that overlay?

gytis-ivaskevicius commented 3 years ago

Okay so first of all it breaks overlays exporter, not everything. a possible workaround is to use builtins.removeAttrs function to keep it from crashing.

One of the ideas I thought of just now is to use builtins.tryEval function as a try/catch. Basically idea is to ignore overlay if evaluation fails

Radvendii commented 3 years ago

Yeah, that seems good. I should say that I wasn't using this directly, I was using github.com/divnix/digga, which called the function. So maybe this problem should be resolved there.

I guess it should at least be documented that this cannot be called on an arbitrary overlay.