[RDY] buildLinux: Add more overrides#34351
Conversation
There was a problem hiding this comment.
, preferBuiltin ? hostPlatform.platform.kernelPreferBuiltin or false
There was a problem hiding this comment.
ok didn't know. Is the or specific to A ? B or C structure ? tried to use it in other places without success.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'd prefer this to be called buildRoot since it's called that in manual-config.nix as well.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.]
There was a problem hiding this comment.
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
force-pushed
the
kernel_wip
branch
2 times, most recently
from
b7a15b1 to
26ec476
Compare
GrahamcOfBorg
added
10.rebuild-darwin: 0
|
I rebased against master to support 4.15 and addressed the reviw. currently running nox-review. |
There was a problem hiding this comment.
Hmm. Does this now copy way too much to $dev/lib/modules/${modDirVersion}/source?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
nevermind looks like I haven't pushed the latest changes. Need to rebase with the new kernel updates on master too :'(
|
nox-review failed with but I don't think this is due to my patch. I ran with success for instance: I have rebased and pushed. I think it's ready. |
teto
changed the title
|
yep the virtualbox failures don't stem from this PR #34459 |
|
The virtualbox issues should be fixed on master (5.2.6 IIRC), let me know if that's not what you're seeing. |
|
|
teto
force-pushed
the
kernel_wip
branch
2 times, most recently
from
2fed7b9 to
2722bb7
Compare
|
I pushed the missing update (+rebase). VM boots fine. |
|
@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. |
|
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=$archThanks for the reviews/help along the way. |
|
@teto thanks! Merging to staging as there's just enough kernel-dependent packages to call it a mass rebuild. |
| 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; |
There was a problem hiding this comment.
@teto great change, I didn't even notice this before :). kernelExtraConfig isn't used anywhere, so let's definitely get rid of that!
There was a problem hiding this comment.
Do you mean moving kernelExtraConfig from platforms.* to the buildLinux calls ?
There was a problem hiding this comment.
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
| , writeTextFile, ubootTools | ||
| , callPackage | ||
| }: | ||
|
|
There was a problem hiding this comment.
I don't understand the intention here. Most of the parameters are unused, and having buildPackages (and others) twice is a bit confusing...
There was a problem hiding this comment.
That's a good point. Sorry I missed this.
There was a problem hiding this comment.
I see this PR as setting up the inlining of manual-config.nix into generic.nix. we can probably kill many callPackages with that.
There was a problem hiding this comment.
I copy pasted from manual-config.nix and forgot to trim it down. Will do in #34351 (comment) sorry
There was a problem hiding this comment.
When doing this, we might also clean up the parameters in <nixpkgs/pkgs/os-specific/linux/kernel/linux-*.nix>.
6 participants
Motivation for this change
Make linux kernel packaging easier & nix-shell work smoother, possibly as part of #34274 (comment)
Things done
$buildRootto 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.instead of
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).
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)@Ericson2314 If you agree with it, I might replace the
./import generic.nixbybuildLinuxother 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.