nix: overlay polish for prev parameter

[GrizzlT]

Describe your PR, what does it fix/add?

After a good night's sleep, I realized I was a bit too quick in #4555. Since the overlay composition also overrides udis86, hyprland-protocols and wlroots-hyprland. These too, should be taken from prev instead of final, or my shadowing trick would not work anymore. This is changed in this PR.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

I don't know if callPackage should be taken from final or prev in this case. Since it's just a function, I assume either would be fine if there is no explicit overriding in this project. Something to note then would be the requirement to explicitly reference all custom inputs not taken from nixpkgs when making the hyprland derivation.

In the same mindset, one might want to change the references to final.hyprland to something tied to the scope of the overlay. There exists a function makeScope in nixpkgs for this. I have not looked at this yet and would be implemented in a following PR. For now, for my workflow with shadowing input overlays, this could easily be worked around by adding a overlay that exports the hyprland package from my shadowed name set, should I need these extra outputs.

Is it ready for merging, or does it need work?

ready, might want to add a comment

[fufexan]

Overlays make my head hurt. I'll have to ask @spikespaz to give his thoughts on this as well.

[spikespaz]

Glad to have learned about lib.composeManyExtensions today! Appears to work for both overlays and those functions which are used with extends.

[spikespaz]

This PR should be rebased to remove the merge commit, and rebased onto the main branch instead. Your changes are not rendered properly on GitHub web view, which makes it more challenging to review.

[spikespaz]

Since the idea of the overlay is to allow for composition, we should not rely on this exact derivation for any of the derivations in this flake. That would break downstream overrides.

I don't know if callPackage should be taken from final or prev in this case. Since it's just a function, I assume either would be fine if there is no explicit overriding in this project.

callPackage itself can be overridden (though unlikely), for example if cross-compiling. As mentioned in my review, it is inadvisable to be using prev.callPackage.

[spikespaz]

I have also looked at the previous PR, #4555. I welcome the composeManyExtensions usage, but we shouldn't depend on prev for "final" usages of an attribute.

[spikespaz]

I would like to just add one more thing: the naming final: prev: I find to be misleading. It should be self: super: which is more technically correct, with more accurate names. We use final and prev as a convention to make it easier for new users to grok the meaning.

[GrizzlT]

Great feedback, I'll make changes accordingly!

@spikespaz should I change final and prev in this PR as well? Or is that up for discussion some other day?

[spikespaz]

@GrizzlT Are you talking about renaming final and prev to self and super? If so, I would personally like to do this, however many would argue with me, saying that it heightens the learning curve. Truthfully I disagree with this sentiment; I think that self and super ought to be explained for what they are. It is similar to composing classes in a programming language (though this is only an analogy) and I think that is sufficient explanation for new Nix users.

Regardless of the disagreement that I've read (and participated in), I do name them differently from final and prev in my own flakes. However, I do not use self and super for a single reason: overrides (not overlays) can also use self and super as arguments to overrideAttrs. This causes a problem where the self/super from the outer scope is shadowed, making them inaccessible without renaming them in a let block. For this reason, I considered a few things:

• pkgs/lib and pkgsUnfix/libUnfix
  • More verbose than the last option.
• pkgs'/lib' and pkgs/lib
  • This is confusing, because some may try to use pkgs out of habit and end up with the first approximation of the Nixpkgs instance, where they probably meant to use pkgs'. This can confuse readers also.
• pkgs/lib and pkgs0/lib0
  • These are the names I use. I read the latter as "pkgs origin" (or "lib origin").
  • The name pkgs or lib means exactly the same thing (semantic equivalence) that it does in every other context outside of an overlay, meaning that within the overlay, naming is exactly consistent with the rest of the codebase that is not concerned with fixpoints.
  • This is probably contentious because it is unconventional, but it allows for self and super to be used in proper context in a lower scope.

[spikespaz]

I think my comments must be marked as "resolved" before I can approve properly. That just changed my existing requested changes into an approved review.

[GrizzlT]

@spikespaz Are you sure you're looking at the changes of all commits combined? My force-pushed commits should already contain all the changes requested. This looks like something from git(hub) on your side. (missing fetch?)

[GrizzlT]

callPackage itself can be overridden (though unlikely), for example if cross-compiling. As mentioned in my review, it is inadvisable to be using prev.callPackage.

It looks like self and super are explained in the nixcon talk back in 2017 in contradiction with your convention here. It is mentioned in that talk to call functions from super instead of self. Is there a new convention I didn't know to read about?

Would there be any merit to shadowing dependencies? Is it always encouraged to let downstream users "force" the use of a specific version of a dependency across all packages that have that dependency? I'm struggling to find resources that explain what to do when exposing an overlay that depends on the results of another overlay (such as fenix for rust).

