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

[RFC] add ability to merge structured configs #42838

Merged
merged 4 commits into from Jan 28, 2019

Conversation

teto
Copy link
Member

@teto teto commented Jul 1, 2018

POC

Motivation for this change

Listed here #41103.
To sum up, I would like programs that require kernel modifications to be able to specify their kernel requirements. This might be disabled etc. but here is a proposition to show how it could look like

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

follow up of #41393

@teto
Copy link
Member Author

teto commented Jul 5, 2018

@dezgeg thanks for reviewing/merging the structured config PR \o/

Being able to specify required config for packages was one of the motivation. I would love some feedback on how to best achieve this since the current way is not practical aka:

openvswitch = stdenv.mkDerivation {
....
  passthru.kernelExtraConfig = with import ../../../../lib/kernel.nix { inherit (kernel) version; lib = stdenv.lib; }; {
    OPENVSWITCH  = whenAtLeast "2.0" yes;
  };
}

I moved the version dependant filters whenAtLeast etc.. out of lib/kernel.nix so that lib/kernel.nix can be imported by lib/default.nix. This replaces with import ../../../../lib/kernel.nix { inherit (kernel) version; lib = stdenv.lib; }; with with import lib.kernel; but I am still stuck with
passthru.kernelExtraConfig = { OPENVSWITCH = yes; }

The problem is that if we just pass it like this

  my_lenovo_kernel = prev.linux_latest.override({
    structuredExtraConfig = prev.pkgs.openvswitch.passthru.kernelExtraConfig;
})

it erases conditions (e.g., whenAtLeast ) in common-config.nix such as
OPENVSWITCH = optional whenAtLeast "3.4" yes;

I wonder if it wouldn't be better to have packages define a list of config items like passthru.kernelExtraConfig = [ OPENVSWITCH ];and have the kernel config logic enable these the common-config.nix (strip the optional for instance) ?

To sum up, the best interface would be IMO to have in common-config.nix
OPENVSWITCH = optional whenAtLeast "3.4" no;
and find some way to turn this into OPENVSWITCH = whenAtLeast "3.4" yes; when building my_lenovo_kernel (cf previous example).

@teto
Copy link
Member Author

teto commented Jul 14, 2018

I am experimenting a bit as I am trying to solve the initial issue. Without types, the code is hard to follow but what I've come up with is something similar to the module system so that hopefully merging
{ DEBUG = option yes; } and { DEBUG = yes; } returns sthg like { DEBUG = option yes; }.
So for instance yes is defined as yes = { answer = "y"; }. My fear is that I am recreating the module system so any hindsight would be nice.

lib/debug.nix Outdated
@@ -77,7 +77,7 @@ rec {
(modify depth snip x)) y;

/* A combination of `traceVal` and `traceSeq` */
traceValSeqFn = f: v: traceVal f (builtins.deepSeq v v);
traceValSeqFn = f: v: traceValFn f (builtins.deepSeq v v);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I thought we merged that line already?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not up to date. I've started modifying the code to adopt somewhat the module backend so it might be best not to review it now

@teto teto force-pushed the kernel_autoconf branch 3 times, most recently from c16fd9e to 609ed28 Compare August 1, 2018 03:27
@teto teto changed the title [WIP] openvswitch: expose required kernel config [WIP] expose required kernel config Aug 1, 2018
@teto teto changed the title [WIP] expose required kernel config [WIP] add ability to merge structured configs Aug 1, 2018
@teto
Copy link
Member Author

teto commented Aug 1, 2018

Thanks to the advice on https://discourse.nixos.org/t/use-lib-types-system-to-merge-attrsets-without-the-module-system/534/10, I managed to use the module system to merge structured configs. I renamed the pull request so that it focuses on this aspect only. It seems to work for basic things (linux_latest) but I am sure I've broken things in other places.

I tried to preserve the current api (option no; whenOlder "4.3" yes etc) but there are a few difficulties. Each kernel option is now a kernelItem submodule of the form https://github.com/NixOS/nixpkgs/pull/42838/files#diff-810dcf1e9edefca0d59fd0b90d11242bR30 .

One problem is for the "freeform" options like currently
MMC_BLOCK_MINORS = "32";
I could preprocess (slow) the structured config to convert the "32" into a submodule { answer = "32";}
so I preferred to write instead MMC_BLOCK_MINORS = freeform "32"; # equivalent to { answer = "32"} but I wonder if MMC_BLOCK_MINORS.answer = "32"; is better ?
It might be best (to allow for further checking) to distinguish between freeform answers (type = types.str) and tristate values (types.enum [ "y" "n" "m"])

I've converted the items of the form
B43_PCMCIA = option (whenOlder "4.4" yes);
to
B43_PCMCIA = whenOlder "4.4" (option yes);
with the whenOlder now being a mkIf. I suspect this can be a problem for some kernels. I will need to tackle that too.

