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

buildLinux: add overrides for modules #34672

Merged
merged 2 commits into from Mar 8, 2018
Merged

Conversation

teto
Copy link
Member

@teto teto commented Feb 6, 2018

Follow up PR of #34351

  • Easy override of autoModules and preferBuiltin parameters (currently living in hostSystem set).
Motivation for this change

Make it easier to configure the kernel (overriding platforms is 'hard').
One question in the previous PR was should we delete kernelAutoModules/kernelPreferBuiltin from the hostPlatform.platform.* ?
Some platforms may prefer to have builtin modules because crosscompiling is faster and having to install modules separataly would be a pain so it might make sense. Not too sure though.

Things done
  • 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.

@teto teto requested a review from FRidh as a code owner February 8, 2018 13:08
@teto teto changed the base branch from staging to master February 8, 2018 13:11
@teto
Copy link
Member Author

teto commented Feb 8, 2018

sry @FRidh you can ignore the request, the PR was made against staging and my rebase against master, the push triggered the request.

@FRidh FRidh removed their request for review February 8, 2018 13:12
@teto
Copy link
Member Author

teto commented Feb 8, 2018

@Ericson2314 @dezgeg follow up of the previous kernel patch. Seems like changing the base branch against which to evaluate the PR confused the bot.

}:

{ stdenv, buildPackages, perl, buildLinux
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the perl into the other set because I don't think people override it and if so it makes more sense in the callPackage call.

@teto
Copy link
Member Author

teto commented Feb 14, 2018

@vcunat I tried to fix the useless parameters you mentioned in my previous PR. I hope it's good now.

@Ericson2314
Copy link
Member

But do we even need to arguments like that? Just have buildLinux import instead of callPackageing generic.nix

@teto
Copy link
Member Author

teto commented Feb 19, 2018

What's the gain of
buildLinux = makeOverridable (import ../os-specific/linux/kernel/generic.nix { inherit perl bison ... });
vs
buildLinux = makeOverridable (callPackage ../os-specific/linux/kernel/generic.nix { });
I tried to rebase but it adds other dependencies so I would like to understand why I should convert it to an import (or if you want to do it, please feel free).

@Ericson2314
Copy link
Member

@teto having two sets of imports with a fairly arbitrary division of labor strikes me as a) confusing and b) limiting customizability by making certain inputs harder to optimize.

@teto teto force-pushed the kernel_overrides branch 2 times, most recently from 0377171 to 02c34f2 Compare March 5, 2018 06:24
@teto
Copy link
Member Author

teto commented Mar 5, 2018

I tried to merge the 2 sets as requested but then

 buildLinux = makeOverridable (import ../os-specific/linux/kernel/generic.nix {
    inherit stdenv buildPackages ncurses callPackage overrideCC gcc7 perl bison flex;
  });

triggers

$ nix-build -A linux_4_15                                                                                                                                                                                                                   
error: while evaluating 'callPackageWith' at /home/teto/nixpkgs/lib/customisation.nix:113:35, called from /home/teto/nixpkgs/pkgs/top-level/all-packages.nix:13178:16:
while evaluating 'makeOverridable' at /home/teto/nixpkgs/lib/customisation.nix:72:24, called from /home/teto/nixpkgs/lib/customisation.nix:117:8:
while evaluating anonymous function at /home/teto/nixpkgs/pkgs/os-specific/linux/kernel/linux-4.15.nix:1:1, called from /home/teto/nixpkgs/lib/customisation.nix:74:12:
while evaluating 'makeOverridable' at /home/teto/nixpkgs/lib/customisation.nix:72:24, called from /home/teto/nixpkgs/pkgs/os-specific/linux/kernel/linux-4.15.nix:5:1:
anonymous function at /home/teto/nixpkgs/pkgs/os-specific/linux/kernel/generic.nix:1:1 called without required argument 'src', at /home/teto/nixpkgs/pkgs/top-level/all-packages.nix:13444:33

and I believe we still want to make it overridable, we don't want every buildLinux call to pass all arguments, only src/patches.

NB: I also made arch overridable as I've started to use LKL, aka the Linux Kernel Library and would like eventually to have LKL use a generated config, lkl derivation is here https://github.com/teto/nixpkgs/blob/kernel_overrides/pkgs/applications/virtualization/lkl/default.nix)

@Ericson2314
Copy link
Member

Ericson2314 commented Mar 5, 2018

Try:

let # `let` here just for syntax highlighting

 buildLinux = attrs: callPackage ../os-specific/linux/kernel/generic.nix attrs;

@teto
Copy link
Member Author

teto commented Mar 6, 2018

Thanks for your patience, your suggestion seems to work just fine :)

@Ericson2314
Copy link
Member

Lmk when you think it's ready. CC @vcunat.

@teto
Copy link
Member Author

teto commented Mar 6, 2018

I tried it on my usecase and it worked fine. I could also compile default kernels. I think it's ready if there is no further comment. I am not sure if I should split the parameters fixing and additionnal overrides in separate commit or squash everything. If the latter it can be done via github's UI.

@Ericson2314
Copy link
Member

One big squash can be down via GitHub, but not two commits. Your call (as far as I'm concerned) which option we go with.

teto added 2 commits March 8, 2018 12:45
and passes parameters in a single set
- Easy override of autoModules and preferBuiltin and kernelArch parameters (currently living in `hostSystem` set).
@teto
Copy link
Member Author

teto commented Mar 8, 2018

@GrahamcOfBorg eval

@teto
Copy link
Member Author

teto commented Mar 8, 2018

I made it 2 commits, both should work and can be reverted independently. They seemed to work locally not sure why grahamcofborg fails (looks master related)

@teto
Copy link
Member Author

teto commented Mar 8, 2018

@GrahamcOfBorg eval

@grahamc
Copy link
Member

grahamc commented Mar 8, 2018

@GrahamcOfBorg eval

@Ericson2314 Ericson2314 merged commit ce5a762 into NixOS:master Mar 8, 2018
@Ericson2314
Copy link
Member

Thank you!

@Ericson2314
Copy link
Member

Oh oops I misread; this should have gone to staging.

@teto teto deleted the kernel_overrides branch March 9, 2018 03:40
@vcunat
Copy link
Member

vcunat commented Mar 12, 2018

It probably "only" rebuilds kernels (and modules), so that seems OK for master (and the count is typically close to 500 in such cases).

@Ericson2314
Copy link
Member

whew :)

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

5 participants