[RDY] buildLinux: Add more overrides #34351

teto · 2018-01-28T10:26:40Z

Motivation for this change

Make linux kernel packaging easier & nix-shell work smoother, possibly as part of #34274 (comment)

adds possibility to override fields that currently belong to the hostSystem part (namely autoModules and preferBuiltin) . Otherwise one has to run this kind of commands [RDY] kernel doc: explain how to configure custom kernel #29944 (comment). Hopefully this can also allow to Remove "argsOverride" from linux kernel Remove "argsOverride" from linux kernel #32014 and also address There should be an easy method to add extra kernel config in your configuration.nix file. #20187
make it easier to work in a nix-shell, to prevent [wip] kernel: attempt to rerun unpackPhase if sthg fishy (e.g. buildRoot unset) #31463

Things done

Added error message in generate-config.pl
moved the $buildRoot to within the source folder, this way it doesn't have to be created before the unpackPhase and make it easier to work on kernel source without running the unpackPhase.
defined buildLinux as generic.nix instead of manual-config.nix . This way one can run

buildLinux (rec {
  mptcpVersion = "0.93";
  modDirVersion = "4.9.60";
  version = "${modDirVersion}-mptcp_v${mptcpVersion}";
  # autoModules= true;

instead of

import ./generic.nix (rec {
  mptcpVersion = "0.93";
  modDirVersion = "4.9.60";

which looks more userfriendly (more similar to the rest of nixpkgs, e.g.,buildPythonPackage, buildLua). If allows to merge at a later date the generic.nix and manual-config.nix to simplify the derivation (#2296).

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
Fits CONTRIBUTING.md.

@Ericson2314 If you agree with it, I might replace the ./import generic.nix by buildLinux other than mptcp. I added ncurses as a dependancy because I hate not being able to edit my kernel .config manually when in my shell. I could remove it or make it optional.

dezgeg · 2018-01-29T13:27:36Z

pkgs/os-specific/linux/kernel/generic.nix

+
+# easy overrides to hostPlatform.platform members
+, autoModules ? hostPlatform.platform.kernelAutoModules
+, preferBuiltin ? if (hostPlatform.platform ? kernelPreferBuiltin) then hostPlatform.platform.kernelPreferBuiltin else false


, preferBuiltin ? hostPlatform.platform.kernelPreferBuiltin or false

ok didn't know. Is the or specific to A ? B or C structure ? tried to use it in other places without success.

It's related to the attribute set syntax: X.Y or Z is basically the same as if X ? Y then X.Y else Z.

dezgeg · 2018-01-29T13:29:40Z

pkgs/os-specific/linux/kernel/generate-config.pl

 my $debug = $ENV{'DEBUG'};
 my $autoModules = $ENV{'AUTO_MODULES'};
 my $preferBuiltin = $ENV{'PREFER_BUILTIN'};
-
+my $ignoreConfigErrors = $ENV{'ignoreConfigErrors'};
+my $buildFolder=$ENV{'BUILD_FOLDER'};


Missing spaces around =

I'd prefer this to be called buildRoot since it's called that in manual-config.nix as well.

Ericson2314 · 2018-01-29T16:18:26Z

pkgs/os-specific/linux/kernel/generic.nix

@@ -27,8 +35,13 @@
                       hostPlatform != stdenv.buildPlatform
 , extraMeta ? {}
 , hostPlatform
+
+# easy overrides to hostPlatform.platform members
+, autoModules ? hostPlatform.platform.kernelAutoModules


@teto So I have a feeling I'm on the verge of spreading myself too thin, so I'm going to leave reviewing the actual kernel building code to @dezgeg and others.

Happy to comment on the general principle of config though. So if hostPlatform.platform.kernelAutoModules, etc, are used in multiple packages, we should probably keep them as and not allow per-package overrides. But in most (all?) of these cases, that's not the case. Then there's little point of keeping anything in *Platform at all.

So, if @dezgeg is OK with it, I'd be happy just making this autoModules ? hostPlatform.isx86 is something. Or maybe there is a multiple-package-principle here of precise vs multiplatform configurations so we could have autoModules ? !hostPlatform.preciseConfig. Again, coordinate with @dezgeg and kernel people who are more up to speed on the specifics of this config me, but I just wanted to articulate the general principle of changes like this as I see it.

I am not sure what motivates setting the autoModules to true, is it x86 only ?
I would favor changing this (if need be) in a follow up PR just to be safe as this one is already kinda touchy.

The "multiplatform" examples in lib.systems.examples also do this, so I gather it boils down to trying to create a one-size-fits-all kernel vs tailor it to an exact machine.

Well, it's not so clear-cut; it also affects whether you get a kernel that has support for all the possible WiFi dongles, PCI cards, etc.

I'd say keep the platform.kernelAutoModules for now since indeed this is a large patch already.

Well conversely, could we just not add the extra parameters then? It looks like this PR does other things which are good/useful on their own.

The initial motivation for the PR was to override these so I would hate it to see it go xD
I do a bit of kernel development and having to compile every single module is really a pain. This override cuts down compilation time a lot.

@teto I'm not saying never do that :), but just do it in a second PR. I like the separation of concerns to better understand each is isolation, and (long term) I don't believe we should have override options for anything that is also in *Platform. [I also am confused why this would help with compilation times, but that is a separate matter.]

having kernelAutoModules set to true mean sthat nix builds ton of modules I don't use, (GPU drivers etc) because it answers M to the interactive kernel config generator. Before your indications and even with it, I still find it hard to override the platform.kernelAutoModules value, having the override makes it more nixpkgs-like. It's no problem having it out-of-tree, it should generate less rebase conflicts as it's much smaller patch.

teto · 2018-01-30T03:40:11Z

I rebased against master to support 4.15 and addressed the reviw. currently running nox-review.

dezgeg · 2018-01-31T10:58:46Z

pkgs/os-specific/linux/kernel/manual-config.nix

@@ -151,7 +171,7 @@ let
        unlink $out/lib/modules/${modDirVersion}/source

        mkdir -p $dev/lib/modules/${modDirVersion}/build
-        cp -dpR ../$sourceRoot $dev/lib/modules/${modDirVersion}/source
+        cp -dpR .. $dev/lib/modules/${modDirVersion}/source


Hmm. Does this now copy way too much to $dev/lib/modules/${modDirVersion}/source?

This is a problem I realized while testing with nox-review, I moved the build folder to within source (aka someFolder/source/build) so that it looks more like other packages (cmake-based etc). It used to have this architecture:
someFolder
|--build
|--source
The change just accounts for this folder move.

To sum up, it should be exactly the same as before. my nox-review has been running for a day but it still hasn't met any error so I without further comments I consider the PR ready.

nevermind looks like I haven't pushed the latest changes. Need to rebase with the new kernel updates on master too :'(

teto · 2018-02-01T04:02:10Z

nox-review failed with

             ^~~~~~~~~~~~~~~~~
/tmp/nix-build-VirtualBox-GuestAdditions-5.2.4-4.15.drv-0/install/src/vboxguest-5.2.4/vboxguest/r0drv/linux/timer-r0drv-linux.c:1592:52: error: 'struct timer_list' has no member named 'data'

but I don't think this is due to my patch. I ran with success for instance:
nix-build -A linuxPackages_4_9.virtualboxGuestAdditions ~/nixpkgs --show-trace

I have rebased and pushed. I think it's ready.

teto · 2018-02-02T06:15:06Z

yep the virtualbox failures don't stem from this PR #34459

dtzWill · 2018-02-02T08:21:33Z

The virtualbox issues should be fixed on master (5.2.6 IIRC), let me know if that's not what you're seeing.

teto · 2018-02-02T09:30:01Z

nix-build -A linuxPackages_4_15.virtualboxGuestAdditions fails on master (but virtualbox succeeds)

=== Building 'vboxvideo' module ===
make[1]: Entering directory '/tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo'
make V= CONFIG_MODULE_SIG= -C /nix/store/ganihdijy50pgqxpqmlwlk9alikv9nf0-linux-4.15-dev/lib/modules/4.15.0/build SUBDIRS=/tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo SRCROOT=/tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo -j4 modules
make[2]: Entering directory '/nix/store/ganihdijy50pgqxpqmlwlk9alikv9nf0-linux-4.15-dev/lib/modules/4.15.0/build'
  CC [M]  /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/hgsmi_base.o
  CC [M]  /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/modesetting.o
  CC [M]  /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_drv.o
  CC [M]  /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_fb.o
  CC [M]  /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_irq.o
  CC [M]  /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_main.o
  CC [M]  /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_mode.o
  CC [M]  /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_ttm.o
/tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_mode.c: In function 'vbox_best_single_encoder':
/tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_mode.c:400:43: warning: passing argument 2 of 'drm_encoder_find' makes pointer from integer without a cast [-Wint-conversion]
   return drm_encoder_find(connector->dev, enc_id);
                                           ^~~~~~
In file included from /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_drv.h:62:0,
                 from /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_mode.c:35:
/nix/store/ganihdijy50pgqxpqmlwlk9alikv9nf0-linux-4.15-dev/lib/modules/4.15.0/source/include/drm/drm_encoder.h:217:35: note: expected 'struct drm_file *' but argument is of type 'int'
 static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
                                   ^~~~~~~~~~~~~~~~
/tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_mode.c:400:10: error: too few arguments to function 'drm_encoder_find'
   return drm_encoder_find(connector->dev, enc_id);
          ^~~~~~~~~~~~~~~~
In file included from /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_drv.h:62:0,
                 from /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_mode.c:35:
/nix/store/ganihdijy50pgqxpqmlwlk9alikv9nf0-linux-4.15-dev/lib/modules/4.15.0/source/include/drm/drm_encoder.h:217:35: note: declared here
 static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
                                   ^~~~~~~~~~~~~~~~
make[5]: *** [/nix/store/ganihdijy50pgqxpqmlwlk9alikv9nf0-linux-4.15-dev/lib/modules/4.15.0/source/scripts/Makefile.build:316: /tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/vbox_mode.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [/nix/store/ganihdijy50pgqxpqmlwlk9alikv9nf0-linux-4.15-dev/lib/modules/4.15.0/source/Makefile:1508: _module_/tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo] Error 2
make[3]: *** [Makefile:146: sub-make] Error 2
make[2]: *** [Makefile:24: __sub-make] Error 2
make[2]: Leaving directory '/nix/store/ganihdijy50pgqxpqmlwlk9alikv9nf0-linux-4.15-dev/lib/modules/4.15.0/build'
make[1]: *** [/tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo/Makefile.include.footer:101: vboxvideo] Error 2
make[1]: Leaving directory '/tmp/nix-build-VirtualBox-GuestAdditions-5.2.6-4.15.drv-0/install/src/vboxguest-5.2.6/vboxvideo'
make: *** [Makefile:34: all] Error 1
builder for ‘/nix/store/wl28l39s94lfbjqw851kqs5ar5aw1m9h-VirtualBox-GuestAdditions-5.2.6-4.15.drv’ failed with exit code 2

teto · 2018-02-06T07:37:16Z

I pushed the missing update (+rebase). VM boots fine.

Ericson2314 · 2018-02-06T15:47:57Z

@teto Assuming @dezgeg approves, which seems to be the case (?), if you can temporarily remove the new overrides as per what I said in #34351 (comment), I'll go ahead and merge this. Then you can make another one with just the overrides, and I'll do my best to ensure that goes faster than this. Thanks for your work.

teto · 2018-02-06T17:04:22Z

I removed the overrides as requested and updated commit message. Here is the diff with the previously pushed version

diff --git a/pkgs/os-specific/linux/kernel/generic.nix b/pkgs/os-specific/linux/kernel/generic.nix
index 852f9cdf48a..5a5081e5efb 100644
--- a/pkgs/os-specific/linux/kernel/generic.nix
+++ b/pkgs/os-specific/linux/kernel/generic.nix
@@ -35,11 +35,6 @@
                        hostPlatform != stdenv.buildPlatform
 , extraMeta ? {}
 , hostPlatform
-
-# easy overrides to hostPlatform.platform members
-, autoModules ? hostPlatform.platform.kernelAutoModules
-, preferBuiltin ? hostPlatform.platform.kernelPreferBuiltin or false
-
 , ...
 } @ args:
 
@@ -72,8 +67,7 @@ let
     in lib.concatStringsSep "\n" ([baseConfig] ++ configFromPatches);
 
   configfile = stdenv.mkDerivation {
-    inherit ignoreConfigErrors autoModules preferBuiltin;
-
+    inherit ignoreConfigErrors;
     name = "linux-config-${version}";
 
     generateConfig = ./generate-config.pl;
@@ -88,6 +82,8 @@ let
     kernelBaseConfig = hostPlatform.platform.kernelBaseConfig;
     # e.g. "bzImage"
     kernelTarget = hostPlatform.platform.kernelTarget;
+    autoModules = hostPlatform.platform.kernelAutoModules;
+    preferBuiltin = hostPlatform.platform.kernelPreferBuiltin or false;
     arch = hostPlatform.platform.kernelArch;
 
     prePatch = kernel.prePatch + ''
@@ -100,7 +96,6 @@ let
 
     buildPhase = ''
       export buildRoot="''${buildRoot:-build}"
-      echo "config buildPhase buildRoot=$buildRoot pwd=$PWD"
 
       # Get a basic config file for later refinement with $generateConfig.
       make HOSTCC=${buildPackages.stdenv.cc.targetPrefix}gcc -C . O="$buildRoot" $kernelBaseConfig ARCH=$arch

Thanks for the reviews/help along the way.

Ericson2314 · 2018-02-06T17:08:08Z

@teto thanks! Merging to staging as there's just enough kernel-dependent packages to call it a mass rebuild.

Ericson2314 · 2018-02-06T17:14:08Z

pkgs/os-specific/linux/kernel/generic.nix

-    kernelPlatform = hostPlatform;
+    inherit stdenv version ;
+    # append extraConfig for backwards compatibility but also means the user can't override the kernelExtraConfig part
+    extraConfig = extraConfig + lib.optionalString (hostPlatform ? kernelExtraConfig ) hostPlatform.kernelExtraConfig;


@teto great change, I didn't even notice this before :). kernelExtraConfig isn't used anywhere, so let's definitely get rid of that!

Do you mean moving kernelExtraConfig from platforms.* to the buildLinux calls ?

Oh wait, my grep was bad, nevermind.

- defined buildLinux as generic.nix instead of manual-config.nix. This makes kernel derivations a tad more similar to your typical derivations. - moved $buildRoot to within the source folder, this way it doesn't have to be created before the unpackPhase and make it easier to work on kernel source without running the unpackPhase

vcunat · 2018-02-10T09:28:58Z

pkgs/os-specific/linux/kernel/generic.nix

+, writeTextFile, ubootTools
+, callPackage
+}:
+


I don't understand the intention here. Most of the parameters are unused, and having buildPackages (and others) twice is a bit confusing...

That's a good point. Sorry I missed this.

I see this PR as setting up the inlining of manual-config.nix into generic.nix. we can probably kill many callPackages with that.

I copy pasted from manual-config.nix and forgot to trim it down. Will do in #34351 (comment) sorry

When doing this, we might also clean up the parameters in <nixpkgs/pkgs/os-specific/linux/kernel/linux-*.nix>.

Ericson2314 requested a review from dezgeg January 29, 2018 05:52

dezgeg reviewed Jan 29, 2018

View reviewed changes

Ericson2314 reviewed Jan 29, 2018

View reviewed changes

Ericson2314 mentioned this pull request Jan 29, 2018

Remove cruft from platform descriptions #34274

Open

6 tasks

teto force-pushed the kernel_wip branch 2 times, most recently from b7a15b1 to 26ec476 Compare January 30, 2018 03:26

GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 501+ labels Jan 30, 2018

teto force-pushed the kernel_wip branch from 26ec476 to da62a70 Compare January 31, 2018 05:57

dezgeg reviewed Jan 31, 2018

View reviewed changes

teto force-pushed the kernel_wip branch from da62a70 to 918518a Compare February 1, 2018 03:08

teto changed the title ~~[WIP] buildLinux: Add more overrides~~ [RDY] buildLinux: Add more overrides Feb 1, 2018

dtzWill mentioned this pull request Feb 2, 2018

4.15 breakage tracking #34459

Closed

8 tasks

teto force-pushed the kernel_wip branch 2 times, most recently from 2fed7b9 to 2722bb7 Compare February 5, 2018 06:55

teto force-pushed the kernel_wip branch from 2722bb7 to f620b1b Compare February 6, 2018 16:47

Ericson2314 changed the base branch from master to staging February 6, 2018 17:07

Ericson2314 merged commit 63b8175 into NixOS:staging Feb 6, 2018

Ericson2314 reviewed Feb 6, 2018

View reviewed changes

teto mentioned this pull request Feb 6, 2018

buildLinux: add overrides for modules #34672

Merged

8 tasks

teto deleted the kernel_wip branch February 9, 2018 03:57

vcunat reviewed Feb 10, 2018

View reviewed changes

thefloweringash mentioned this pull request Feb 18, 2018

Restore linuxManualConfig as linux/kernel/manual-config.nix #35094

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RDY] buildLinux: Add more overrides #34351

[RDY] buildLinux: Add more overrides #34351

teto commented Jan 28, 2018 •

edited

Loading

dezgeg Jan 29, 2018

teto Jan 30, 2018

dezgeg Jan 31, 2018

dezgeg Jan 29, 2018

dezgeg Jan 29, 2018

teto Jan 30, 2018

Ericson2314 Jan 29, 2018

teto Jan 30, 2018 •

edited

Loading

Ericson2314 Jan 30, 2018

dezgeg Jan 31, 2018

Ericson2314 Feb 1, 2018

teto Feb 2, 2018

Ericson2314 Feb 6, 2018

teto Feb 6, 2018

teto commented Jan 30, 2018

dezgeg Jan 31, 2018

teto Jan 31, 2018 •

edited

Loading

teto Jan 31, 2018

teto commented Feb 1, 2018

teto commented Feb 2, 2018

dtzWill commented Feb 2, 2018

teto commented Feb 2, 2018

teto commented Feb 6, 2018

Ericson2314 commented Feb 6, 2018 •

edited

Loading

teto commented Feb 6, 2018 •

edited by Ericson2314

Loading

Ericson2314 commented Feb 6, 2018

Ericson2314 Feb 6, 2018

teto Feb 6, 2018

Ericson2314 Feb 6, 2018

vcunat Feb 10, 2018 •

edited

Loading

Ericson2314 Feb 11, 2018

Ericson2314 Feb 11, 2018

teto Feb 11, 2018

vcunat Feb 11, 2018 •

edited

Loading

[RDY] buildLinux: Add more overrides #34351

[RDY] buildLinux: Add more overrides #34351

Conversation

teto commented Jan 28, 2018 • edited Loading

Motivation for this change

Things done

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teto Jan 30, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teto commented Jan 30, 2018

Choose a reason for hiding this comment

teto Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teto commented Feb 1, 2018

teto commented Feb 2, 2018

dtzWill commented Feb 2, 2018

teto commented Feb 2, 2018

teto commented Feb 6, 2018

Ericson2314 commented Feb 6, 2018 • edited Loading

teto commented Feb 6, 2018 • edited by Ericson2314 Loading

Ericson2314 commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcunat Feb 10, 2018 • edited Loading

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

vcunat Feb 11, 2018 • edited Loading

Choose a reason for hiding this comment

teto commented Jan 28, 2018 •

edited

Loading

teto Jan 30, 2018 •

edited

Loading

teto Jan 31, 2018 •

edited

Loading

Ericson2314 commented Feb 6, 2018 •

edited

Loading

teto commented Feb 6, 2018 •

edited by Ericson2314

Loading

vcunat Feb 10, 2018 •

edited

Loading

vcunat Feb 11, 2018 •

edited

Loading