nixos: make it easy to apply kernel patches #15095

cstrahan · 2016-04-30T04:25:52Z

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

This makes it easy to specify kernel patches:

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

To make the boot.kernelPatches option possible, this also makes it easy
override the kernel and/or packages within a linuxPackages set. For
example:

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

For additional context, you can read my letter to the mailing list: http://lists.science.uu.nl/pipermail/nix-dev/2016-May/020342.html

/cc @thoughtpolice @nshalman

mention-bot · 2016-04-30T04:26:03Z

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

magnetophon · 2016-04-30T20:40:12Z

@henrytill maybe good for musnix?

cstrahan · 2016-04-30T22:22:29Z

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: {}));

cstrahan · 2016-04-30T22:38:50Z

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.

cstrahan · 2016-05-01T00:14:14Z

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;

cstrahan · 2016-05-01T20:26:16Z

@dezgeg What are your thoughts on the makeObject function?

joachifm · 2016-05-01T21:02:42Z

Does

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

not accomplish the same thing?

cstrahan · 2016-05-01T21:21:30Z

@joachifm

That can definitely work, however:

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.
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.

joachifm · 2016-05-01T21:47:57Z

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 ...

Ericson2314 · 2016-05-01T21:50:07Z

lib/trivial.nix

+          override = f: construct (extends f rattrs);
+        });
+    in construct rattrs;
+


Off topic: but we should make a fix.nix/combinator.nix/etc for all these. It's getting rather presumptuous to have all this is trivial.nix. :)

Yeah, it's not looking so trivial anymore :). I can move things around, if we so wish.

cstrahan · 2016-05-01T21:52:51Z

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.

Ericson2314 · 2016-05-01T22:04:04Z

pkgs/top-level/all-packages.nix

+  linuxPackages_4_3 = recurseIntoAttrs (self.linuxPackagesFor self.linux_4_3);
+  linuxPackages_4_4 = recurseIntoAttrs (self.linuxPackagesFor self.linux_4_4);
+  linuxPackages_4_5 = recurseIntoAttrs (self.linuxPackagesFor self.linux_4_5);
+  linuxPackages_testing = recurseIntoAttrs (self.linuxPackagesFor self.linux_testing);
  linuxPackages_custom = {version, src, configfile}:


Damn that's some crazy indenting. IIUC our style these days + removing unneeded bindings would make it more like:

let # the `let` is just for syntax highlighting linuxPackages_custom = {version, src, configfile}: recurseIntoAttrs (self.linuxPackagesFor (self.linuxManualConfig { inherit version src configfile; allowImportFromDerivation = true; }));

I wanted to keep the diff easy to view, but I suppose I could also reformat things. I think I'd prefer to do that in a separate PR, though.

Yeah there's "we're touching the lines anyways" on one and and "PR mission creep" on the other.

Ericson2314 · 2016-05-01T22:33:48Z

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.

cstrahan · 2016-05-01T22:49:10Z

@Ericson2314 Thanks for reviewing :)

cstrahan · 2016-05-02T03:43:12Z

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?

cstrahan · 2016-09-12T16:43:00Z

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.

domenkozar · 2016-09-12T16:44:40Z

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

cstrahan · 2016-09-26T20:10:45Z

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

This makes it easy to specify kernel patches: boot.kernelPatches = [ pkgs.kernelPatches.ubuntu_fan_4_4 ]; To make the `boot.kernelPatches` option possible, this also makes it easy to extend and/or modify the kernel packages within a linuxPackages set. For example: pkgs.linuxPackages.extend (self: super: { kernel = super.kernel.override { kernelPatches = super.kernel.kernelPatches ++ [ pkgs.kernelPatches.ubuntu_fan_4_4 ]; }; });

domenkozar · 2016-09-28T19:23:03Z

@cstrahan after 16.09 :)

cstrahan · 2016-09-28T20:30:48Z

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

domenkozar · 2016-09-29T10:57:11Z

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.

Ericson2314 · 2016-10-04T18:28:52Z

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

domenkozar · 2016-10-07T18:46:00Z

Good job, @cstrahan rebase and merge :)

Ericson2314 · 2016-10-07T18:59:52Z

lib/trivial.nix

+  #     nix-repl> obj
+  #     { __unfix__ = «lambda»; bar = "bar"; extend = «lambda»; foo = "foo + "; foobar = "foo + bar"; }
+  makeExtensible = rattrs:
+    fix' rattrs // {


I think this can be plain fix? Will work either way

Ericson2314 · 2016-10-11T22:08:48Z

nixos/modules/system/boot/kernel.nix

