-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
Conversation
b9422eb
to
c2639ff
Compare
c2639ff
to
29cd090
Compare
sry @FRidh you can ignore the request, the PR was made against staging and my rebase against master, the push triggered the request. |
@Ericson2314 @dezgeg follow up of the previous kernel patch. Seems like changing the base branch against which to evaluate the PR confused the bot. |
29cd090
to
1f6295d
Compare
1f6295d
to
6981fd1
Compare
}: | ||
|
||
{ stdenv, buildPackages, perl, buildLinux |
There was a problem hiding this comment.
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.
@vcunat I tried to fix the useless parameters you mentioned in my previous PR. I hope it's good now. |
But do we even need to arguments like that? Just have buildLinux import instead of |
What's the gain of |
@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. |
0377171
to
02c34f2
Compare
I tried to merge the 2 sets as requested but then
triggers
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 |
Try: let # `let` here just for syntax highlighting
buildLinux = attrs: callPackage ../os-specific/linux/kernel/generic.nix attrs; |
Thanks for your patience, your suggestion seems to work just fine :) |
Lmk when you think it's ready. CC @vcunat. |
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. |
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. |
and passes parameters in a single set
- Easy override of autoModules and preferBuiltin and kernelArch parameters (currently living in `hostSystem` set).
@GrahamcOfBorg eval |
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) |
@GrahamcOfBorg eval |
@GrahamcOfBorg eval |
Thank you! |
Oh oops I misread; this should have gone to staging. |
It probably "only" rebuilds kernels (and modules), so that seems OK for master (and the count is typically close to 500 in such cases). |
whew :) |
Follow up PR of #34351
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
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)