atop, netatop, nixos/atop: improve packaging and options #123053

pschyska · 2021-05-15T00:09:56Z

Motivation for this change

The atop package where missing some files from upstream that I wanted to use (systemd units), mostly because $out instead of $(out) was used in a make context and this led to them not being copied.
The netatop package was missing the unit file for the netatop service, which is required for process accounting.
The nixos/atop module was extended to include configuration for all of atop's standard components, and optionally atopgpu and netatop.

Because I don't have a NVIDIA GPU, I wasn't able to test atopgpud.

Things done

r-rmcgibbo · 2021-05-15T01:06:58Z

Result of nixpkgs-review pr 123053 at a8898703 run on aarch64-linux 1

8 packages marked as broken and skipped:

linuxPackages-libre.netatop
linuxPackages_4_4.netatop
linuxPackages_4_9.netatop
linuxPackages_hardkernel_4_14.netatop
linuxPackages_hardkernel_latest.netatop
linuxPackages_latest-libre.netatop
linuxPackages_testing_bcachefs.netatop
linuxPackages_xanmod.netatop

11 packages built successfully:

atop
linuxPackages.netatop (linuxPackages_5_10.netatop)
linuxPackages_4_14.netatop
linuxPackages_4_19.netatop
linuxPackages_5_11.netatop (linuxPackages_latest.netatop)
linuxPackages_5_12.netatop
linuxPackages_5_4.netatop
linuxPackages_hardened.netatop
linuxPackages_latest_hardened.netatop
linuxPackages_lqx.netatop
linuxPackages_zen.netatop

Result of nixpkgs-review pr 123053 at a8898703 run on x86_64-linux 1

4 packages marked as broken and skipped:

linuxPackages-libre.netatop
linuxPackages_hardkernel_4_14.netatop
linuxPackages_hardkernel_latest.netatop
linuxPackages_latest-libre.netatop

15 packages built successfully:

atop
linuxPackages.netatop (linuxPackages_5_10.netatop)
linuxPackages_4_14.netatop
linuxPackages_4_19.netatop
linuxPackages_4_4.netatop
linuxPackages_4_9.netatop
linuxPackages_5_11.netatop (linuxPackages_latest.netatop)
linuxPackages_5_12.netatop
linuxPackages_5_4.netatop
linuxPackages_hardened.netatop
linuxPackages_latest_hardened.netatop
linuxPackages_lqx.netatop
linuxPackages_testing_bcachefs.netatop
linuxPackages_xanmod.netatop
linuxPackages_zen.netatop

7 suggestions:

warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/os-specific/linux/netatop/default.nix:19:15:
```
   |
19 |   patches = [ ./netatop.service.patch ];
   |               ^
```
warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/os-specific/linux/atop/default.nix:41:5:
```
   |
41 |     ./atopacct.service.patch
   |     ^
```
warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/os-specific/linux/atop/default.nix:38:5:
```
   |
38 |     ./atop-pm.sh.patch
   |     ^
```
warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/os-specific/linux/atop/default.nix:42:31:
```
   |
42 |   ] ++ (if withAtopgpu then [ ./atopgpu.service.patch ] else [ ]);
   |                               ^
```
warning: unclear-gpl
gpl2 is a deprecated license, please check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

Near pkgs/os-specific/linux/netatop/default.nix:42:5:
```
   |
42 |     license = lib.licenses.gpl2;
   |     ^
```
warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/os-specific/linux/atop/default.nix:39:5:
```
   |
39 |     ./atop-rotate.service.patch
   |     ^
```
warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/os-specific/linux/atop/default.nix:40:5:
```
   |
40 |     ./atop.service.patch
   |     ^
```

nixos/tests/atop.nix

…top, gpuatop, allow setuid wrapper

…er enabling options

- use "with subtest" everywhere - do more in nix and less in python - use makeTest directly to define multiple tests instead of one with multiple nodes -> this enables them to run in parallel

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

…s and comment

…patches

pschyska · 2021-05-16T20:10:48Z

@ofborg test atop.everything

pschyska · 2021-05-16T20:13:05Z

Sorry @7c6f434c, I accidentally requested a re-review just now.