@teto
Copy link
Member Author

teto commented Aug 10, 2018

I went ahead and proceeded with the modifications:

  • extraStructuredConfig now expects a list of attributes. It makes the composition of the kernel config easier.
  • I've proceeded with distinguishing freeform options from tristate ones
  • will look for a structured config in kernelPatches too
  • one can now access the structuredConfig from a kernel via linux_test.configfile.structuredConfig in order to reinject it another kernel, no need to rewrite the config from scratch
  • I've defined the following merge strategies in case of conflict:
    -- freeform items must be equal or they conflict (mergeEqualOption)
    -- for tristate (y/m/n) entries, I use the mergeAnswer strategy which takes the best available value, "best" being defined by the user (by default "y" > "m" > "n", e.g. if one entry is both marked "y" and "n", "y" wins)
    -- if one item is both marked optional/mandatory, mandatory wins (mergeFalseByDefault)

Here is what I test with:

    linux_test = let 
      configStructured = with prev.lib.kernel; [ 


        # common-config.nix default is 32 

        { MMC_BLOCK_MINORS   = freeform "32"; } # same as default, won't trigger any error 
        # { MMC_BLOCK_MINORS   = freeform "64"; } # will trigger an error but the message is not great:
        # The option `settings.MMC_BLOCK_MINORS.freeform' has conflicting definitions, in `<unknown-file>' and `<unknown-file>'

        # mandatory should win by default
        { USB_DEBUG = option yes;}
        # { USB_DEBUG = yes;}

        # default for "8139TOO_PIO" is no
        { "8139TOO_PIO"  = yes; } # yes wins over no by default
      ];
    in
    prev.linux_latest.override {
      kernelPreferBuiltin = true;
      structuredExtraConfig = configStructured;
  };

You can then have a look at the generated config via
nix-build -A linux_latest.configfile ~/nixpkgs3 or look in nix repl for linux_test.configfile.structuredConfig

I am sure the code may be improved and there are some cases that might break. I would like to write some tests but not sure how to do.

@teto teto changed the title [WIP] add ability to merge structured configs [RFC] add ability to merge structured configs Aug 10, 2018
@edolstra
Copy link
Member

Can lib/kernel.nix be moved out of lib? lib really shouldn't contain package-specific functions.

@teto
Copy link
Member Author

teto commented Jan 25, 2019

I did well to check several mistakes had crept in.
The diff between hardened kernels now returns nothing \o/ (I just checked for linux_hardened).
The lib/kernels.nix isn't made public anymore and is imported just by the files needing it.
To sum it up, the PR exposes nothing new (structuredExtraConfig is already available in nixos-unstable), it just adds an internal mechanism to merge kernel configurations.
Once this PR is merged, we need to think about what and how to expose kernel configuration to packages / user (boot.kernelConfiguration etc, might warrant an RFC).

@infinisil
Copy link
Member

@teto Merge conflict

This should make the composability of kernel configurations more straigthforward.

- now distinguish freeform options from tristate ones
- will look for a structured config in kernelPatches too
one can now access the structuredConfig from a kernel via linux_test.configfile.structuredConfig
in order to reinject it into another kernel, no need to rewrite the config from scratch

The following merge strategies are used in case of conflict:
-- freeform items must be equal or they conflict (mergeEqualOption)
-- for tristate (y/m/n) entries, I use the mergeAnswer strategy which takes the best available value, "best" being defined by the user (by default "y" > "m" > "n", e.g. if one entry is both marked "y" and "n", "y" wins)
-- if one item is both marked optional/mandatory, mandatory wins (mergeFalseByDefault)
@teto
Copy link
Member Author

teto commented Jan 28, 2019

@infinisil ty, pushed an update.

@infinisil
Copy link
Member

Evaluation fails now, messed up in all-packages.nix

@dtzWill
Copy link
Member

dtzWill commented Jan 28, 2019 via email

@teto
Copy link
Member Author

teto commented Jan 28, 2019

@infinisil sorry, the lights were green after my last push, I guess some caching problem on the browser. The timeout for the first check seems to appear once the config was generated so I think it's fine.

@dtzWill very excited about this too. The current achievement is the possibility to do extraStructuredConfig=mkMerge [ ] and be warned in case of contradicting configurations. Yet it allows for some exciting future possibilities.

@infinisil infinisil merged commit 51d2eed into NixOS:master Jan 28, 2019
@teto teto deleted the kernel_autoconf branch January 28, 2019 09:39
@lopsided98
Copy link
Contributor

I think this broke the legacy extraConfig option, which is used by linux_hardkernel_4_14. The build is failing because a config option is not being applied. That package should probably be switched to structured config, but I don't think this PR intended to break string based config.

@teto
Copy link
Member Author

teto commented Jan 30, 2019

you are right, it shouldn't sorry for that, let me look into this.

@teto
Copy link
Member Author

