upower: Upgrade to 1.90.6 and extend CriticalPowerActions #341086

benjamb · 2024-09-10T19:51:24Z

Description of changes

Upgrade to 1.90.6
Support Suspend and Ignore CriticalPowerActions introduced in this release.

Things done

Add a 👍 reaction to pull requests you find important.

benjamb · 2024-09-10T19:52:28Z

When I build locally, the compilation succeeds, but I run into test failures.

jtojnar

Skimming the upstream diff, looks okay:

https://gitlab.freedesktop.org/upower/upower/-/compare/v1.90.4...v1.90.5

jtojnar · 2024-09-10T21:56:26Z

nixos/modules/services/hardware/upower.nix

@@ -218,6 +246,7 @@ in
        TimeLow = cfg.timeLow;
        TimeCritical = cfg.timeCritical;
        TimeAction = cfg.timeAction;
+        AllowRiskyCriticalPowerAction = cfg.allowRiskyCriticalPowerAction;


Ugh, we should really switch to RFC 42-style settings so that we do not need to keep adding the options forever like this. Opened #341107 for now

That does indeed seem like better way of doing things.

jtojnar · 2024-09-10T22:10:32Z

pkgs/os-specific/linux/upower/default.nix

@@ -80,6 +81,7 @@ stdenv.mkDerivation (finalAttrs: {
  buildInputs = [
    libgudev
    libusb1
+    polkit


Looking at the diff, https://gitlab.freedesktop.org/upower/upower/-/commit/0c9ba8048fd38567aa1292f3760fbff48904c3ca sets POLKIT_ACTIONDIR define but it does not look like it is currently used? If not, that should not be an issue.

Indeed, it doesn't look like it's used.

pkgs/os-specific/linux/upower/default.nix

benjamb · 2024-09-18T08:30:14Z

@jtojnar I've bumped up to 1.90.6, which avoids the need for patching the tests.

benjamb · 2024-10-01T22:24:34Z

@jtojnar Don't suppose you have time for another review pass?

jfly

Thank you for this change! I just had my laptop die on my for the upteenth time, and decided to finally set up upower.

I don't have hibernation support enabled on my machine, so I want this Sleep functionality!

jfly · 2024-10-09T19:27:18Z

nixos/modules/services/hardware/upower.nix

+    assertions = [
+      {
+        assertion =
+          with builtins;


nit, feel free to ignore: what is builtins used for here? Is it just elem? I think this would be nicer as just a inherit (builtins) elem; in the let block instead.

jfly · 2024-10-09T19:28:45Z

nixos/modules/services/hardware/upower.nix

+            ];
+            riskyActionEnabled = elem cfg.criticalPowerAction riskyActions;
+          in
+          !cfg.allowRiskyCriticalPowerAction && riskyActionEnabled;


nit, feel free to ignore: would this be cleaner written as riskyActionEnabled -> cfg.allowRiskyCriticalPowerAction? I think it's the same boolean logic, and reads a bit more nicely ("if a risky action is enabled, then we must allow risky actions").

Actually, is the current assertion correct? I'm trying out this patch locally, and I've set services.upower.criticalPowerAction = "Suspend", so riskyActionEnabled = true. I have not yet set services.upower.allowRiskyCriticalPowerAction to anything, so it's just the default value of false.

Plugging that into this expression, I get:

!cfg.allowRiskyCriticalPowerAction && riskyActionEnabled => !false && true => true && true => true

That's wrong, right? Don't we want this configuration to trigger the assertion (i.e., evaluate to false)?

I think the riskyActionEnabled -> cfg.allowRiskyCriticalPowerAction I suggested above is correct. Or perhaps !riskyActionEnabled or cfg.allowRiskyCriticalPowerAction.

TIL ->.

Quite right. I was treating that as an expression that when evaluated as true would trigger an error, rather than (what should have been clear to me) when the assertion condition is not met.

Good spot!

jfly · 2024-10-09T19:33:42Z

nixos/modules/services/hardware/upower.nix

-          (UPS or laptop batteries) supplying the computer
+          (UPS or laptop batteries) supplying the computer.
+
+          When set to `Suspend` or `Ignore`, `allowRiskyCriticalPowerAction`


I recently learned there's special syntax for referencing options. See https://github.com/NixOS/nixpkgs/tree/master/doc#roles

Suggested change

When set to `Suspend` or `Ignore`, `allowRiskyCriticalPowerAction`

When set to `Suspend` or `Ignore`, {option}`services.upower.allowRiskyCriticalPowerAction`

Good to know! Done.

jfly

Latest changes LGTM

benjamb · 2024-10-23T11:41:40Z

@jfly @jtojnar What's required of me in order for this to land? It would be great to get this in before 24.11.

nixos-discourse · 2024-10-26T23:17:31Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/2062

jfly · 2024-10-26T23:17:57Z

I've been using this for a couple weeks and it's working great for me.

I've added a link to this PR in https://discourse.nixos.org/t/prs-already-reviewed/2617/2062.

jfly · 2024-11-22T08:11:32Z

@benjamb, do you have time to rebase this branchb and address the conflicts? If it's helpful, I'm maintaining a branch here with your changes: https://github.com/jfly/nixpkgs/tree/upower-critical-actions

Changes: https://gitlab.freedesktop.org/upower/upower/-/blob/v1.90.6/NEWS

benjamb · 2024-11-22T14:11:49Z

@benjamb, do you have time to rebase this brand and address the conflicts?

Done!

jfly · 2024-12-16T07:19:31Z

@jtojnar, any chance of getting this merged?

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Sep 10, 2024

ofborg bot requested a review from jtojnar September 10, 2024 21:42

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 101-500 labels Sep 10, 2024

jtojnar reviewed Sep 10, 2024

View reviewed changes

benjamb force-pushed the upower-critical-actions branch 2 times, most recently from 8c1e1f5 to ad1a29d Compare September 11, 2024 11:15

ofborg bot requested a review from jtojnar September 11, 2024 21:23

benjamb force-pushed the upower-critical-actions branch 3 times, most recently from 9a67952 to 508db42 Compare September 18, 2024 08:26

benjamb changed the title ~~upower: Upgrade to 1.90.5 and extend CriticalPowerActions~~ upower: Upgrade to 1.90.6 and extend CriticalPowerActions Sep 18, 2024

This was referenced Sep 25, 2024

upower: update percentage defaults #344466

Closed

nixos/upower: update percentage defaults #344839

Merged

jfly approved these changes Oct 9, 2024

View reviewed changes

benjamb force-pushed the upower-critical-actions branch from 508db42 to 57077af Compare October 9, 2024 23:40

jfly approved these changes Oct 10, 2024

View reviewed changes

wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 29, 2024

benjamb added 2 commits November 22, 2024 14:10

upower: 1.90.4 -> 1.90.6

41b082d

Changes: https://gitlab.freedesktop.org/upower/upower/-/blob/v1.90.6/NEWS

upower: Support Suspend and Ignore CriticalPowerActions

789bb45

benjamb force-pushed the upower-critical-actions branch from 57077af to 789bb45 Compare November 22, 2024 14:11

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 23, 2024

wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 23, 2024

ofborg bot added 10.rebuild-darwin: 11-100 and removed 10.rebuild-darwin: 1-10 labels Nov 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upower: Upgrade to 1.90.6 and extend CriticalPowerActions #341086

upower: Upgrade to 1.90.6 and extend CriticalPowerActions #341086

benjamb commented Sep 10, 2024 •

edited

Loading

benjamb commented Sep 10, 2024

jtojnar left a comment

jtojnar Sep 10, 2024

benjamb Sep 12, 2024

jtojnar Sep 10, 2024

benjamb Sep 12, 2024

benjamb commented Sep 18, 2024

benjamb commented Oct 1, 2024

jfly left a comment

jfly Oct 9, 2024

benjamb Oct 9, 2024

jfly Oct 9, 2024

jfly Oct 9, 2024

benjamb Oct 9, 2024

jfly Oct 9, 2024

benjamb Oct 9, 2024

jfly left a comment

benjamb commented Oct 23, 2024

nixos-discourse commented Oct 26, 2024

jfly commented Oct 26, 2024

jfly commented Nov 22, 2024 •

edited

Loading

benjamb commented Nov 22, 2024

jfly commented Dec 16, 2024

	When set to `Suspend` or `Ignore`, `allowRiskyCriticalPowerAction`
	When set to `Suspend` or `Ignore`, {option}`services.upower.allowRiskyCriticalPowerAction`

upower: Upgrade to 1.90.6 and extend CriticalPowerActions #341086

Are you sure you want to change the base?

upower: Upgrade to 1.90.6 and extend CriticalPowerActions #341086

Conversation

benjamb commented Sep 10, 2024 • edited Loading

Description of changes

Things done

benjamb commented Sep 10, 2024

jtojnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamb commented Sep 18, 2024

benjamb commented Oct 1, 2024

jfly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfly left a comment

Choose a reason for hiding this comment

benjamb commented Oct 23, 2024

nixos-discourse commented Oct 26, 2024

jfly commented Oct 26, 2024

jfly commented Nov 22, 2024 • edited Loading

benjamb commented Nov 22, 2024

jfly commented Dec 16, 2024

benjamb commented Sep 10, 2024 •

edited

Loading

jfly commented Nov 22, 2024 •

edited

Loading