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.

Sign up for GitHub

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

nixos/powerManagement: set cpuFreqGovernor with mkOptionDefault #34350

Merged
merged 1 commit into from Jan 28, 2018
Merged

nixos/powerManagement: set cpuFreqGovernor with mkOptionDefault #34350

merged 1 commit into from Jan 28, 2018

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Jan 28, 2018 •
edited

Motivation for this change

nixos-generate-config detects the cpuFreqGovernor suited best for my
machine, e.g. powerManagement.cpuFreqGovernor = lib.mkDefault "powersave";.

However the powerManagement module sets a sensitive default for
cpuFreqGovernor using mkDefault to avoid breackage with older
setups. Since 140ac2f1 the hardware-configuration.nix sets the
gorvernor with mkDefault as well which causes evaluation errors if the
powermanagement module is enabled:

error: The unique option `powerManagement.cpuFreqGovernor' is defined multiple times, in `/home/ma27/Projects/nixos-config/hardware-configuration.nix' and `/nix/var/nix/profiles/per-user/root/channels/nixos/nixpkgs/nixos/modules/config/power-management.nix'.

Using mkOptionDefault rather than mkDefault in the powermanagement
module fixes this issue as it decreases the priority of the module and
prefers the value set in hardware-configuration.nix.

I have confirmed the change using the following VM declaration:

{
  cpuFreq = { lib, ... }: {
    powerManagement.cpuFreqGovernor = lib.mkDefault "powersave";
    powerManagement.enable = true;
  };
}
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.

@Ma27
nixos/powerManagement: set cpuFreqGovernor with mkOptionDefault
8ed3a90 
`nixos-generate-config` detects the `cpuFreqGovernor` suited best for my
machine, e.g. `powerManagement.cpuFreqGovernor = lib.mkDefault "powersave";`.

However the `powerManagement` module sets a sensitive default for
`cpuFreqGovernor` using `mkDefault` to avoid breackage with older
setups. Since 140ac2f1 the `hardware-configuration.nix` sets the
gorvernor with `mkDefault` as well which causes evaluation errors if the
powermanagement module is enabled:

```
error: The unique option `powerManagement.cpuFreqGovernor' is defined multiple times, in `/home/ma27/Projects/nixos-config/hardware-configuration.nix' and `/nix/var/nix/profiles/per-user/root/channels/nixos/nixpkgs/nixos/modules/config/power-management.nix'.
```

Using `mkOptionDefault` rather than `mkDefault` in the powermanagement
module fixes this issue as it decreases the priority of the module and
prefers the value set in `hardware-configuration.nix`.

I have confirmed the change using the following VM declaration:

```
{
  cpuFreq = { lib, ... }: {
    powerManagement.cpuFreqGovernor = lib.mkDefault "powersave";
    powerManagement.enable = true;
  };
}
```
@fpletz fpletz merged commit 50dda06 into NixOS:master Jan 28, 2018
@Ma27 Ma27 deleted the fix-powermanagement-default branch January 28, 2018 15:27
@grahamc
Copy link
Member

grahamc commented Jan 28, 2018

This seems to break tests:

while evaluating the option `powerManagement.cpuFreqGovernor':
while evaluating the attribute ‘mergedValue’ at /home/borg/borg-rs/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/7c6f434c-buildbox/lib/modules.nix:339:5:
while evaluating anonymous function at /home/borg/borg-rs/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/7c6f434c-buildbox/lib/modules.nix:339:32, called from /home/borg/borg-rs/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/7c6f434c-buildbox/lib/modules.nix:339:19:
while evaluating ‘merge’ at /home/borg/borg-rs/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/7c6f434c-buildbox/lib/types.nix:319:20, called from /home/borg/borg-rs/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/7c6f434c-buildbox/lib/modules.nix:342:8:
The option `powerManagement.cpuFreqGovernor` is defined both null and not null, in `/home/borg/borg-rs/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/7c6f434c-buildbox/nixos/modules/tasks/cpu-freq.nix' and `/home/borg/borg-rs/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/7c6f434c-buildbox/nixos/modules/config/power-management.nix'.

see also: https://logs.nix.gsc.io/?key=nixos/nixpkgs.34348

@grahamc grahamc mentioned this pull request Jan 28, 2018
Closed
8 tasks
@fpletz
Copy link
Member

fpletz commented Jan 28, 2018

@grahamc I removed that definition after some more digging in 36103e9. Not explicitly setting cpufreq governors by default should be fine.

fpletz added a commit that referenced this pull request Jan 28, 2018
@fpletz
nixos/powerManagement: remove duplicate definition
36103e9 
When not set just use the kernel default. `nixos-generate-config` will pick
a reasonable default.

cc #34350
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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
4 participants
@Ma27 @grahamc @fpletz @GrahamcOfBorg