By analyzing the blame information on this pull request, we identified @thoughtpolice, @edolstra and @shlevy to be potential reviewers

@henrytill maybe good for musnix?

I'm tempted to factor out the recursive bits into something reusable.

Would something like this in lib/trivial.nix make sense? (aside from naming, for which I'd like suggestions)

  # Compose two extension functions:
  #
  #     nix-repl> h = self: super: { bar = "<${super.bar}>"; }
  #
  #     nix-repl> fix (extends (composeExtensions h g) f)
  #     { bar = "<bar>"; foo = "foo + "; foobar = "foo + <bar>"; }
  composeExtensions = f: g: self: super:
    let attrs = g self super;
        super' = super // attrs;
    in attrs // f self super';

  # Create an overridable, recursive attribute set. For example:
  #
  #     nix-repl> obj = makeObject (self: { })
  #
  #     nix-repl> obj
  #     { override = «lambda»; }
  #
  #     nix-repl> obj = obj.override (self: super: { foo = "foo"; })
  #
  #     nix-repl> obj
  #     { foo = "foo"; override = «lambda»; }
  #
  #     nix-repl> obj = obj.override (self: super: { foo = super.foo + " + "; bar = "bar"; foobar =
  #     self.foo + self.bar; })
  #
  #     nix-repl> obj
  #     { bar = "bar"; foo = "foo + "; foobar = "foo + bar"; override = «lambda»; }
  makeObject = f:
    let
      construct = overrides: self:
        let
          override = f:
            let overrides' = composeExtensions f overrides;
            in fix (extends overrides' (construct overrides'));
        in f self // { inherit override; };
    in fix (construct (self: super: {}));

I pushed up a second commit that shows what this would look like with the recursive bits factored out.

What do you think? I'd like some feedback before this gets merged.

Actually, I suppose this would be a better/leaner implementation of the the makeObject function that I proposed above:

  makeObject = rattrs:
    let
      construct = rattrs:
        fix (self: rattrs self // {
          override = f: construct (extends f rattrs);
        });
    in construct rattrs;

@dezgeg What are your thoughts on the makeObject function?

Does

{
  boot.kernelPackages = with pkgs;
    let self = linuxPackagesFor (kernel.override { kernelPatches = [ /* ... */ ]; }) self; in self;
}

not accomplish the same thing?

@joachifm

That can definitely work, however:

1. That doesn't compose well. For example, you can't have the grsecurity kernel module set that option while simultaneously setting that option yourself to apply additional patches -- you can only have one or the other.
2. That's a whole lot of work just to apply some patches, and it requires that you know about linuxPackagesFor and how it works (and the oddity of having to pass self yourself).

As an example, when we want to extend/override Haskell packages, we aren't required to re-implement haskell via haskell = pkgs.callPackage <nixpkgs/pkgs/haskell-packages.nix> { /* ... overrides ... */ };. Instead, we just provide an "overrides" function, thus keeping knowledge of the implementation out of our configurations. Presently, we can't extend the linuxPackages -- to add or modify a package in that set, we have to completely re-evaluate the expressions that make up the set.

I believe this PR makes things compose better and makes the kernel packages extensible without leaking knowledge of the implementation details into your configuration.

This interface is better, no doubt about it. I'm not convinced composing kernel patches like this is really sane, but it is quite user unfriendly to do it at all right now, so ...

I'm not convinced composing kernel patches like this is really sane, but it is quite user unfriendly to do it at all right now, so ...

Right - the more patches you throw in, the greater the chance that one of the patches won't apply (or worse, will apply, but will be buggy). For the simple, 99.9% case where someone just wants to add their patches to the default linuxPackages value, the user won't run into any problems, and this provides a much lower barrier to getting their patches applied.

And, of course, having a way to add packages to the set of linuxPackages is only a good thing, given that currently linuxPackages is a closed set at the moment.

Big +1 from me! I think in a follow-up PR it should be fairly straightforward to deal with with the multiple linuxPackages too----platform needs an overhaul anyways (#10874). But that is out of scope for this PR.

@Ericson2314 Thanks for reviewing :)

I see a potential problem regarding the above definitions of makeObject: its idea of override is different than callPackage's.

callPackage provides an override attribute with allows you to change the inputs to a function, while makeObject provides an override attribute that allows you to modify and/or extend the outputs. If you compose callPackage over makeObject, the override function from callPackage clobbers the one from makeObject. Even with that problem aside, the conflation of the word override is potentially confusing.

To address that last point, I'm thinking that makeObject should instead provide an extend attribute, that way the word override isn't overloaded.

