auto-epp: init at 1.2.1 #261378

lamarios · 2023-10-16T10:34:19Z

nixos/services/hardware/auto-epp-go: init

module to set up auto-epp-go with its kernel parameter, systemd service and config file

Description of changes

Things done

h7x4

You'll have to split up your changes into three commits:

maintainers: add lamarios
auto-epp: init at 1.2.1
nixos/auto-epp: init

I'm not entirely convinced that the packaging is correct, but I'll leave that to other reviewers who might know better than me.

h7x4 · 2023-10-16T17:14:30Z

nixos/modules/services/hardware/auto-epp.nix

+let
+
+ cfg = config.services.auto-epp;
+ package = pkgs.auto-epp;


Consider making this an mkPackageOption

nixos/modules/services/hardware/auto-epp.nix

h7x4 · 2023-10-16T17:30:54Z

nixos/modules/services/hardware/auto-epp.nix

+
+    systemd.services.auto-epp = {
+      enable = true;
+      after = [ "network.target" "network-online.target"  ];


Does this service really need networking in order to work?

most likely not, i was following upstream. Will change accordingly.

Cool. Maybe we should notify upstream

h7x4 · 2023-10-16T17:32:33Z

nixos/modules/services/hardware/auto-epp.nix

+      description = "auto-epp - Automatic EPP Changer for amd-pstate-epp";
+      serviceConfig = {
+        Type = "simple";
+        User = "root";


Does this service need to run as root? Maybe we could make it DynamicUser and give it the permissions it needs through CapabilityBoundingSet?

I'll need to research on this as i have no idea on how those work.

After some check, it requires to run as root, logs from journalctl:

Oct 17 12:02:01 yoga7 auto-epp[53340]: auto-epp must be run with root privileges.

see upstream

pkgs/tools/system/auto-epp/default.nix

nixos/modules/services/hardware/auto-epp.nix

h7x4 · 2023-10-16T17:41:54Z

nixos/modules/services/hardware/auto-epp.nix

@@ -0,0 +1,59 @@
+{ config, lib, pkgs, ... }:
+with lib;


There are ongoing efforts to avoid with lib; statements. I won't block the PR on this, but would you mind changing it out for an inherit (lib) ...; for the most commonly used items, and just prepending lib. to everything that's only used once or twice?

ok, all addressed except commit splitting, let's see if it's good to go first then i'll split

h7x4

Okay, just a few more comments

nixos/modules/services/hardware/auto-epp.nix

h7x4 · 2023-10-19T16:35:39Z

pkgs/tools/system/auto-epp/default.nix

+  '';
+
+  meta = with lib; {
+    mainProgram = pname;


nit: while i understand the intent here, it's semantically wrong. The executable doesn't necessarily have the same name as the package - it just happens to be the case here (and for many other packages as well).

sure, i'll create a variable for it

h7x4 · 2023-10-19T16:36:59Z

pkgs/tools/system/auto-epp/default.nix

@@ -0,0 +1,31 @@
+{ lib, stdenv, fetchFromGitHub, python3 }:


Would you be willing to move your package to the new by-name set by the way? AFAIK it's not obligatory just yet, but many new packages are shifting towards using this.

sure, any docs or example i can read ?

Sure! There's https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/README.md

if i understand properly, i just need to move my default.nix file to pkgs/by-name/au/auto-epp/package.nix and remove it from the top-level/all-packages.nix ?

Yes, I think so

nixos/modules/services/hardware/auto-epp.nix

h7x4 · 2023-10-20T15:35:24Z

nixos/modules/services/hardware/auto-epp.nix

+    systemd.packages = [ cfg.package ];
+
+    systemd.services.auto-epp = lib.mkIf cfg.enable {
+      enable = true;


This is redundant, I think: https://github.com/NixOS/nixpkgs/blob/master/nixos/lib/systemd-unit-options.nix#L37-L38

h7x4 · 2023-10-20T15:37:00Z

This is looking pretty good. You'll have to rebase your commits into

maintainers: add lamarios
auto-epp: init at 1.2.1
nixos/auto-epp: init

But apart from that, this is ready to merge as far as I'm concerned

lamarios · 2023-10-21T07:57:06Z

@h7x4 all done ! Thanks for the review

lamarios · 2023-10-26T02:59:29Z

nixos/modules/services/hardware/auto-epp.nix

+            Settings = {
+              epp_state_for_AC = mkOption {
+                type = types.str;
+                default = "balance_performance";


@h7x4, I had to make changes, i realized i inverted the defaults for battery and ac modes... This was power before.

lamarios · 2023-10-26T02:59:36Z

nixos/modules/services/hardware/auto-epp.nix

+
+              epp_state_for_BAT = mkOption {
+                type = types.str;
+                default = "power";


@h7x4, I had to make changes, i realized i inverted the defaults for battery and ac modes... This was balance_performance before

h7x4

Some final comments

pkgs/by-name/au/auto-epp/package.nix

nixos/modules/services/hardware/auto-epp.nix

lamarios · 2023-11-16T04:18:18Z

Some final comments

all done, thanks

pbsds

Result of nixpkgs-review pr 261378 run on x86_64-linux 1

2 packages blacklisted:

nixos-install-tools
tests.nixos-functions.nixos-test

1 package built:

auto-epp

I also built the manual with nix build github:nixos/nixpkgs/refs/pull/261378/head#htmlDocs.nixosManual.
Couple of nits, yet to test building the module.

nixos/modules/services/hardware/auto-epp.nix

pkgs/by-name/au/auto-epp/package.nix

nixos/modules/services/hardware/auto-epp.nix

lamarios · 2023-11-28T04:13:56Z

Result of nixpkgs-review pr 261378 run on x86_64-linux 1
2 packages blacklisted:
* nixos-install-tools

* tests.nixos-functions.nixos-test
1 package built:
* auto-epp
I also built the manual with nix build github:nixos/nixpkgs/refs/pull/261378/head#htmlDocs.nixosManual. Couple of nits, yet to test building the module.

After committing the suggestions, there's an issue with build manual:

    3| let
       >             4|   cfg = config.services.auto-epp;
       >              |         ^
       >             5|   format = pkgs.formats.ini {};
       > Cacheable portion of option doc build failed.
       > Usually this means that an option attribute that ends up in documentation (eg `default` or `description`) depends on the restricted module arguments `config` or `pkgs`.

Looks like we can't use config.xxx in the docs. Should we hardcode the url to point to the github repo instead ?

h7x4 · 2023-11-28T09:54:01Z

Should we hardcode the url to point to the github repo instead ?

Yes, I think that would be good. Most module docs point to the projects main branch, so maybe https://github.com/jothi-prasath/auto-epp/blob/master/sample-auto-epp.conf?

lamarios · 2023-11-29T06:29:44Z

Should we hardcode the url to point to the github repo instead ?

Yes, I think that would be good. Most module docs point to the projects main branch, so maybe https://github.com/jothi-prasath/auto-epp/blob/master/sample-auto-epp.conf?

done

github-actions bot added 6.topic: nixos 8.has: module (update) labels Oct 16, 2023

lamarios force-pushed the auto-epp branch 2 times, most recently from 872b65b to f6a3ae1 Compare October 16, 2023 10:38

ofborg bot added 8.has: package (new) 11.by: package-maintainer 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Oct 16, 2023

h7x4 added 8.has: module (new) and removed 8.has: module (update) labels Oct 16, 2023

github-actions bot added the 8.has: module (update) label Oct 16, 2023

h7x4 requested changes Oct 16, 2023

View reviewed changes

lamarios force-pushed the auto-epp branch 6 times, most recently from fff1c60 to de72325 Compare October 18, 2023 08:52

h7x4 reviewed Oct 19, 2023

View reviewed changes

lamarios mentioned this pull request Oct 20, 2023

Not forcing to run as root, instead check write permission on required files jothi-prasath/auto-epp#8

Open

h7x4 reviewed Oct 20, 2023

View reviewed changes

lamarios force-pushed the auto-epp branch 2 times, most recently from 0afbf62 to eeb99ca Compare October 21, 2023 07:56

h7x4 approved these changes Oct 21, 2023

View reviewed changes

delroth added the 12.approvals: 1 label Oct 21, 2023

lamarios force-pushed the auto-epp branch from eeb99ca to cf9ad81 Compare October 26, 2023 02:58

lamarios commented Oct 26, 2023

View reviewed changes

h7x4 reviewed Nov 15, 2023

View reviewed changes

pkgs/by-name/au/auto-epp/package.nix Outdated Show resolved Hide resolved

nixos/modules/services/hardware/auto-epp.nix Outdated Show resolved Hide resolved

maintainers: add lamarios

2b0ca5c

lamarios force-pushed the auto-epp branch from b29f655 to d6218fa Compare November 16, 2023 04:17

lamarios force-pushed the auto-epp branch from d6218fa to ace5ec0 Compare November 16, 2023 04:20

pbsds reviewed Nov 27, 2023

View reviewed changes

lamarios added 2 commits November 29, 2023 14:28

auto-epp: init at 1.2.1

f8e9bff

nixos/auto-epp: init

b034425

lamarios force-pushed the auto-epp branch from c249908 to b034425 Compare November 29, 2023 06:28

pbsds merged commit 23fb8f6 into NixOS:master Nov 30, 2023
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auto-epp: init at 1.2.1 #261378

auto-epp: init at 1.2.1 #261378

lamarios commented Oct 16, 2023 •

edited

h7x4 left a comment

h7x4 Oct 16, 2023

h7x4 Oct 16, 2023

lamarios Oct 17, 2023

h7x4 Oct 19, 2023

h7x4 Oct 16, 2023

lamarios Oct 17, 2023

lamarios Oct 17, 2023 •

edited

h7x4 Oct 16, 2023

lamarios Oct 17, 2023

h7x4 left a comment

h7x4 Oct 19, 2023

lamarios Oct 20, 2023 •

edited

h7x4 Oct 19, 2023

lamarios Oct 20, 2023

h7x4 Oct 20, 2023

lamarios Oct 20, 2023

h7x4 Oct 20, 2023

lamarios Oct 20, 2023

h7x4 Oct 20, 2023

h7x4 commented Oct 20, 2023

lamarios commented Oct 21, 2023

lamarios Oct 26, 2023 •

edited

lamarios Oct 26, 2023 •

edited

h7x4 left a comment

lamarios commented Nov 16, 2023

pbsds left a comment

lamarios commented Nov 28, 2023

h7x4 commented Nov 28, 2023

lamarios commented Nov 29, 2023

auto-epp: init at 1.2.1 #261378

auto-epp: init at 1.2.1 #261378

Conversation

lamarios commented Oct 16, 2023 • edited

Description of changes

Things done

h7x4 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

lamarios Oct 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h7x4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lamarios Oct 20, 2023 • edited

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

h7x4 commented Oct 20, 2023

lamarios commented Oct 21, 2023

lamarios Oct 26, 2023 • edited

Choose a reason for hiding this comment

lamarios Oct 26, 2023 • edited

Choose a reason for hiding this comment

h7x4 left a comment

Choose a reason for hiding this comment

lamarios commented Nov 16, 2023

pbsds left a comment

Choose a reason for hiding this comment

lamarios commented Nov 28, 2023

h7x4 commented Nov 28, 2023

lamarios commented Nov 29, 2023

lamarios commented Oct 16, 2023 •

edited

lamarios Oct 17, 2023 •

edited

lamarios Oct 20, 2023 •

edited

lamarios Oct 26, 2023 •

edited

lamarios Oct 26, 2023 •

edited