[spikespaz]

It looks like self and super are explained in the nixcon talk back in 2017 in contradiction with your convention here.

I'll go watch that. My "convention" comes from reasoning with myself, plus trial and error to the effect of avoiding infinite recursion. Perhaps I am mistaken. If I am wrong, all of the flakes in this org (and others) are incorrect because I made pull requests specifically to address this problem. Perhaps the fault was with my usage, not the overlay definitions. I may also be misremembering.

I'm struggling to find resources that explain what to do when exposing an overlay that depends on the results of another overlay (such as fenix for rust).

This exactly is why the flake is structured as it is. I could not find an answer to this question either, and NobbZ in the Discord said essentially that it was up to me to make that decision. So I opted to provide all important derivations as their own overlay, and then also export ones that are joined together which contain the dependency overlays in the correct order. This is done for some reasons:

• If the user desires to not have shadowed package attributes in their pkgs, they may always use the flake's packages.${system} attributes.
  • If the user chooses overlays, they're asking for trouble anyway (me) and should be aware of the side-effects of overlays (potentially mass-rebuilds).
• Modified/patched/forked packages should be exported under a different name, and merged into pkgs with that new name, to avoid the last bullet point above. Example, wlroots-hyprland, locked just for Hyprland.
• If a package is not already in Nixpkgs (or existing is a very old unmaintained version) I think it's probably fine to use the original name, no suffixes.

[GrizzlT]

I am playing around with a new solution to this problem right now. I find it a limitation that dependencies of the final output package should be exposed to the pkgs root. This makes it difficult for example to use different versions of the rust compiler for different packages. And if you change the default rustPlatform through an overlay, you end up missing out on a bunch of binary cache speedup. There's an overall limiting factor to this approach, since it prohibits my shadowing attempt of which I personally think it should ideally be possible to do.

I propose the following method:

let
  overlayOutsideHidden = self: super: {
    myPackage = self.hello;
  };

  overlayCustomExposed = self: super: let
    extraPkgs = overlayOutsideHidden self super;
  in {
    myCustomHello = super.callPackage ./package.nix extraPkgs;
  };

  overlayOverrideCustom = self: super: {
    myCustomHello = super.myCustomHello.override { myPackage = self.fd; };
  };

  overlayOverrideOutsideDep = self: super: {
    hello = self.zoxide;
  };
in {
  overlay1 = overlayCustomExposed;
  overlay2 = overlayOverrideCustom;
  overlay3 = overlayOverrideOutsideDep;
}

I am assuming here that the hyprland package outputs are the main objective of the overlay in this project. I do this also because wlroots-hyprland, udis86 and hyprland-protocols seem to be used for version pinning purposes (like my rust scenario).

I showed 4 overlays to cover the edge cases I could think of:

• overlayOutsideHidden is a dependency overlay, it exposes some custom dependencies such as wlroots-hyprland, a rust version toolchain, udis86 pinned version etc. Its outputs are not exposed to the final nixpkgs fixpoint.
• overlayCustomExposed is the main overlay, this would be the same as the hyprland-packages overlay exposed through the flake.
• overlayOverrideCustom is for a downstream overlay that wants to change the version of wlroots-hyprland or udis86 for example. In my rust scenario, this would be a way to change the rust compiler used for a specific package.
• overlayOverrideOutsideDep is an example of how such a "hidden" overlay still works with downstream overrides on the nixpkgs fixpoint.