That also happens to resolve the first point, but that does make me wonder about how combinations of override and extend should behave. With that change alone, doing (somePkgSet.extend (self: super: { /* ... */ }).override { /* ... args ... */ } will result in the extensions from extFn being ignored, as override will result in the package function being re-evaluated. In a similar vein, the result of extend will lose the override attribute. Adding a new function analogous to callPackage but for recursive, extensible package sets would fix that. I see two alternative implementations:

Option 1 - don't preserve extensions when overriding

callRecPackagesWith = autoArgs: fn: args:
  let
    f = if builtins.isFunction fn then fn else import fn;
    auto = builtins.intersectAttrs (builtins.functionArgs f) autoArgs;
    go = args: lib.makeObject (self:
      f args self // { override = newArgs: go (args // newArgs); }
    );
  in go (auto // args);

# Examples (common between both implementations):
fn = { foo, bar, baz, quux }: self: { inherit foo bar baz quux; };
crp = callRecPackagesWith { foo = "foo"; bar = "bar"; baz = "baz"; quux = "quux"; };
a = crp fn { foo = "AAA"; };
b = a.override { bar = "BBB"; };
c = b.extend (self: super: { baz = super.baz + " CCC"; });
d = c.override { baz = "DDD"; };

nix-repl> :l ./lib
Added 321 variables.

nix-repl> a
{ bar = "bar"; baz = "baz"; extend = «lambda»; foo = "AAA"; override = «lambda»; quux = "quux"; }

nix-repl> b
{ bar = "BBB"; baz = "baz"; extend = «lambda»; foo = "AAA"; override = «lambda»; quux = "quux"; }

nix-repl> c
{ bar = "BBB"; baz = "baz CCC"; extend = «lambda»; foo = "AAA"; override = «lambda»; quux = "quux"; }

nix-repl> d
{ bar = "BBB"; baz = "DDD"; extend = «lambda»; foo = "AAA"; override = «lambda»; quux = "quux"; }

Note how in d we lose the extension on baz.

Option 2 - do preserve extensions when overriding

callRecPackagesWith = autoArgs: fn: args:
  let
    f = if builtins.isFunction fn then fn else import fn;
    auto = builtins.intersectAttrs (builtins.functionArgs f) autoArgs;
    go = extensions: args:
      lib.fix (self:
        lib.extends extensions (f args) self
          // { override = newArgs: go extensions (args // newArgs); }
          // { extend = extFn: go (lib.composeExtensions extFn extensions) args; }
      );
  in go (self: super: {}) (auto // args);

# Examples (common between both implementations):
fn = { foo, bar, baz, quux }: self: { inherit foo bar baz quux; };
crp = callRecPackagesWith { foo = "foo"; bar = "bar"; baz = "baz"; quux = "quux"; };
a = crp fn { foo = "AAA"; };
b = a.override { bar = "BBB"; };
c = b.extend (self: super: { baz = super.baz + " CCC"; });
d = c.override { baz = "DDD"; };

nix-repl> :l ./lib
Added 321 variables.

nix-repl> a
{ bar = "bar"; baz = "baz"; extend = «lambda»; foo = "AAA"; override = «lambda»; quux = "quux"; }

nix-repl> b
{ bar = "BBB"; baz = "baz"; extend = «lambda»; foo = "AAA"; override = «lambda»; quux = "quux"; }

nix-repl> c
{ bar = "BBB"; baz = "baz CCC"; extend = «lambda»; foo = "AAA"; override = «lambda»; quux = "quux"; }

nix-repl> d
{ bar = "BBB"; baz = "DDD CCC"; extend = «lambda»; foo = "AAA"; override = «lambda»; quux = "quux"; }1

Note how in d we keep the extension on baz, and we see that d.baz == "DDD CCC".

TL;DR

I think I'd like to have the second callRecPackagesWith (though I'd like a less atrocious name for it), and I'd like to rename makeObject's override to extend.

@Ericson2314 How does that sound?

I rebased on top of the tip of master and made some of the suggested changes. I chose the word "extensible" (e.g. in makeExtensible) over "extendable" per https://stereochro.me/weblog/3246, despite the lack of symmetry between the words "extend" and "extensible":

People get these works mixed up all the time, but, at least in technical circles, the difference is important.

Let’s imagine you have a rod. The rod can be said to be extendible if it’s telescoping (like an antenna), or if it can be folded up and unfurled (like the support rods for a tent). The rod can be said to be extensible if you can put attachments onto the end to vary what the rod may be used for.

And that’s why XML stands for ‘eXtensible Markup Language’ and why EPP stands for ‘Extensible Provisioning Protocol’. It’s not just some arbitrary choice of synonym.

@domenkozar How does this look now? If the changes look good, I can add docs in functions.xml when I have a spare moment.

I won't have time to look at this this week, but noone else does, I'll do my best next week.

@domenkozar Any chance you'll have some free time to take a look at this? 😄

@cstrahan after 16.09 :)

Understandable, though it would be nice to have 16.09 :)

Agreed, but it's going to be released tomorrow so that ship is already gone. We can talk about backporting once it's stable enough in the master branch.

Besides the merge conflict, this is ready now? I'm very interested in seeing this merged :)

Good job, @cstrahan rebase and merge :)

I did my changes and rebased in https://github.com/Ericson2314/nixpkgs/tree/kernel-patches

@Ericson2314 If you'd like to correct that broken reference to override (that should be extend instead) and push/merge you changes into master, I'm cool with that (actually, I'd appreciate it 😉).

Alternatively, I have a deployment to tend to tonight, and I'll either have time much later tonight, or I can try to push this up tomorrow.

@cstrahan I would do the honors, but I don't have push :). There's no rush so tomorrow is fine.

Eh, I just went ahead and got it out of the way -- it only took a minute or two to rebase.

It seems pretty ad hoc to have an option for applying patches to the kernel. Why not add an option for applying patches to Firefox, or any of the thousands of packages in Nixpkgs? This opens the door for a huge amount of option bloat.

@edolstra I believe it is because Linux already was exposing parameters for patches, unlike most packages? This old PR merely reexposes that parameter on the NixOS level.