services/hardware/undervolt: Fix several issues

[Pandapip1]

Description of changes

This fixes several issues:

1. Inconsistent indentation
2. Adds an assertion that will fail if the service would fail to start due to no undervolt settings being applied.
3. Fixes incorrect temperature handling.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[eclairevoyant]

wouldn't this be simpler? (obviating the need for hasCliArgs)

[eclairevoyant]

Suggested change

	systemd.services.undervolt = mkIf hasCliArgs {
	systemd.services.undervolt = mkIf (cliArgs != [ ]) {

[jian-lin]

I am afraid this will not work. If users only set services.undervolt.verbose = true but no other settings, then cliArgs will be [ "--verbose" ] and the service will still fail to start (I guess).

[eclairevoyant]

eh fair enough, I still feel it's redundant to list out every single flag though. Maybe exclude the flags that don't allow the service to start?

[Pandapip1]

It's redundant, but this is one of the times where a tradeoff has to be made between correct functionality and pretty code. This code, while not pretty, is self-evident. Having a list of flags be excluded will result in harder-to-read code that looks marginally prettier.

[eclairevoyant]

It's not about "pretty" but "error-prone".

[eclairevoyant]

Okay, since we need to care about combinations of flags, we're forced to maintain an inclusion list. Marking as resolved.

[jian-lin]

The service will fail to start if no undervolt settings are applied. This disables the service to not pollute the journal with failures.

This is the original version of PR description and probably is the author's motivation.

I am a bit confused about how this service pollutes the journal with failures. There is no restart logic defined for this service so it only produces one failure log after nixos-rebuild test (or switch).

I am not a fan of maintaining a list in assertion. IMHO, in this case, a runtime failure signal from nixos-rebuild is better than an eval time error (assertion). To sum up, I suggest not adding an assertion.

[Pandapip1]

I am a bit confused about how this service pollutes the journal with failures. There is no restart logic defined for this service so it only produces one failure log after nixos-rebuild test (or switch).

I reboot frequently.

I am not a fan of maintaining a list in assertion

I don't particularly like it either, but I don't really see a way around it. The undervolt utility interface looks pretty stable though, so it shouldn't need too much maintenance.

IMHO, in this case, a runtime failure signal from nixos-rebuild is better than an eval time error (assertion). To sum up, I suggest not adding an assertion.

I am of the opinion that compile/eval tine errors are always preferable to runtime errors.

[jian-lin]

I am a bit confused about how this service pollutes the journal with failures. There is no restart logic defined for this service so it only produces one failure log after nixos-rebuild test (or switch).

I reboot frequently.

I do not follow. Why don't you fix your config for services.undervolt when nixos-rebuild gives you an error?

[Pandapip1]

nixos-rebuild boot doesn't give an error though? It evals and builds the system just fine.

[jian-lin]

I think it is better to use assetions than mkIf.

[Pandapip1]

I think it is better to use assetions than mkIf.

Could you provide a rationale? I'm not opposed to it, but my rationale for using mkIf is twofold:

1. It simplifies configuration for multiple machines
2. The expected result of enabling the undervolt service with no arguments is nothing, not a compile-time error

[eclairevoyant]

We should give an error (when possible) regarding invalid configurations. Silently failing is far worse.

[jian-lin]

I think it is better to use assetions than mkIf.

Could you provide a rationale?

The semantics of services.foo.enable = true is to run the service. For example, you can tell that from the description of services.undervolt.enable: Whether to enable Undervolting service for Intel CPUs. The service means some systemd services together with some related system configurations.

So when a user sets services.foo.enable = true, he expects some services to run. If, for some reason, the services fail to run, the NixOS module ideally should give the user clear signals, such as eval-time errors which is "compile-time error" in your words. And assertions are a perfect fit for this.

I'm not opposed to it, but my rationale for using mkIf is twofold:

1. It simplifies configuration for multiple machines

I do not follow. Could you give a real-live example?

2. The expected result of enabling the undervolt service with no arguments is nothing, not a compile-time error

As I said above, an eval-time error is the expected result in this case. Hiding the fact that the service is not run even if services.undervolt.enable = true is a very bad behavior and is against the convention.

What you want sounds like a new option services.undervolt.runIfPossible, which I do not think is a good idea. Just do not enable services.undervolt.enable if you do not want to run the service.

[Pandapip1]

We should give an error (when possible) regarding invalid configurations.

I disagree that the configuration would be invalid. That being said, there seems to be a consensus that it is somehow invalid. While I don't completely understand the rationale for it, for the time being I'll switch to assertions.