[Nix-dev] Feedback requested: new `boot.kernelPatches` option
zimbatm
zimbatm at zimbatm.com
Thu May 5 15:12:32 CEST 2016
Just got some random thoughts attached to what you said. makeObject looks
like a cleaner implementation of lib.makeOverridable which is used to wrap
all callPackage derivations. I haven’t thought deeply about that stuff
though.
Regarding the naming, I think in nix an “object” is a set that has a
“_type” attribute (see lib/types.nix). Except for the derivation who uses
the “type” attribute without the underscore for some reason. makeOverridable
is actually not too bad if it can supersede that function.
Regarding the kernel, instead of splitting the options around, wouldn’t it
be cool if the kernel could just be overridden and then set as an option
like this:
boot.kernel = pkgs.kernel.override {
patches = [ ./my-patch.patch ];
};
With package sets where there is a primary dependency we could also decide
to define is as pass-trough from that package itself. So instead of having
to set a kernelPackages that matches we could start using
pkgs.kernel.packages so everything is bundled together (provided the
override works as intended).
On Sun, 1 May 2016 at 21:04 Charles Strahan <charles at cstrahan.com> wrote:
> 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
> _______________________________________________
> nix-dev mailing list
> nix-dev at lists.science.uu.nl
> http://lists.science.uu.nl/mailman/listinfo/nix-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.science.uu.nl/pipermail/nix-dev/attachments/20160505/0435e264/attachment-0001.html
More information about the nix-dev
mailing list