Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

overrideAttrs on kernel derivation loses passthru values #111504

Closed
samueldr opened this issue Jan 31, 2021 · 5 comments · Fixed by #288154
Closed

overrideAttrs on kernel derivation loses passthru values #111504

samueldr opened this issue Jan 31, 2021 · 5 comments · Fixed by #288154

Comments

@samueldr
Copy link
Member

samueldr commented Jan 31, 2021

Describe the bug

There is an oddity with how the kernel derivations react when overrideAttrs is used with them. Some passthru values are lost, namely features and commonStructuredConfig.

In turn, this makes customizing them harder than expected. These values are used in the NixOS modules.

To Reproduce

Using overrideAttrs, the passthru value given to the function does not hold all values found on the derivation's own passthru.

Details:

let
  kernel = pkgs.linux_latest;
  overriddenKernel = kernel.overrideAttrs({ passthru ? {}, ... }: {
    passthru = passthru // {
      world = "y'all";
    };
  });
in
  /* ok   */ assert kernel ? features;
  /* ok   */ assert kernel ? commonStructuredConfig;
  /* fail */ assert overriddenKernel ? features;
  /* fail */ assert overriddenKernel ? commonStructuredConfig;

Expected behavior

Values in passthru on the derivation should all be present in the overrideAttrs call.

Workaround

let
  kernel = pkgs.linux_latest;
  overriddenKernel = kernel.overrideAttrs({ passthru ? {}, ... }: {
    passthru = kernel.passthru // { # <- use the derivation rather than the param
      world = "y'all";
    };
  });
in

Additional context

And where I suspect this is coming from:

  • passthru = {
    features = kernelFeatures;
    inherit commonStructuredConfig isXen isZen isHardened isLibre;
    kernelOlder = lib.versionOlder version;
    kernelAtLeast = lib.versionAtLeast version;
    passthru = kernel.passthru // (removeAttrs passthru [ "passthru" ]);
    };
    in lib.extendDerivation true passthru kernel

This is can be observed against 891f607, and is an evaluation issue. No builds happened.

The linked gist contains a full reproduction.

Maintainer information:

# a list of nixpkgs attributes affected by the problem
attribute:
# a list of nixos modules affected by the problem
module:
@infinisil
Copy link
Member

infinisil commented Jan 31, 2021

Haven't tested this properly, but

diff --git a/pkgs/os-specific/linux/kernel/generic.nix b/pkgs/os-specific/linux/kernel/generic.nix
index dd3050a93ee..a90245b8a91 100644
--- a/pkgs/os-specific/linux/kernel/generic.nix
+++ b/pkgs/os-specific/linux/kernel/generic.nix
@@ -186,4 +186,6 @@ let
     passthru = kernel.passthru // (removeAttrs passthru [ "passthru" ]);
   };
 
-in lib.extendDerivation true passthru kernel
+in kernel.overrideAttrs (old: {
+  passthru = old.passthru // passthru;
+})

works

We wouldn't have these kinds of problems if some generic override interface like NixOS/rfcs#67 (comment) existed

@infinisil
Copy link
Member

Short explanation: extendDerivation updates the derivation without updating all the override functions. So .overrideAttrs still points to the one from kernel. So once you call that, all the changes from extendDerivation are undone.

samueldr added a commit to eamsden/mobile-nixos that referenced this issue Feb 1, 2021
See NixOS/nixpkgs#111504 for the reason behind
this workaround.
samueldr added a commit to eamsden/mobile-nixos that referenced this issue Feb 1, 2021
See NixOS/nixpkgs#111504 for the reason behind
this workaround.
@kira-bruneau
Copy link
Contributor

Wow what a coincidence. I ran into this same problem today trying to adapt @thefloweringash's port of lg-hammerhead to work on Mobile NixOS's master 😄 (specifically with the kernelWithDTB wrapper).

samueldr added a commit to samueldr-wip/mobile-nixos-wip that referenced this issue Feb 2, 2021
See NixOS/nixpkgs#111504 for the reason behind
this workaround.
samueldr added a commit to eamsden/mobile-nixos that referenced this issue Feb 2, 2021
See NixOS/nixpkgs#111504 for the reason behind
this workaround.
archseer pushed a commit to archseer/mobile-nixos that referenced this issue Feb 3, 2021
See NixOS/nixpkgs#111504 for the reason behind
this workaround.
@stale
Copy link

stale bot commented Aug 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 4, 2021
infinisil added a commit to infinisil/nixpkgs that referenced this issue Dec 7, 2021
This was reported in NixOS#111504 but
further complicated in NixOS#121518

This should clean it up
@infinisil infinisil mentioned this issue Dec 7, 2021
13 tasks
@ShamrockLee
Copy link
Contributor

I made a new PR (#288154) to tackle this issue. Please take a look.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
@samueldr @kira-bruneau @infinisil @ShamrockLee and others