[Nix-dev] Feedback requested: new `boot.kernelPatches` option

[Charles Strahan]

Hello,

Nahum Shalman and I were chatting on #nixos, and he was having the same
trouble I once had when trying to apply kernel patches. As a result of
that conversation, I have opened a pull request
(https://github.com/NixOS/nixpkgs/pull/15095) adding support for easily
specifying kernel patches in one's NixOS configuration, and I'd greatly
appreciate some feedback.

Here's an example of using the new boot.kernelPatches option:

  boot.kernelPatches = [ pkgs.kernelPatches.ubuntu_fan_4_4 ];

A bit of back story, as well as a description of its use and
implementation:

Back in August of 2015, I needed to apply some kernel patches. The first
thing I did was look to the wiki
(https://nixos.org/wiki/How_to_tweak_Linux_kernel_config_options), which
suggested doing something like this:

  nixpkgs.config = {
    packageOverrides = pkgs: {
      stdenv = pkgs.stdenv // {
        platform = pkgs.stdenv.platform // {
          kernelPatches = [
            { patch = ../patches/i915_317.patch; name = "i915-fix"; };
            { patch =
            ../patches/override_for_missing_acs_capabilities.patch;
              name = "acs_overrides"; };
          ];
        };
      }; 
    };
  };

I find that rather unintuitive (if not an outright hack). After studying
the relevant expressions, I amended the wiki page to suggest the
following (which is almost identical to the suggestion in the manual
today):

   nixpkgs.config = {
     packageOverrides = super: let self = super.pkgs; in {
       linux_3_18 = super.linux_3_18.override {
         kernelPatches = super.linux_3_18.kernelPatches ++ [
           # we'll add the Ubuntu Fan Networking patches from Nixpkgs
           self.kernelPatches.ubuntu_fan

           # and we'll also add one of our own patches
           { patch = ../patches/i915_317.patch; name = "i915-fix"; }
         ];

         # add "CONFIG_PPP_FILTER y" option to the set of kernel options
         extraConfig = "PPP_FILTER y" ;
       };
     };
   };

That's still pretty bad, as we're attempting override the kernel while
hoping that `linux_3_18` is the kernel used by the `linuxPackages`
attribute (which happens to be the default value for the
`boot.kernelPackages` NixOS option) -- too many assumptions being made,
too fragile.

Let's take another look at the new `boot.kernelPatches` option:

  boot.kernelPatches = [ pkgs.kernelPatches.ubuntu_fan_4_4 ];

Much better, no?


To implement this option, I also had to add support for overriding the
kernel and/or packages within a given `linuxPackages` set. This is done
by using the new `linuxPackages.override` attribute like so:

  pkgs.linuxPackages.override (self: super: {
    kernel = super.kernel.override {
      kernelPatches = super.kernel.kernelPatches ++ [
      pkgs.kernelPatches.ubuntu_fan_4_4 ];
    };
  });

As a result, setting fine grained options for one's kernel (or adjusting
each of the kernel packages) can look something like this:

  boot.kernelPackages = pkgs.linuxPackages.override (self: super: {
    kernel = super.kernel.override {
      kernelPatches = super.kernel.kernelPatches ++ [
      pkgs.kernelPatches.ubuntu_fan_4_4 ];
      extraConfig = "PPP_FILTER y" ;
    };
  });

I think that's a lot cleaner. Granted, the value of
`boot.kernelPackages` will not be the same value as `pkgs.linuxPackages`
(e.g. the value that the rest of nixpkgs refers to), but that's
addressed by doing the following instead:

  nixpkgs.config = {
    packageOverrides = super: let self = super.pkgs; in {
      linuxPackages = super.linuxPackages.override (self: super: {
        kernel = super.kernel.override {
          kernelPatches = super.kernel.kernelPatches ++ [
          pkgs.kernelPatches.ubuntu_fan_4_4 ];
          extraConfig = "PPP_FILTER y" ;
        };
      });
     };
   };


(Ideally, the `boot.kernelPatches` option would effectively do the
latter, that way the system closure doesn't include two
kernels/kernelPackages: one for the running kernel (i.e.
boot.kernelPackages) and another for every occurrence of `linuxPackages`
within nixpkgs. Regardless, this pull request doesn't make things any
worse off: today, if you set the `boot.kernelPackages` options to
anything other than `pkgs.linuxPackages`, you now have *two* sets of
kernel packages in your system's build-time (and maybe also run-time)
closure. In the future, we might want to find some way to push the value
of `boot.kernelPackages` into nixpkgs, as we don't have a suitable
mechanism for that today.)


After implementing the `override` mechanism for `linuxPackages`, I
realized it would be trivial to factor out a reusable function:

  # 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 = rattrs:
    let
      construct = rattrs:
        fix (self: rattrs self // {
          override = f: construct (extends f rattrs);
        });
    in construct rattrs;


(I'd love suggestions for what to name it -- makeObject was just the
first thing that came to mind.)

One of the nice things about this implementation is that you can chain
as many `.override` as you want, whereas I've seen similar ad hoc
implementations in nixpkgs that allow for specifying overrides once
(e.g. haskellPackages) -- or worse, each `.override` "forgets" the
previous invocation. Aside from the inherent brain-hurt induced by
laziness and fixpoints, I think this implementation is pretty
straightforward and clearly "does the right thing"; I think there would
be value in encapsulating this pattern in nixpkgs once, rather than
having a bunch of ad hoc implementations that vary (quite subtly) in
their semantics.


So, with all of that said, what do you think of the new
`boot.kernelPatches` configuration option?

What are your thoughts on the `makeObject` function, and what would be a
better name for it?

I don't feel comfortable pushing this to master without having some
feedback on the overrides mechanism, as I don't want to see a new
pattern/abstraction get used pervasively in nixpkgs only to
(potentially) find that it was the wrong pattern/abstraction. Any
feedback would be greatly appreciated so I can move forward with the
pull request.

-Charles