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

Remove "argsOverride" from linux kernel #32014

Closed
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.

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 :-)

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Nov 24, 2017
@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
8 tasks
@layus
Copy link
Member Author

layus commented Feb 16, 2018

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

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

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

@veprbl veprbl added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 28, 2018
@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
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants