lib.makeOverridable: propagate .override

[wolfgangwalther]

Just a draft to resolve #441492 (comment):

Standard by-name caveat, though: you actually need to do { mkRenameAlias, ... }@args: (mkRenameAlias "25.11" "renamed-package").override args for renames, which sucks.

With this PR, you can do:

nix repl . --arg config { allowUnfree = true; }

nix-repl> _1password-gui-beta.override { channel = "stable"; lib = { inherit (lib) maintainers licenses sourceTypes; }; polkitPolicyOwners = [ "foo" ]; }
«derivation /nix/store/kgxk8mpa1wszjzdab9gf4yvjaidxkcpc-1password-8.11.8.drv»

where:

• channel = stable is passed to https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/_1/_1password-gui-beta/package.nix
• lib is only passed to https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/_1/_1password-gui/package.nix, but not ./linux.nix - otherwise it would error because stuff in lib is failing.
• polkitPolicyOwners is passed to https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/_1/_1password-gui/linux.nix

The idea is that outer .overrides "consume" arguments and pass on what is left.

In theory, this should not be a breaking change, because arguments that the outer function can't consume would currently throw an error. Unless... people are using ... }@args stuff. I hit some of that in https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/libraries/gobject-introspection/wrapper.nix#L27-L33 - and just temporarily worked around that for now.

I'd like to discuss the idea first - is this something that we want to have? Is this something that we can actually do given the breaking change potential for this specific @args use-case?

Even if we have breaking changes.. we surely have a lot more packages just flat out doing it wrong, which we would fix that way. _1password-gui is just the first thing that I hit, I'm sure.

WDYT @emilazy?

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[emilazy]

I am not sure I have a super clear opinion on whether this is a good thing or not. I dislike .override in general for mixing defined public API (feature flags, dependencies explicitly intended to be overridden) and .overrideAttrs‐style monkey‐patching. So my initial reaction is a negative “this is exposing even more implementation details of packages”, but… we’re already pretty deep down that hole already.

I do think that not being able to express this kind of thing nicely is a wart of by-name.

I wonder about the perf impact.

[wolfgangwalther]

I wonder about the perf impact.

I do as well, but I would have to fix a lot more stuff before I can get a report :/

[wolfgangwalther]

I am not sure I have a super clear opinion on whether this is a good thing or not. I dislike .override in general for mixing defined public API (feature flags, dependencies explicitly intended to be overridden) and .overrideAttrs‐style monkey‐patching. So my initial reaction is a negative “this is exposing even more implementation details of packages”, but… we’re already pretty deep down that hole already.

The thing is.. that .overrideAttrs already works in these cases. So right now, in these scenarios, overrideAttrs just works - while override doesn't. In light of #421201, we should make .override better than it is right now.

I do dislike the confusion of flags and dependencies as well - and with overrideAttrs discouraged, it will only become more, because now a lot of packages will need to expose things like src, version, hash or something like that. I do think that's a different problem to solve, though.

[emilazy]

now a lot of packages will need to expose things like src, version, hash or something like that

I suspect not: most packages aren’t generic builders made to build arbitrary things vaguely related to the thing they’re packaging. Rather, the cases where something so low‐level would be needed needed are precisely the cases where the choice between a multi‐version package, a generic builder function, and a vendored derivation should be made.

[wolfgangwalther]

Well yes, but how do you integrate a multi-version package with a builder function into by-name?

I see these options:

• A default package with .override-able function args, or
• A builder function as a separate top-level attribute, which takes these things as arguments and is called explicitly from the versioned attributes.

Even if you use the builder function, it will end up the same - you need to deal with two callPackage calls and thus two .override layers.

[wolfgangwalther]

I do think that not being able to express this kind of thing nicely is a wart of by-name.

With the same idea used in #442066, we could probably also make overrides "first class", something like:

{
  type = "override";
  package = "foo";
  override = self: old: {
    bar = old.bar.override { foobar = self.foobar_1_0; };
  };
}

This would then not go through callPackage again.