@@ -21,6 +23,11 @@ in

    boot.kernelPackages = mkOption {
      default = pkgs.linuxPackages;
+      apply = kernelPackages: kernelPackages.override (self: super: {


Shouldn't this be .extend?

Oh, yikes. Yes -- thanks for catching that. Didn't catch that after the renaming override to extend.

Ericson2314 · 2016-10-11T22:12:07Z

pkgs/top-level/all-packages.nix

@@ -11419,121 +11419,123 @@ in
     for a specific kernel.  This function can then be called for
     whatever kernel you're using. */

-  linuxPackagesFor = kernel: self: let callPackage = newScope self; in rec {
+  linuxPackagesFor = kernel: lib.makeExtensible (self: {


Come to think of it, we could just do (self: with self; { and cut down a lot of the diff.

I'm not opposed to that.

Ericson2314 · 2016-10-11T22:46:41Z

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

cstrahan · 2016-10-11T23:26:00Z

@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.

Ericson2314 · 2016-10-11T23:26:52Z

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

cstrahan · 2016-10-12T00:01:53Z

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

nbp · 2017-07-16T16:39:52Z

pkgs/top-level/all-packages.nix


-    acpi_call = callPackage ../os-specific/linux/acpi-call {};
+    acpi_call = self.callPackage ../os-specific/linux/acpi-call {};


I would prefer if this line could be read as super.callPackage, in order to avoid mistakes while using the overlays.

edolstra · 2017-07-17T11:31:41Z

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.

Ericson2314 · 2017-07-17T15:12:17Z

@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.

cstrahan force-pushed the kernel-patches branch 2 times, most recently from d0cc79c to a618189 Compare April 30, 2016 04:27

cstrahan force-pushed the kernel-patches branch 2 times, most recently from c7fc7eb to 3f99906 Compare April 30, 2016 22:49

cstrahan force-pushed the kernel-patches branch 3 times, most recently from cbd0a18 to aacb684 Compare May 1, 2016 18:19

Ericson2314 reviewed May 1, 2016
View reviewed changes

cstrahan force-pushed the kernel-patches branch from aacb684 to d3f5c1b Compare May 1, 2016 22:01

Ericson2314 reviewed May 1, 2016
View reviewed changes

cstrahan force-pushed the kernel-patches branch from d3f5c1b to 3c129d2 Compare May 2, 2016 02:14

cstrahan force-pushed the kernel-patches branch from 071d71d to 89e62c9 Compare September 12, 2016 16:47

cstrahan force-pushed the kernel-patches branch from 89e62c9 to d1030ef Compare September 26, 2016 20:18

domenkozar added this to the 17.03 milestone Sep 28, 2016

domenkozar self-assigned this Sep 28, 2016

Ericson2314 reviewed Oct 7, 2016

View reviewed changes

fpletz approved these changes Oct 8, 2016

View reviewed changes

Ericson2314 reviewed Oct 11, 2016

View reviewed changes

cstrahan closed this in da36847 Oct 12, 2016

This was referenced Oct 25, 2016

nixos: fix kernel override logic #19855

Closed

nixos: fix minimal-kernel override mechanism #19970

Closed

cstrahan mentioned this pull request Feb 27, 2017

haskellPackages: use makeExtensible #23192

Merged

7 tasks

cstrahan mentioned this pull request Jul 14, 2017

Improved makeOverridable #27319

Merged

nbp reviewed Jul 16, 2017

View reviewed changes

ncfavier mentioned this pull request Mar 1, 2022

Make lib more unified and overridable #157056

Closed


		acpi_call = callPackage ../os-specific/linux/acpi-call {};
		acpi_call = self.callPackage ../os-specific/linux/acpi-call {};

nixos: make it easy to apply kernel patches #15095

nixos: make it easy to apply kernel patches #15095

Conversation

cstrahan commented Apr 30, 2016 • edited Loading

Things done

mention-bot commented Apr 30, 2016

magnetophon commented Apr 30, 2016

cstrahan commented Apr 30, 2016 • edited Loading

cstrahan commented Apr 30, 2016 • edited Loading

cstrahan commented May 1, 2016

cstrahan commented May 1, 2016

joachifm commented May 1, 2016

cstrahan commented May 1, 2016

joachifm commented May 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstrahan commented May 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 May 1, 2016 • edited Loading

Choose a reason for hiding this comment

Ericson2314 commented May 1, 2016

cstrahan commented May 1, 2016

cstrahan commented May 2, 2016 • edited Loading

cstrahan commented Sep 12, 2016

domenkozar commented Sep 12, 2016

cstrahan commented Sep 26, 2016

domenkozar commented Sep 28, 2016

cstrahan commented Sep 28, 2016

domenkozar commented Sep 29, 2016

Ericson2314 commented Oct 4, 2016

domenkozar commented Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Oct 11, 2016

cstrahan commented Oct 11, 2016

Ericson2314 commented Oct 11, 2016 • edited Loading

cstrahan commented Oct 12, 2016

Choose a reason for hiding this comment

edolstra commented Jul 17, 2017

Ericson2314 commented Jul 17, 2017 • edited Loading

cstrahan commented Apr 30, 2016 •

edited

Loading

cstrahan commented Apr 30, 2016 •

edited

Loading

cstrahan commented Apr 30, 2016 •

edited

Loading

Ericson2314 May 1, 2016 •

edited

Loading

cstrahan commented May 2, 2016 •

edited

Loading

Ericson2314 commented Oct 11, 2016 •

edited

Loading

Ericson2314 commented Jul 17, 2017 •

edited

Loading