teto commented Jan 30, 2019

@lopsided98 what kind of error do you have ? I have

make: Entering directory '/build/source'
make[1]: Entering directory '/build/source/build'
  HOSTCC  scripts/basic/fixdep
  GEN     ./Makefile
  HOSTCC  scripts/kconfig/conf.o
  SHIPPED scripts/kconfig/zconf.tab.c
  SHIPPED scripts/kconfig/zconf.lex.c
  HOSTCC  scripts/kconfig/zconf.tab.o
  HOSTLD  scripts/kconfig/conf
***
*** Can't find default configuration "arch/x86/configs/odroidxu4_defconfig"!
***

which shouldn't be influenceed by my changes. Also I can reproduce this error on nixos-unstable so it must be some other change
nix build -f channel:nixos-unstable linux_hardkernel_4_14

@lopsided98
Copy link
Contributor

Your error is caused by building it on x86; odroidxu4_defconfig only works on ARM. You should be able to cross compile it with nix build -f channel:nixos-unstable pkgsCross.armv7l-hf-multiplatform.linux_hardkernel_4_14.

My error is:

<3>  CC [M]  drivers/gator/gator_events_mali_midgard.o
<3>../drivers/gator/gator_events_mali_midgard.c:25:10: fatal error: linux/mali_linux_trace.h: No such file or directory
<3> #include "linux/mali_linux_trace.h"
<3>          ^~~~~~~~~~~~~~~~~~~~~~~~~~
<3>compilation terminated.
<3>make[4]: *** [../scripts/Makefile.build:326: drivers/gator/gator_events_mali_midgard.o] Error 1
<3>make[3]: *** [../scripts/Makefile.build:585: drivers/gator] Error 2
<3>make[2]: *** [/build/source/Makefile:1044: drivers] Error 2
<3>make[2]: *** Waiting for unfinished jobs....

This error was fixed in 5ac7334 by adding GATOR n to extraConfig, so the return of this error implies that extraConfig is no longer being applied.

@teto
Copy link
Member Author

teto commented Jan 30, 2019

seems like you are right, extraConfig is not taken into account.It's very late here so I can try tomorrow, until then does

diff --git a/pkgs/os-specific/linux/kernel/generic.nix b/pkgs/os-specific/linux/kernel/generic.nix
index a41f1eb989b..a83fbc89324 100644
--- a/pkgs/os-specific/linux/kernel/generic.nix
+++ b/pkgs/os-specific/linux/kernel/generic.nix
@@ -74,9 +74,9 @@ let
   };
 
   # extra config in legacy string format
-  extraConfig = extraConfig + lib.optionalString (stdenv.hostPlatform.platform ? kernelExtraConfig) stdenv.hostPlatform.platform.kernelExtraConfig;
+  extraStrConfig = extraConfig + lib.optionalString (stdenv.hostPlatform.platform ? kernelExtraConfig) stdenv.hostPlatform.platform.kernelExtraConfig;
 
-  intermediateNixConfig = configfile.moduleStructuredConfig.intermediateNixConfig;
+  intermediateNixConfig = configfile.moduleStructuredConfig.intermediateNixConfig + extraStrConfig;
 
   structuredConfigFromPatches =
         map ({extraStructuredConfig ? {}, ...}: {settings=extraStructuredConfig;}) kernelPatches;

solve it ?

@lopsided98
Copy link
Contributor

error: while evaluating the attribute 'drvPath' at /home/ben/Documents/Projects/nixpkgs/lib/customisation.nix:149:7:
while evaluating the attribute 'configurePhase' of the derivation 'linux-4.14.87-153-armv7l-unknown-linux-gnueabihf' at /home/ben/Documents/Projects/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:183:11:
while evaluating the attribute 'kernelConfig' of the derivation 'linux-config-4.14.87-153-armv7l-unknown-linux-gnueabihf' at /home/ben/Documents/Projects/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:183:11:
while evaluating 'kernelConfigFun' at /home/ben/Documents/Projects/nixpkgs/pkgs/os-specific/linux/kernel/generic.nix:85:21, called from /home/ben/Documents/Projects/nixpkgs/pkgs/os-specific/linux/kernel/generic.nix:97:20:
infinite recursion encountered, at /home/ben/Documents/Projects/nixpkgs/pkgs/os-specific/linux/kernel/generic.nix:77:17

@teto
Copy link
Member Author

teto commented Jan 30, 2019

(I've updated the snippet).

@lopsided98
Copy link
Contributor

That patch fixes the issue.

teto added a commit to teto/nixpkgs that referenced this pull request Jan 31, 2019
PR NixOS#42838 wrongly started to ignore extraConfig. This fixes that.
@teto
Copy link
Member Author

teto commented Feb 10, 2019

Elco didn t want this to leak. Instead of using the alias 'no' you can also pass the submodule directly: SND={answer="n";}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet