lib/modules: Add function to create option alias that respects priority #53397
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…he priority This commit adds a function `mkAliasOptionModuleWithPriority`. This function will make an alias to an existing option and copy over the priority. This functionality is needed for PRs like NixOS#53041. In that case `nixos-generate-config` added an option to `hardware-configuration.nix` with `mkDefault`. That option was then changed and an alias created for the old name. The end user should be able to set the non-alias option in their `configuration.nix` and have everything work correctly. Without this function, the priority for the option won't be copied over correctly and the end-user will get a message saying they have the same option set to two different values.
nbp
suggested changes
Jan 5, 2019
| withPriority = true; | ||
| }; | ||
|
|
||
| doRename = { from, to, visible, warn, use, withPriority ? false }: |
There was a problem hiding this comment.
Once this issue is fixed. Open a follow-up issue / PR to evaluate the impact of switching this flag to true and replacing mkAliasAndWrapDefinitions with the one you are adding.
|
@nbp Thanks for the review and the good suggestions. I have fixed the things you've pointed out. This PR should be ready for a final review and hopefully merging. |
|
Hey @nbp, any chance getting this merged? |
nbp
approved these changes
Jan 14, 2019
10 tasks
infinisil
pushed a commit
that referenced
this pull request
Feb 23, 2019
…lias. This commit changes the `mkAliasOptionModule` function to make sure that the priority for the aliased option is propagated to the non-aliased option. This also affects the `mkRenamedOptionModule` function in a similar fashion. This also removes the `mkAliasOptionModuleWithPriority` function, since its functionality is now subsumed by `mkAliasOptionModule`. This change was recommended by @nbp: #53397 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
This commit adds a function
mkAliasOptionModuleWithPriority. This function will make an alias to an existing option and copy over the priority. (This is in contrast tomkAliasOptionModulewhich does not copy over the priority.)This functionality is needed for PRs like #53041 (specifically #53041 (comment)).
Here's the use-case:
There exists an option
powerManagement.cpuFreqGovernor. On some systems, this is set inhardware-configuration.nixbynixos-generate-config:Note how it gets a low priority. The end-user (or other modules) should be able to easily override it.
In #53041, I wanted to add options to set the CPU max and min frequency:
I thought it was silly to have the options called both
powerManagement.cpuFreqGovernorandpowerManagement.cpufreq.max, so I renamedpowerManagement.cpuFreqGovernortopowerManagement.cpufreq.governor. This is fine, but it will break for people that are usinghardware-configuration.nixgenerated with an older version ofnixos-generate-config(since in their config it is calledpowerManagement.cpuFreqGovernor).In order to accommodate those people, I added the alias
powerManagement.cpuFreqGovernortopowerManagement.cpufreq.governor:This should let people use their older
hardware-configuration.nixwith thepowerManagement.cpuFreqGovernoroption.However, there is one edge case this doesn't work for.
Imagine that the user has
powerManagement.cpuFreqGovernorset inhardware-configuration.nix:The user then tries to set
powerManagement.cpufreq.governorto something else inconfiguration.nix:This actually will fail. The user will get an error that
powerManagement.cpufreq.governoris defined two places (which is true, but the definition inhardware-configuration.nixshould have low priority).What's going on here? Well, the
mkAliasOptionModulefunction doesn't actually copy over the priority of the alias option to the normal option. So to the nix module system, it really does look likepowerManagement.cpufreq.governoris being set two different places.This PR adds a function
mkAliasOptionModuleWithPrioritythat does correctly(?) copy over the priority from the alias option to the normal option.Tests have also been added to make sure this is working correctly.
Pinging @infinisil since he helped a lot with #53041. Also pinging @edolstra @nbp since they seem to be code-owners of the stuff in
lib/.Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)