The only requirement of this convention is that all dependency overlays must not depend on package overrides defined in their own overlay (but there's an easy fix using lib.composeExtensions). For the dependencies in this project, this is currently the case.

I know my shadowing approach will not work with overlays that want to expose packages that depend on each other but even for those overlays, I find this dependency hiding useful. This is the case for the hyprland-unwrapped overlay output and others but I'm not really interested in using those so the impact would be much less.
Do you see any obvious problems with this approach? If so, are there things that can still be used from it?

[GrizzlT]

If the user desires to _not_ have shadowed package attributes in their `pkgs`, they may always use the flake's `packages.${system}` attributes.

True, although this has the cost of an extra nixpkgs fixpoint calculation. This might also make cross-compiling difficult but I can't say much about that. See my comment above for a possible solution.

  * If the user chooses overlays, they're asking for trouble anyway (me) and should be aware of the side-effects of overlays (potentially mass-rebuilds).

It would be nice to have more conventions for defining and using overlays.

* Modified/patched/forked packages should be exported under a different name, and merged into `pkgs` with that new name, to avoid the last bullet point above. Example, `wlroots-hyprland`, locked just for Hyprland.

I agree, also see my solution in the comment above where I do this by pushing the name change from the nixpkgs fixpoint to the package derivation definition (also a change committed to this PR).

[spikespaz]

So, a few days ago (as per your suggestion), I watched the first 4/5 of Nicolas Pierron's standup at NixConf 2017. He did briefly show a "bad example", telling us that functions ought to be used from super rather than self, but he provided no explanation as to why. I continued watching, hoping for technical description of implementation detail, but then stopped because the remainder of the presentation was about usage of the new mechanism at Mozilla specifically for their own purposes.

I didn't immediately discredit this evidence though, and persisted in my research. I inspected the relevant section of the Nixpkgs manual which states that:

The second argument (super) [...] should be used either to refer to packages you wish to override, or to access functions defined in Nixpkgs.

That statement, again, with no explanation whatsoever.

I question this, since to me, and through experience, using functions from self (the final evaluated fix-point) makes the most sense, as this allows the functions themselves to be subject to the overlay mechanism proper.

I followed the commit history back to the introduction of the original doc/languages-frameworks/overlays.xml and discovered that this statement was introduced by Nicolas himself and has never been modified. This is reaffirming: I now wonder if this particular advice warrants a revision, at the very least, an explanation. For the last seven years, this trite remark has been accepted as the source of truth.

As it turns out, I'm not the first person to question this. In another PR, a commit tries to change super.callPackage to self.callPackage and Nicolas requests the change to be dropped. @orivej asks for a reason to which there are two responses:

The first, and most convincing, is that there is a performance penalty to using functions from self. @orivej makes a rebuttal, with an example that shows this is not the case unless the function is overridden.

As an aside, as far as I am concerned: even if the function is overridden, the performance penalty is the cost of intended behavior and should not be a consideration.

The second argument from Nicolas is that there would be some undesirable (yet unspecified) interaction with a feature called "automatic grafting". This feature was never merged into Nixpkgs.

Lastly, I found another forum post, in which lies a comment by Silvian Mosberger (@infinisil, a member of the NixOS organization):

My conclusion from writing overlays for some years is that the only good reason for using super is to access attributes you want to override, to avoid infinite recursion. This effectively means that the resulting attribute set can be represented with a single fixed-point function self: { ... } and everything can be further overridden without any hidden values being unchangeable. This also makes a lot of sense if you think of overlays as just a way to extend such a fixed-point function (which they are, see also lib.makeExtensible).

I believe this summarizes what my own experience and intuition indicated to me: prefer self for all of which you do not directly modify, so that any attributes therein are made subject to the overlay mechanism proper, and that super ought to be used only for attributes which you wish to override. The only exception to this practice which I (and evidently others) recommend is where infinite recursion crops up, and you end up using super instead to wish away the problem.

[spikespaz]

Please remember to revert: https://github.com/hyprwm/Hyprland/pull/4555/files#diff-2ad718dbee070e69c53ba853ab0fb35c49816b00d91c07bab2c024acd81a6409L37

[spikespaz]

you end up missing out on a bunch of binary cache speedup.

Using overlays, you knowlingly give this up.

since it prohibits my shadowing attempt of which I personally think it should ideally be possible to do.

Can you please elaborate? What do you mean by shadowing?

I am assuming here that the hyprland package outputs are the main objective of the overlay in this project.

The hyprland package overlays are for adding composable attributes to the top level pkgs used throughout the configuration.

I do this also because wlroots-hyprland, udis86 and hyprland-protocols seem to be used for version pinning purposes (like my rust scenario).

Yes, but the overlays output of this flake should also optionally provide the pinned packages used here, fairly granularly. Renaming them with a hyprland- prefix would be appropriate.

I showed 4 overlays to cover the edge cases I could think of

I'm reading through instances of scope-making in Nixpkgs. There seems to be a few ways to do it, but looks like one of them allows providing overlays to the scope.

If we do that, we can have the overlays exported as flake outputs, but do not export them in the larger default package set overlays.

The other way to do it is importing Nixpkgs twice. We don't want to do that.

Your approach here:

let
  extraPkgs = overlayOutsideHidden self super;
in

Is an incorrect application of an overlay. The self provided to it would not be the final package set, this is not how overlays are instantiated.

makeScope in nixpkgs handles this, but it may be hard to understand.

https://github.com/NixOS/nixpkgs/blob/27575f4f60da0664047e4ee8ce2797b969b4492a/lib/customisation.nix#L321

I quite like this example, but I believe it is outdated:

https://github.com/NixOS/nixpkgs/blob/27575f4f60da0664047e4ee8ce2797b969b4492a/pkgs/development/idris-modules/default.nix#L1-L7

[GrizzlT]

you end up missing out on a bunch of binary cache speedup.

Using overlays, you knowlingly give this up.
Can you please elaborate? What do you mean by shadowing?

I apologize for the lack of context so far, the following is an overview of my thinking:

I liked the idea of automatic input extraction in my flake for my nixossystem configs. Instead of manually defining all the overlays used for my systems, I wanted to include all default overlays of the flake inputs automatically. In an attempt to come up with something clever, I deemed it a good idea to wrap the overlays of the inputs to my flake into an attribute set so that my automation wouldn't accidentally run into name clashes of the nixpkgs fixpoint (unlikely, but I did it in the spirit of figuring out whether that's good practice or not).

I was then confronted by hyprland's need for wlroots-hyprland to be exposed in the nixpkgs fixpoint.
This led to my two PRs on this repo. It also led to a good bit of thinking. "Can I hide wlroots-hyprland from the nixpkgs fixpoint but still access the hyprland package?", "Should I do this?" and "What does makeScope have to do with this?" were all questions I didn't know know the answers to.

While I currently don't think I should commit to this "auto-extract packages from input overlays" idea, I still think it might be valuable to be able to use derivations from an outside overlay without unnecessary exposure of dependencies from said overlay to the nixpkgs fixpoint.

I think this because I want to create a Rust project with Nix and I would like to choose the compiler version used in the derivation. The two most interesting overlays for this purpose seem fenix and rust-overlay in my opinion.

Maybe it's my tendency to abstract that doesn't align well with nixpkgs but I don't like the idea of multiple rust packages exposing the same overlay only to use a specific rust compiler from that overlay. Instead of exposing all the compilers to the fixpoint, I would much rather just pick a specific compiler and export only that one compiler under a different name linked to my project. This would allow for the proper overlay mechanism to still provide flexibility for users that which override the compiler used in my project, while exposing a clean, minimal set of derivations.

I'm reading through instances of scope-making in Nixpkgs. There seems to be a few ways to do it, but looks like one of them allows providing overlays to the scope.

Your approach here:

let
  extraPkgs = overlayOutsideHidden self super;
in

Is an incorrect application of an overlay. The self provided to it would not be the final package set, this is not how overlays are instantiated.

makeScope in nixpkgs handles this, but it may be hard to understand.

I am interested in the specific way you discovered. So far I came up with this snippet to scope the outputs of an overlay.

let 
  overlayOutsideHidden = self: super: {
    myPackageDependency = self.hello;
  };

  overlayToScope = overlay: (final: prev: (
    lib.makeScope final.newScope (self:
      final // (overlay self (prev // { inherit (self) callPackage; }))
    )
  ));
in
  myOverlay = self: super: let
    extraPkgs = overlayToScope overlayOutsideHidden self super;
  in {
    myCustomHello = self.callPackage ({ myPackageDependency }: myPackageDependency) extraPkgs; # this is oversimplified, not all attributes from extraPkgs are used
  };

This way, no overlay out there needs to change and as long as no one uses super.callPackage, I think this has perfect access to the nixpkgs fixpoint and allows for recursive outputs in a single overlay (fixing the problems with my previous snippet). To avoid the issue of missing derivations for recursive overlays that do use super.callPackage, I explicitly override callPackage in super.

This reinforces the idea for me that callPackage should be used from self and not super.

[GrizzlT]

@spikespaz This PR is also ready for merge last I checked, I only changed the overlay to use final over prev everywhere. I also changed the name of wlroots in the hyprland derivation to wlroots-hyprland per exposed in the overlay. I think these few changes should be in line with everything you've said.

[spikespaz]

Oh, I approved it just because I'm struggling with the UI (I don't think it's me, but maybe). However, I do request that, since we are here, you rename the default.nix's udis86 input to hyprland-udis86 since you pointed out it's in Nixpkgs now. That is broken. This rename can be reverted later when there's another PR for the package scope (to hide version-locked dependencies from the default overlay).

Aside from that, all looks good.

[GrizzlT]

I realized that changing wlroots in the package derivation to wlroots-hyprland is a breaking change, do we want to keep compatibility in this PR or introduce breaking changes?

[fufexan]

I would like not to break compatibility, as long as it's not too much work to get the overlays working fine as well.

[GrizzlT]

Then I think it's best to revert #4555's changes to final and only rename udis86 to hyprland-udis86 in the overlay. Scoping the overlay outputs should be discussed in a future PR. Since I found a different solution for my original problem, there's not really a need to change much.

[fufexan]

Perhaps we should have udis86-hyprland instead of hyprland-udis86, to be more in line with the already existing wlroots-hyprland.

[fufexan]

LGTM, thank you!