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

Jump to bottom

Remove "argsOverride" from linux kernel #32014

Closed
wants to merge 1 commit into from
Closed

Remove "argsOverride" from linux kernel #32014

wants to merge 1 commit into from

Conversation

layus
Copy link
Member

@layus layus commented Nov 24, 2017

Motivation for this change

It is a bit strange to use these argsOverride attribute because it does not play well with the .override mechanism. The current situation is also inconsistent since #31596 was merged.

Things done

This patch unifies the syntax for all the kernels by removing the argsOverride support and merging the input args after the default values, making everything overridable.
A deprecation warning was also introduced for users relying on that feature.

  • Tested that all the derivations are strictly unchanged by this patch.
  • Fits CONTRIBUTING.md.

@layus
linux kernel: deprecate argsOverride
f0a7354 
argsOverride were introduced in 31fa2cd (NixOS#1654).
It seems simpler to just override the default values with the attrs themselves.
It also makes it more intuitive to use. (See NixOS#31596).
@layus
Copy link
Member Author

layus commented Nov 24, 2017

@teto @globin for review :-)

@teto
Copy link
Member

teto commented Nov 25, 2017

Depreciation warning is cool.
I don't have much experience regarding the kernel implementation aka why argsOverride was introduce in the first place but being able to use the usual 'override' seems like the way to go

@teto
Copy link
Member

teto commented Dec 5, 2017

can we label this as kernel/merge once rebased ? its is useful

@teto
Copy link
Member

teto commented Jan 25, 2018

rebase ?

@teto teto mentioned this pull request Jan 28, 2018
Merged
8 tasks
@layus
Copy link
Member Author

layus commented Feb 16, 2018 •
edited

Retrospectively, I think this is a bad idea. there is no reason to merge the full "args" attrset into the final derivation. We should really make buildLinux overridable.

@teto
Copy link
Member

teto commented Feb 18, 2018 •
edited

Isn't it already the case?
buildLinux = makeOverridable (callPackage ../os-specific/linux/kernel/generic.nix {});
argsOverride doesn't seem to be used anywhere in nixpkgs. I believe you could even remove the
args // rec {. Seems in line with the recent kernel building improvements.

EDIT: Note the usecase for introducing argsOverride should be adressed too by #31610

matthewbauer
matthewbauer requested changes Apr 21, 2018
View reviewed changes
Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

merge conflict

@layus layus closed this Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer requested changes

Assignees
No one assigned
Labels
2.status: merge conflict 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@layus @teto @matthewbauer @veprbl @GrahamcOfBorg