7c6f434c · 2021-05-16T20:56:54Z

@ofborg test atop.defaults

I am not a very careful person

7c6f434c

Diff looks good

pkgs/os-specific/linux/netatop/netatop.service.patch

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

SuperSandro2000 · 2021-05-16T23:05:29Z

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

+    "BINPATH=/bin"
+    "SBINPATH=/bin"
+    "MAN1PATH=/share/man/man1"
+    "MAN5PATH=/share/man/man5"
+    "MAN8PATH=/share/man/man8"
+    "SYSDPATH=/lib/systemd/system"
+    "PMPATHD=/lib/systemd/system-sleep"


Why do they not include $out?

They are all used as like this: $(DESTDIR)$(BINPATH), so $out is correctly prepended.

@SuperSandro2000 Works for you? Alternativeley I can DESTDIR="" and fully qualify all the names, but then I'd have to go through the Makefile again because I only mentioned the keys that were relevant, and were wrong by default with DESTDIR=$(out).

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

pkgs/os-specific/linux/atop/atopacct.service.patch

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

pschyska · 2021-05-18T19:32:05Z

❤️ Thank you @7c6f434c @aanderse @SuperSandro2000

samueldr · 2021-05-21T20:14:48Z

nixos/modules/programs/atop.nix

+        };
+        package = mkOption {
+          type = types.package;
+          default = config.boot.kernelPackages.netatop;


detaultText attribute missing for a package.

This can lead to breakage in some evals:

error: attribute 'netatop' missing, at /nix/store/md569yk7k26jdknlbarfyai4i1lb7kkj-nixexprs.tar.xz/nixos/modules/programs/atop.nix:38:21

(The other option here with a package would also need a defaultText)

(I don't think we have a linkable discussion or documentation about how default values for types.package options can cause issues with evaluations, sorry :/)

Hey @samueldr, sorry I broke your website build :-(
I'm opening a PR momentarily adding defaultTexts.
I'm having a problem with the tests locally, but I think I've got a fix already.

No biggie about the build failure :). It's an unusual failure type that, for this particular package, won't be a problem on the Hydra for NixOS itself. So it's really not obvious that it can cause some fringe breakage.

see: NixOS#123053 (comment)

Shados · 2021-05-31T14:17:13Z

This rather unexpected broke my atoprc because programs.atop.enable did not previously exist, but is now required to build it. Given this is a breaking change, it should probably at least get a mention in the NixOS release notes, if not an automated warning (although given the nature of the change, I don't think that could be cleanly done).

…f configured without the need to atop.enable see: NixOS#123053 (comment)

pschyska · 2021-05-31T19:00:45Z

@Shados Sorry for breaking your config. This case slipped my attention. I just opened a mini PR adding a test for your case and a fix.

FRidh · 2022-04-04T11:04:49Z

Note this allows the use of unfree packages. I am going to revert that part.

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 May 15, 2021

pschyska force-pushed the master branch from 9174379 to a889870 Compare May 15, 2021 00:14

ofborg bot requested review from 7c6f434c and viric May 15, 2021 00:23

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels May 15, 2021

7c6f434c reviewed May 16, 2021

View reviewed changes

nixos/tests/atop.nix Outdated Show resolved Hide resolved

pschyska added 5 commits May 16, 2021 18:22

netatop: package netatop.service

2703a79

atop: fix packaging .service files, add option to build with atopgpu

327dcea

nixos/atop: Add configuration for atop services, allow to enable neta…

8f3d2e5

…top, gpuatop, allow setuid wrapper

nixos/atop: Never enable setuidWrapper by default, rename service/tim…

b87c366

…er enabling options

nixos/atop: Rework the test

fb90a9c

- use "with subtest" everywhere - do more in nix and less in python - use makeTest directly to define multiple tests instead of one with multiple nodes -> this enables them to run in parallel

pschyska force-pushed the master branch from a889870 to fb90a9c Compare May 16, 2021 16:22

ofborg bot requested a review from 7c6f434c May 16, 2021 16:31

7c6f434c reviewed May 16, 2021

View reviewed changes

pkgs/os-specific/linux/atop/default.nix Outdated Show resolved Hide resolved

pschyska added 4 commits May 16, 2021 21:43

nixos/atop: Add a note about netatop tainting the kernel

526bc6a

nixos/atop: Split up restart triggers between atop and netatop

563ba07

netatop: Split out path fixing and PIDFile changes to separate patche…

27d323d

…s and comment

atop: Merge all path-fixing to a single patch, comment the remaining …

76554be

…patches

pschyska requested a review from 7c6f434c May 16, 2021 20:12

7c6f434c approved these changes May 16, 2021

View reviewed changes

SuperSandro2000 reviewed May 16, 2021

View reviewed changes

atop:, nixos/atop: Apply style suggestions

3b0bc54

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

ofborg bot requested a review from 7c6f434c May 16, 2021 23:37

atop: use substituteInPlace instead of sed

033cdff

pschyska force-pushed the master branch from 3693ffc to 033cdff Compare May 17, 2021 11:58

7c6f434c merged commit 02ba323 into NixOS:master May 18, 2021

samueldr reviewed May 21, 2021

View reviewed changes

samueldr mentioned this pull request May 21, 2021

README: move chat links mobile-nixos/mobile-nixos#358

Merged

pschyska mentioned this pull request May 21, 2021

nixos/atop: Add defaultText to types.package options, Fix timing-related test failures. #123926

Merged

9 tasks

pschyska added a commit to pschyska/nixpkgs that referenced this pull request May 22, 2021

nixos/atop: Add defaultText for types.package options

9cb76c2

see: NixOS#123053 (comment)

pschyska added a commit to pschyska/nixpkgs that referenced this pull request May 31, 2021

nixos/atop: Don't break backwards-compatibility by rendering atoprc i…

f2b4568

…f configured without the need to atop.enable see: NixOS#123053 (comment)

pschyska mentioned this pull request May 31, 2021

nixos/atop: Don't break backwards-compatibility by rendering atoprc in any case #125116

Closed

9 tasks

FRidh mentioned this pull request Apr 4, 2022

nixos tests atop: don't allow unfree packages #167163

Merged

13 tasks

Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

atop, netatop, nixos/atop: improve packaging and options #123053

atop, netatop, nixos/atop: improve packaging and options #123053

pschyska commented May 15, 2021 •

edited

Loading

r-rmcgibbo commented May 15, 2021 •

edited

Loading

pschyska commented May 16, 2021

pschyska commented May 16, 2021

7c6f434c commented May 16, 2021

7c6f434c left a comment

SuperSandro2000 May 16, 2021

pschyska May 16, 2021 •

edited

Loading

pschyska May 17, 2021

pschyska commented May 18, 2021

samueldr May 21, 2021 •

edited

Loading

pschyska May 21, 2021

pschyska May 21, 2021

samueldr May 21, 2021

Shados commented May 31, 2021

pschyska commented May 31, 2021

FRidh commented Apr 4, 2022

atop, netatop, nixos/atop: improve packaging and options #123053

atop, netatop, nixos/atop: improve packaging and options #123053

Conversation

pschyska commented May 15, 2021 • edited Loading

Motivation for this change

Things done

r-rmcgibbo commented May 15, 2021 • edited Loading

pschyska commented May 16, 2021

pschyska commented May 16, 2021

7c6f434c commented May 16, 2021

7c6f434c left a comment

Choose a reason for hiding this comment

SuperSandro2000 May 16, 2021

Choose a reason for hiding this comment

pschyska May 16, 2021 • edited Loading

Choose a reason for hiding this comment

pschyska May 17, 2021

Choose a reason for hiding this comment

pschyska commented May 18, 2021

samueldr May 21, 2021 • edited Loading

Choose a reason for hiding this comment

pschyska May 21, 2021

Choose a reason for hiding this comment

pschyska May 21, 2021

Choose a reason for hiding this comment

samueldr May 21, 2021

Choose a reason for hiding this comment

Shados commented May 31, 2021

pschyska commented May 31, 2021

FRidh commented Apr 4, 2022

pschyska commented May 15, 2021 •

edited

Loading

r-rmcgibbo commented May 15, 2021 •

edited

Loading

pschyska May 16, 2021 •

edited

Loading

samueldr May 21, 2021 •

edited

Loading