-
-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
services/hardware/undervolt: Fix several issues #320288
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this be simpler? (obviating the need for hasCliArgs
)
@@ -165,7 +177,7 @@ in | |||
|
|||
environment.systemPackages = [ cfg.package ]; | |||
|
|||
systemd.services.undervolt = { | |||
systemd.services.undervolt = mkIf hasCliArgs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
systemd.services.undervolt = mkIf hasCliArgs { | |
systemd.services.undervolt = mkIf (cliArgs != [ ]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not about "pretty" but "error-prone".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, since we need to care about combinations of flags, we're forced to maintain an inclusion list. Marking as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nixos-rebuild boot
doesn't give an error though? It evals and builds the system just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
We should give an error (when possible) regarding invalid configurations. Silently failing is far worse. |
The semantics of So when a user sets
I do not follow. Could you give a real-live example?
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 What you want sounds like a new option |
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. |
Description of changes
This fixes several issues:
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.