From 2703a7999adeee8e3c880fa539bdede43c0c22a4 Mon Sep 17 00:00:00 2001
From: Paul Schyska <paul@schyska.net>
Date: Fri, 14 May 2021 02:07:17 +0200
Subject: [PATCH 01/11] netatop: package netatop.service

---
 pkgs/os-specific/linux/netatop/default.nix | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/pkgs/os-specific/linux/netatop/default.nix b/pkgs/os-specific/linux/netatop/default.nix
index fb0a4eb7188..242fcbf681e 100644
--- a/pkgs/os-specific/linux/netatop/default.nix
+++ b/pkgs/os-specific/linux/netatop/default.nix
@@ -1,4 +1,4 @@
-{ lib, stdenv, fetchurl, kernel, zlib }:
+{ lib, stdenv, fetchurl, kernel, kmod, zlib }:
 
 let
   version = "3.1";
@@ -12,10 +12,11 @@ stdenv.mkDerivation {
     sha256 = "0qjw8glfdmngfvbn1w63q128vxdz2jlabw13y140ga9i5ibl6vvk";
   };
 
-  buildInputs = [ zlib ];
+  buildInputs = [ kmod zlib ];
 
   hardeningDisable = [ "pic" ];
 
+  patches = [ ./netatop.service.patch ];
   preConfigure = ''
     patchShebangs mkversion
     sed -i -e 's,^KERNDIR.*,KERNDIR=${kernel.dev}/lib/modules/${kernel.modDirVersion}/build,' \
@@ -24,12 +25,14 @@ stdenv.mkDerivation {
         -e s,/usr,$out, \
         -e /init.d/d \
         -e /depmod/d \
-        -e /netatop.service/d \
+        -e s,/lib/systemd,$out/lib/systemd, \
         Makefile
+
+    kmod=${kmod} substituteAllInPlace netatop.service
   '';
 
   preInstall = ''
-    mkdir -p $out/bin $out/sbin $out/share/man/man{4,8}
+    mkdir -p $out/lib/systemd/system $out/bin $out/sbin $out/share/man/man{4,8}
     mkdir -p $out/lib/modules/${kernel.modDirVersion}/extra
   '';
 
@@ -38,6 +41,6 @@ stdenv.mkDerivation {
     homepage = "https://www.atoptool.nl/downloadnetatop.php";
     license = lib.licenses.gpl2;
     platforms = lib.platforms.linux;
-    maintainers = with lib.maintainers; [viric];
+    maintainers = with lib.maintainers; [ viric ];
   };
 }

From 327dcea4ccc8e5e2562c277bd35198751ecc4930 Mon Sep 17 00:00:00 2001
From: Paul Schyska <paul@schyska.net>
Date: Fri, 14 May 2021 02:08:41 +0200
Subject: [PATCH 02/11] atop: fix packaging .service files, add option to build
 with atopgpu

---
 pkgs/os-specific/linux/atop/atop-pm.sh.patch  | 13 ++++
 .../linux/atop/atop-rotate.service.patch      |  7 +++
 .../os-specific/linux/atop/atop.service.patch | 14 +++++
 .../linux/atop/atopacct.service.patch         | 11 ++++
 .../linux/atop/atopgpu.service.patch          |  9 +++
 pkgs/os-specific/linux/atop/default.nix       | 59 ++++++++++++++-----
 .../linux/netatop/netatop.service.patch       | 14 +++++
 7 files changed, 113 insertions(+), 14 deletions(-)
 create mode 100644 pkgs/os-specific/linux/atop/atop-pm.sh.patch
 create mode 100644 pkgs/os-specific/linux/atop/atop-rotate.service.patch
 create mode 100644 pkgs/os-specific/linux/atop/atop.service.patch
 create mode 100644 pkgs/os-specific/linux/atop/atopacct.service.patch
 create mode 100644 pkgs/os-specific/linux/atop/atopgpu.service.patch
 create mode 100644 pkgs/os-specific/linux/netatop/netatop.service.patch

diff --git a/pkgs/os-specific/linux/atop/atop-pm.sh.patch b/pkgs/os-specific/linux/atop/atop-pm.sh.patch
new file mode 100644
index 00000000000..9b66b260992
--- /dev/null
+++ b/pkgs/os-specific/linux/atop/atop-pm.sh.patch
@@ -0,0 +1,13 @@
+--- a/atop-pm.sh
++++ b/atop-pm.sh
+@@ -2,8 +2,8 @@
+
+ case "$1" in
+-	pre)	/usr/bin/systemctl stop atop
++	pre)	@systemd@/bin/systemctl stop atop
+ 		exit 0
+ 		;;
+-	post)	/usr/bin/systemctl start atop
++	post)	@systemd@/bin/systemctl start atop
+ 		exit 0
+ 		;;
diff --git a/pkgs/os-specific/linux/atop/atop-rotate.service.patch b/pkgs/os-specific/linux/atop/atop-rotate.service.patch
new file mode 100644
index 00000000000..951a3c8c837
--- /dev/null
+++ b/pkgs/os-specific/linux/atop/atop-rotate.service.patch
@@ -0,0 +1,7 @@
+--- a/atop-rotate.service
++++ b/atop-rotate.service
+@@ -4,3 +4,3 @@
+ [Service]
+ Type=oneshot
+-ExecStart=/usr/bin/systemctl try-restart atop.service
++ExecStart=@systemd@/bin/systemctl try-restart atop.service
diff --git a/pkgs/os-specific/linux/atop/atop.service.patch b/pkgs/os-specific/linux/atop/atop.service.patch
new file mode 100644
index 00000000000..5467b512229
--- /dev/null
+++ b/pkgs/os-specific/linux/atop/atop.service.patch
@@ -0,0 +1,14 @@
+--- a/atop.service
++++ b/atop.service
+@@ -9,7 +9,8 @@
+ Environment=LOGPATH=/var/log/atop
+-EnvironmentFile=/etc/default/atop
++EnvironmentFile=-/etc/default/atop
+ ExecStartPre=/bin/sh -c 'test -n "$LOGINTERVAL" -a "$LOGINTERVAL" -eq "$LOGINTERVAL"'
+ ExecStartPre=/bin/sh -c 'test -n "$LOGGENERATIONS" -a "$LOGGENERATIONS" -eq "$LOGGENERATIONS"'
+-ExecStart=/bin/sh -c 'exec /usr/bin/atop ${LOGOPTS} -w "${LOGPATH}/atop_$(date +%%Y%%m%%d)" ${LOGINTERVAL}'
+-ExecStartPost=/usr/bin/find "${LOGPATH}" -name "atop_*" -mtime +${LOGGENERATIONS} -exec rm -v {} \;
++ExecStartPre=/bin/sh -c 'mkdir -p "${LOGPATH}"'
++ExecStart=/bin/sh -c 'exec @out@/bin/atop ${LOGOPTS} -w "${LOGPATH}/atop_$(date +%%Y%%m%%d)" ${LOGINTERVAL}'
++ExecStartPost=@findutils@/bin/find "${LOGPATH}" -name "atop_*" -mtime +${LOGGENERATIONS} -exec rm -v {} \;
+ KillSignal=SIGUSR2
diff --git a/pkgs/os-specific/linux/atop/atopacct.service.patch b/pkgs/os-specific/linux/atop/atopacct.service.patch
new file mode 100644
index 00000000000..cbecbef90ec
--- /dev/null
+++ b/pkgs/os-specific/linux/atop/atopacct.service.patch
@@ -0,0 +1,11 @@
+--- a/atopacct.service
++++ b/atopacct.service
+@@ -8,6 +8,6 @@
+ [Service]
+ Type=forking
+-PIDFile=/var/run/atopacctd.pid
+-ExecStart=/usr/sbin/atopacctd
++PIDFile=/run/atopacctd.pid
++ExecStart=@out@/bin/atopacctd
+
+ [Install]
diff --git a/pkgs/os-specific/linux/atop/atopgpu.service.patch b/pkgs/os-specific/linux/atop/atopgpu.service.patch
new file mode 100644
index 00000000000..30c38b67cad
--- /dev/null
+++ b/pkgs/os-specific/linux/atop/atopgpu.service.patch
@@ -0,0 +1,9 @@
+--- a/atopgpu.service
++++ b/atopgpu.service
+@@ -6,5 +6,5 @@
+
+ [Service]
+-ExecStart=/usr/sbin/atopgpud
++ExecStart=@out@/bin/atopgpud
+ Type=oneshot
+ RemainAfterExit=yes
diff --git a/pkgs/os-specific/linux/atop/default.nix b/pkgs/os-specific/linux/atop/default.nix
index e1b64c0a4b5..00510c91a74 100644
--- a/pkgs/os-specific/linux/atop/default.nix
+++ b/pkgs/os-specific/linux/atop/default.nix
@@ -1,4 +1,14 @@
-{lib, stdenv, fetchurl, zlib, ncurses}:
+{ lib
+, stdenv
+, fetchurl
+, zlib
+, ncurses
+, findutils
+, systemd
+, python3
+  # makes the package unfree via pynvml
+, withAtopgpu ? false
+}:
 
 stdenv.mkDerivation rec {
   pname = "atop";
@@ -9,31 +19,52 @@ stdenv.mkDerivation rec {
     sha256 = "nsLKOlcWkvfvqglfmaUQZDK8txzCLNbElZfvBIEFj3I=";
   };
 
-  buildInputs = [zlib ncurses];
+  nativeBuildInputs = if withAtopgpu then [ python3.pkgs.wrapPython ] else [ ];
+  buildInputs = [ zlib ncurses ] ++ (if withAtopgpu then [ python3 ] else [ ]);
+  pythonPath = if withAtopgpu then [ python3.pkgs.pynvml ] else [ ];
 
   makeFlags = [
-    "SCRPATH=$out/etc/atop"
-    "LOGPATH=/var/log/atop"
-    "INIPATH=$out/etc/rc.d/init.d"
-    "SYSDPATH=$out/lib/systemd/system"
-    "CRNPATH=$out/etc/cron.d"
-    "DEFPATH=$out/etc/default"
-    "ROTPATH=$out/etc/logrotate.d"
+    "DESTDIR=$(out)"
+    "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"
   ];
 
+  patches = [
+    ./atop-pm.sh.patch
+    ./atop-rotate.service.patch
+    ./atop.service.patch
+    ./atopacct.service.patch
+  ] ++ (if withAtopgpu then [ ./atopgpu.service.patch ] else [ ]);
+
   preConfigure = ''
-    sed -e "s@/usr/@$out/@g" -i $(find . -type f )
-    sed -e "/mkdir.*LOGPATH/s@mkdir@echo missing dir @" -i Makefile
-    sed -e "/touch.*LOGPATH/s@touch@echo should have created @" -i Makefile
+    for f in *.{sh,service}; do
+      findutils=${findutils} systemd=${systemd} substituteAllInPlace "$f"
+    done
+
     sed -e 's/chown/true/g' -i Makefile
-    sed -e '/chkconfig/d' -i Makefile
     sed -e 's/chmod 04711/chmod 0711/g' -i Makefile
   '';
 
   installTargets = [ "systemdinstall" ];
   preInstall = ''
-    mkdir -p "$out"/{bin,sbin}
+    mkdir -p $out/bin
   '';
+  postInstall = ''
+    # remove extra files we don't need
+    rm -rf $out/{var,etc}
+    rm -rf $out/bin/atop{sar,}-${version}
+  '' + (if withAtopgpu then ''
+    wrapPythonPrograms
+  '' else ''
+    rm $out/lib/systemd/system/atopgpu.service
+    rm $out/bin/atopgpud
+    rm $out/share/man/man8/atopgpud.8
+  '');
 
   meta = with lib; {
     platforms = platforms.linux;
diff --git a/pkgs/os-specific/linux/netatop/netatop.service.patch b/pkgs/os-specific/linux/netatop/netatop.service.patch
new file mode 100644
index 00000000000..b7fae336be9
--- /dev/null
+++ b/pkgs/os-specific/linux/netatop/netatop.service.patch
@@ -0,0 +1,14 @@
+--- a/netatop.service
++++ b/netatop.service
+@@ -7,8 +7,8 @@
+ [Service]
+ Type=oneshot
+-ExecStartPre=/sbin/modprobe netatop
+-ExecStart=/usr/sbin/netatopd
+-ExecStopPost=/sbin/rmmod netatop
+-PIDFile=/var/run/netatop.pid
++ExecStartPre=@kmod@/bin/modprobe netatop
++ExecStart=@out@/bin/netatopd
++ExecStopPost=@kmod@/bin/rmmod netatop
++PIDFile=/run/netatop.pid
+ RemainAfterExit=yes

From 8f3d2e5c3b10ca459a71a59176be9be16d75276f Mon Sep 17 00:00:00 2001
From: Paul Schyska <paul@schyska.net>
Date: Fri, 14 May 2021 02:11:05 +0200
Subject: [PATCH 03/11] nixos/atop: Add configuration for atop services, allow
 to enable netatop, gpuatop, allow setuid wrapper

---
 nixos/modules/programs/atop.nix | 126 ++++++++++++++++++++++++++++--
 nixos/tests/all-tests.nix       |   1 +
 nixos/tests/atop.nix            | 132 ++++++++++++++++++++++++++++++++
 3 files changed, 251 insertions(+), 8 deletions(-)
 create mode 100644 nixos/tests/atop.nix

diff --git a/nixos/modules/programs/atop.nix b/nixos/modules/programs/atop.nix
index 7ef8d687ca1..d1577b32adf 100644
--- a/nixos/modules/programs/atop.nix
+++ b/nixos/modules/programs/atop.nix
@@ -1,6 +1,6 @@
 # Global configuration for atop.
 
-{ config, lib, ... }:
+{ config, lib, pkgs, ... }:
 
 with lib;
 
@@ -12,11 +12,82 @@ in
 
   options = {
 
-    programs.atop = {
+    programs.atop = rec {
 
+      enable = mkEnableOption "Atop";
+
+      package = mkOption {
+        type = types.package;
+        default = pkgs.atop;
+        description = ''
+          Which package to use for Atop.
+        '';
+      };
+
+      netatop = {
+        enable = mkOption {
+          type = types.bool;
+          default = false;
+          description = ''
+            Whether to install and enable the netatop kernel module.
+          '';
+        };
+        package = mkOption {
+          type = types.package;
+          default = config.boot.kernelPackages.netatop;
+          description = ''
+            Which package to use for netatop.
+          '';
+        };
+      };
+
+      atopgpu.enable = mkOption {
+        type = types.bool;
+        default = false;
+        description = ''
+          Whether to install and enable the atopgpud daemon to get information about
+          NVIDIA gpus.
+        '';
+      };
+
+      setuidWrapper.enable = mkOption {
+        type = types.bool;
+        default = cfg.netatop.enable || cfg.atopgpu.enable;
+        description = ''
+          Whether to install a setuid wrapper for Atop. This is required to use some of
+          the features as non-root user (e.g.: ipc information, netatop, atopgpu).
+          Atop tries to drop the root privileges shortly after starting.
+        '';
+      };
+
+      atopsvc.enable = mkOption {
+        type = types.bool;
+        default = true;
+        description = ''
+          Whether to enable the atop service responsible for storing statistics for
+          long-term analysis.
+        '';
+      };
+      atopRotate.enable = mkOption {
+        type = types.bool;
+        default = true;
+        description = ''
+          Whether to enable the atop-rotate timer, which restarts the atop service
+          daily to make sure the data files are rotate.
+        '';
+      };
+      atopacct.enable = mkOption {
+        type = types.bool;
+        default = true;
+        description = ''
+          Whether to enable the atopacct service which manages process accounting.
+          This allows Atop to gather data about processes that disappeared in between
+          two refresh intervals.
+        '';
+      };
       settings = mkOption {
         type = types.attrs;
-        default = {};
+        default = { };
         example = {
           flags = "a1f";
           interval = 5;
@@ -25,12 +96,51 @@ in
           Parameters to be written to <filename>/etc/atoprc</filename>.
         '';
       };
-
     };
   };
 
-  config = mkIf (cfg.settings != {}) {
-    environment.etc.atoprc.text =
-      concatStrings (mapAttrsToList (n: v: "${n} ${toString v}\n") cfg.settings);
-  };
+  config = mkIf cfg.enable (
+    let
+      atop =
+        if cfg.atopgpu.enable then
+          (cfg.package.override { withAtopgpu = true; })
+        else
+          cfg.package;
+      packages = [ atop (lib.mkIf cfg.netatop.enable cfg.netatop.package) ];
+    in
+    {
+      environment.etc = mkIf (cfg.settings != { }) {
+        atoprc.text = concatStrings
+          (mapAttrsToList
+            (n: v: ''
+              ${n} ${toString v}
+            '')
+            cfg.settings);
+      };
+      environment.systemPackages = packages;
+      boot.extraModulePackages = [ (lib.mkIf cfg.netatop.enable cfg.netatop.package) ];
+      systemd =
+        let
+          mkSystemd = type: cond: name: {
+            ${name} = lib.mkIf cond {
+              restartTriggers = packages;
+              wantedBy = [ (if type == "services" then "multi-user.target" else if type == "timers" then "timers.target" else null) ];
+            };
+          };
+          mkService = mkSystemd "services";
+          mkTimer = mkSystemd "timers";
+        in
+        {
+          inherit packages;
+          services =
+            mkService cfg.atopsvc.enable "atop"
+            // mkService cfg.atopacct.enable "atopacct"
+            // mkService cfg.netatop.enable "netatop"
+            // mkService cfg.atopgpu.enable "atopgpu";
+          timers = mkTimer cfg.atopRotate.enable "atop-rotate";
+        };
+      security.wrappers =
+        lib.mkIf cfg.setuidWrapper.enable { atop = { source = "${atop}/bin/atop"; }; };
+    }
+  );
 }
diff --git a/nixos/tests/all-tests.nix b/nixos/tests/all-tests.nix
index 232d89052d4..e468c0da64c 100644
--- a/nixos/tests/all-tests.nix
+++ b/nixos/tests/all-tests.nix
@@ -28,6 +28,7 @@ in
   amazon-init-shell = handleTest ./amazon-init-shell.nix {};
   ammonite = handleTest ./ammonite.nix {};
   atd = handleTest ./atd.nix {};
+  atop = handleTest ./atop.nix {};
   avahi = handleTest ./avahi.nix {};
   avahi-with-resolved = handleTest ./avahi.nix { networkd = true; };
   awscli = handleTest ./awscli.nix { };
diff --git a/nixos/tests/atop.nix b/nixos/tests/atop.nix
new file mode 100644
index 00000000000..8cecf02d28f
--- /dev/null
+++ b/nixos/tests/atop.nix
@@ -0,0 +1,132 @@
+import ./make-test-python.nix ({ pkgs, ... }: {
+  name = "atop";
+
+  nodes = {
+    defaults = { ... }: {
+      programs.atop = {
+        enable = true;
+      };
+    };
+    minimal = { ... }: {
+      programs.atop = {
+        enable = true;
+        atopsvc.enable = false;
+        atopRotate.enable = false;
+        atopacct.enable = false;
+      };
+    };
+    minimal_with_setuid = { ... }: {
+      programs.atop = {
+        enable = true;
+        atopsvc.enable = false;
+        atopRotate.enable = false;
+        atopacct.enable = false;
+        setuidWrapper.enable = true;
+      };
+    };
+
+    atoprc_and_netatop = { ... }: {
+      programs.atop = {
+        enable = true;
+        netatop.enable = true;
+        settings = {
+          flags = "faf1";
+          interval = 2;
+        };
+      };
+    };
+
+    atopgpu = { lib, ... }: {
+      nixpkgs.config.allowUnfreePredicate = pkg: builtins.elem (lib.getName pkg) [
+        "cudatoolkit"
+      ];
+      programs.atop = {
+        enable = true;
+        atopgpu.enable = true;
+      };
+    };
+  };
+
+  testScript = ''
+    def a_version(m):
+      v = m.succeed("atop -V")
+      pkgver = "${pkgs.atop.version}"
+      assert v.startswith("Version: {}".format(pkgver)), "Version is {}, expected `{}`".format(v, pkgver)
+
+    def __exp_path(m, prg, expected):
+      p = m.succeed("type -p \"{}\" | head -c -1".format(prg))
+      assert p == expected, "{} is `{}`, expected `{}`".format(prg, p, expected)
+
+    def a_setuid(m, present=True):
+      if present:
+        __exp_path(m, "atop", "/run/wrappers/bin/atop")
+        stat = m.succeed("stat --printf '%a %u' /run/wrappers/bin/atop")
+        assert stat == "4511 0", "Wrapper stat is {}, expected `4511 0`".format(stat)
+      else:
+        __exp_path(m, "atop", "/run/current-system/sw/bin/atop")
+
+    def assert_no_netatop(m):
+      m.require_unit_state("netatop.service", "inactive")
+      m.fail("modprobe -n -v netatop")
+
+    def a_netatop(m, present=True):
+      m.require_unit_state("netatop.service", "active" if present else "inactive")
+      if present:
+        out = m.succeed("modprobe -n -v netatop")
+        assert out == "", "Module should be loaded, but modprobe would have done `{}`.".format(out)
+      else:
+        m.fail("modprobe -n -v netatop")
+
+    def a_atopgpu(m, present=True):
+      m.require_unit_state("atopgpu.service", "active" if present else "inactive")
+      if present:
+        __exp_path(m, "atopgpud", "/run/current-system/sw/bin/atopgpud")
+
+    # atop.service should log some data to /var/log/atop
+    def a_atopsvc(m, present=True):
+      m.require_unit_state("atop.service", "active" if present else "inactive")
+      if present:
+          files = int(m.succeed("ls -1 /var/log/atop | wc -l"))
+          assert files >= 1, "Expected at least 1 data file"
+        # def check_files(_):
+        #   files = int(m.succeed("ls -1 /var/log/atop | wc -l"))
+        #   return files >= 1
+        # retry(check_files)
+
+    def a_atoprotate(m, present=True):
+      m.require_unit_state("atop-rotate.timer", "active" if present else "inactive")
+
+    # atopacct.service should make kernel write to /run/pacct_source and make dir
+    # /run/pacct_shadow.d
+    def a_atopacct(m, present=True):
+      m.require_unit_state("atopacct.service", "active" if present else "inactive")
+      if present:
+        m.succeed("test -f /run/pacct_source")
+        files = int(m.succeed("ls -1 /run/pacct_shadow.d | wc -l"))
+        assert files >= 1, "Expected at least 1 pacct_shadow.d file"
+
+    def a_atoprc(m, contents):
+      if contents:
+        f = m.succeed("cat /etc/atoprc")
+        assert f == contents, "/etc/atoprc contents: `{}`, expected `{}`".format(f, contents)
+      else:
+        m.succeed("test ! -e /etc/atoprc")
+
+    def assert_all(m, setuid, atopsvc, atoprotate, atopacct, netatop, atopgpu, atoprc):
+      a_version(m)
+      a_setuid(m, setuid)
+      a_atopsvc(m, atopsvc)
+      a_atoprotate(m, atoprotate)
+      a_atopacct(m, atopacct)
+      a_netatop(m, netatop)
+      a_atopgpu(m, atopgpu)
+      a_atoprc(m, atoprc)
+
+    assert_all(defaults, False, True, True, True, False, False, False)
+    assert_all(minimal, False, False, False, False, False, False, False)
+    assert_all(minimal_with_setuid, True, False, False, False, False, False, False)
+    assert_all(atoprc_and_netatop, False, True, True, True, True, False,
+      "flags faf1\ninterval 2\n")
+    assert_all(atopgpu, False, True, True, True, False, True, False)
+  '';
+})

From b87c36604642669d05381f06ac94af1b21f1ce30 Mon Sep 17 00:00:00 2001
From: Paul Schyska <paul@schyska.net>
Date: Sun, 16 May 2021 18:19:27 +0200
Subject: [PATCH 04/11] nixos/atop: Never enable setuidWrapper by default,
 rename service/timer enabling options

---
 nixos/modules/programs/atop.nix | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/nixos/modules/programs/atop.nix b/nixos/modules/programs/atop.nix
index d1577b32adf..3e69e882eae 100644
--- a/nixos/modules/programs/atop.nix
+++ b/nixos/modules/programs/atop.nix
@@ -52,7 +52,7 @@ in
 
       setuidWrapper.enable = mkOption {
         type = types.bool;
-        default = cfg.netatop.enable || cfg.atopgpu.enable;
+        default = false;
         description = ''
           Whether to install a setuid wrapper for Atop. This is required to use some of
           the features as non-root user (e.g.: ipc information, netatop, atopgpu).
@@ -60,7 +60,7 @@ in
         '';
       };
 
-      atopsvc.enable = mkOption {
+      atopService.enable = mkOption {
         type = types.bool;
         default = true;
         description = ''
@@ -68,7 +68,7 @@ in
           long-term analysis.
         '';
       };
-      atopRotate.enable = mkOption {
+      atopRotateTimer.enable = mkOption {
         type = types.bool;
         default = true;
         description = ''
@@ -76,7 +76,7 @@ in
           daily to make sure the data files are rotate.
         '';
       };
-      atopacct.enable = mkOption {
+      atopacctService.enable = mkOption {
         type = types.bool;
         default = true;
         description = ''
@@ -133,11 +133,11 @@ in
         {
           inherit packages;
           services =
-            mkService cfg.atopsvc.enable "atop"
-            // mkService cfg.atopacct.enable "atopacct"
+            mkService cfg.atopService.enable "atop"
+            // mkService cfg.atopacctService.enable "atopacct"
             // mkService cfg.netatop.enable "netatop"
             // mkService cfg.atopgpu.enable "atopgpu";
-          timers = mkTimer cfg.atopRotate.enable "atop-rotate";
+          timers = mkTimer cfg.atopRotateTimer.enable "atop-rotate";
         };
       security.wrappers =
         lib.mkIf cfg.setuidWrapper.enable { atop = { source = "${atop}/bin/atop"; }; };

From fb90a9c552fa422020ea649ddcc8a9132fb2ee7e Mon Sep 17 00:00:00 2001
From: Paul Schyska <paul@schyska.net>
Date: Sun, 16 May 2021 18:20:03 +0200
Subject: [PATCH 05/11] nixos/atop: Rework the test

- 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
---
 nixos/tests/atop.nix | 297 +++++++++++++++++++++++++++----------------
 1 file changed, 189 insertions(+), 108 deletions(-)

diff --git a/nixos/tests/atop.nix b/nixos/tests/atop.nix
index 8cecf02d28f..a2565e610ae 100644
--- a/nixos/tests/atop.nix
+++ b/nixos/tests/atop.nix
@@ -1,132 +1,213 @@
-import ./make-test-python.nix ({ pkgs, ... }: {
+{ system ? builtins.currentSystem
+, config ? { }
+, pkgs ? import ../.. { inherit system config; }
+}:
+
+with import ../lib/testing-python.nix { inherit system pkgs; };
+with pkgs.lib;
+
+let assertions = rec {
+  path = program: path: ''
+    with subtest("The path of ${program} should be ${path}"):
+        p = machine.succeed("type -p \"${program}\" | head -c -1")
+        assert p == "${path}", f"${program} is {p}, expected ${path}"
+  '';
+  unit = name: state: ''
+    with subtest("Unit ${name} should be ${state}"):
+        machine.require_unit_state("${name}", "${state}")
+  '';
+  version = ''
+    import re
+
+    with subtest("binary should report the correct version"):
+        pkgver = "${pkgs.atop.version}"
+        ver = re.sub(r'(?s)^Version: (\d\.\d\.\d).*', r'\1', machine.succeed("atop -V"))
+        assert ver == pkgver, f"Version is `{ver}`, expected `{pkgver}`"
+  '';
+  atoprc = contents:
+    if builtins.stringLength contents > 0 then ''
+      with subtest("/etc/atoprc should have the correct contents"):
+          f = machine.succeed("cat /etc/atoprc")
+          assert f == "${contents}", f"/etc/atoprc contents: '{f}', expected '${contents}'"
+    '' else ''
+      with subtest("/etc/atoprc should not be present"):
+          machine.succeed("test ! -e /etc/atoprc")
+    '';
+  wrapper = present:
+    if present then path "atop" "/run/wrappers/bin/atop" + ''
+      with subtest("Wrapper should be setuid root"):
+          stat = machine.succeed("stat --printf '%a %u' /run/wrappers/bin/atop")
+          assert stat == "4511 0", f"Wrapper stat is {stat}, expected '4511 0'"
+    ''
+    else path "atop" "/run/current-system/sw/bin/atop";
+  atopService = present:
+    if present then
+      unit "atop.service" "active"
+      + ''
+        with subtest("atop.service should have written some data to /var/log/atop"):
+            files = int(machine.succeed("ls -1 /var/log/atop | wc -l"))
+            assert files > 0, "Expected at least 1 data file"
+      '' else unit "atop.service" "inactive";
+  atopRotateTimer = present:
+    unit "atop-rotate.timer" (if present then "active" else "inactive");
+  atopacctService = present:
+    if present then
+      unit "atopacct.service" "active"
+      + ''
+        with subtest("atopacct.service should enable process accounting"):
+            machine.succeed("test -f /run/pacct_source")
+
+        with subtest("atopacct.service should write data to /run/pacct_shadow.d"):
+            files = int(machine.succeed("ls -1 /run/pacct_shadow.d | wc -l"))
+            assert files >= 1, "Expected at least 1 pacct_shadow.d file"
+      '' else unit "atopacct.service" "inactive";
+  netatop = present:
+    if present then
+      unit "netatop.service" "active"
+      + ''
+        with subtest("The netatop kernel module should be loaded"):
+            out = machine.succeed("modprobe -n -v netatop")
+            assert out == "", f"Module should be loaded already, but modprobe would have done {out}."
+      '' else ''
+      with subtest("The netatop kernel module should be absent"):
+          machine.fail("modprobe -n -v netatop")
+    '';
+  atopgpu = present:
+    if present then
+      (unit "atopgpu.service" "active") + (path "atopgpud" "/run/current-system/sw/bin/atopgpud")
+    else (unit "atopgpu.service" "inactive") + ''
+      with subtest("atopgpud should not be present"):
+          machine.fail("type -p atopgpud")
+    '';
+};
+in
+{
   name = "atop";
 
-  nodes = {
-    defaults = { ... }: {
+  justThePackage = makeTest {
+    name = "atop-justThePackage";
+    machine = {
+      environment.systemPackages = [ pkgs.atop ];
+    };
+    testScript = with assertions; builtins.concatStringsSep "\n" [
+      version
+      (atoprc "")
+      (wrapper false)
+      (atopService false)
+      (atopRotateTimer false)
+      (atopacctService false)
+      (netatop false)
+      (atopgpu false)
+    ];
+  };
+  defaults = makeTest {
+    name = "atop-defaults";
+    machine = {
       programs.atop = {
         enable = true;
       };
     };
-    minimal = { ... }: {
+    testScript = with assertions; builtins.concatStringsSep "\n" [
+      version
+      (atoprc "")
+      (wrapper false)
+      (atopService true)
+      (atopRotateTimer true)
+      (atopacctService true)
+      (netatop false)
+      (atopgpu false)
+    ];
+  };
+  minimal = makeTest {
+    name = "atop-minimal";
+    machine = {
       programs.atop = {
         enable = true;
-        atopsvc.enable = false;
-        atopRotate.enable = false;
-        atopacct.enable = false;
+        atopService.enable = false;
+        atopRotateTimer.enable = false;
+        atopacctService.enable = false;
       };
     };
-    minimal_with_setuid = { ... }: {
-      programs.atop = {
-        enable = true;
-        atopsvc.enable = false;
-        atopRotate.enable = false;
-        atopacct.enable = false;
-        setuidWrapper.enable = true;
-      };
-    };
-
-    atoprc_and_netatop = { ... }: {
+    testScript = with assertions; builtins.concatStringsSep "\n" [
+      version
+      (atoprc "")
+      (wrapper false)
+      (atopService false)
+      (atopRotateTimer false)
+      (atopacctService false)
+      (netatop false)
+      (atopgpu false)
+    ];
+  };
+  netatop = makeTest {
+    name = "atop-netatop";
+    machine = {
       programs.atop = {
         enable = true;
         netatop.enable = true;
-        settings = {
-          flags = "faf1";
-          interval = 2;
-        };
       };
     };
-
-    atopgpu = { lib, ... }: {
-      nixpkgs.config.allowUnfreePredicate = pkg: builtins.elem (lib.getName pkg) [
+    testScript = with assertions; builtins.concatStringsSep "\n" [
+      version
+      (atoprc "")
+      (wrapper false)
+      (atopService true)
+      (atopRotateTimer true)
+      (atopacctService true)
+      (netatop true)
+      (atopgpu false)
+    ];
+  };
+  atopgpu = makeTest {
+    name = "atop-atopgpu";
+    machine = {
+      nixpkgs.config.allowUnfreePredicate = pkg: builtins.elem (getName pkg) [
         "cudatoolkit"
       ];
+
       programs.atop = {
         enable = true;
         atopgpu.enable = true;
       };
     };
+    testScript = with assertions; builtins.concatStringsSep "\n" [
+      version
+      (atoprc "")
+      (wrapper false)
+      (atopService true)
+      (atopRotateTimer true)
+      (atopacctService true)
+      (netatop false)
+      (atopgpu true)
+    ];
   };
+  everything = makeTest {
+    name = "atop-everthing";
+    machine = {
+      nixpkgs.config.allowUnfreePredicate = pkg: builtins.elem (getName pkg) [
+        "cudatoolkit"
+      ];
 
-  testScript = ''
-    def a_version(m):
-      v = m.succeed("atop -V")
-      pkgver = "${pkgs.atop.version}"
-      assert v.startswith("Version: {}".format(pkgver)), "Version is {}, expected `{}`".format(v, pkgver)
-
-    def __exp_path(m, prg, expected):
-      p = m.succeed("type -p \"{}\" | head -c -1".format(prg))
-      assert p == expected, "{} is `{}`, expected `{}`".format(prg, p, expected)
-
-    def a_setuid(m, present=True):
-      if present:
-        __exp_path(m, "atop", "/run/wrappers/bin/atop")
-        stat = m.succeed("stat --printf '%a %u' /run/wrappers/bin/atop")
-        assert stat == "4511 0", "Wrapper stat is {}, expected `4511 0`".format(stat)
-      else:
-        __exp_path(m, "atop", "/run/current-system/sw/bin/atop")
-
-    def assert_no_netatop(m):
-      m.require_unit_state("netatop.service", "inactive")
-      m.fail("modprobe -n -v netatop")
-
-    def a_netatop(m, present=True):
-      m.require_unit_state("netatop.service", "active" if present else "inactive")
-      if present:
-        out = m.succeed("modprobe -n -v netatop")
-        assert out == "", "Module should be loaded, but modprobe would have done `{}`.".format(out)
-      else:
-        m.fail("modprobe -n -v netatop")
-
-    def a_atopgpu(m, present=True):
-      m.require_unit_state("atopgpu.service", "active" if present else "inactive")
-      if present:
-        __exp_path(m, "atopgpud", "/run/current-system/sw/bin/atopgpud")
-
-    # atop.service should log some data to /var/log/atop
-    def a_atopsvc(m, present=True):
-      m.require_unit_state("atop.service", "active" if present else "inactive")
-      if present:
-          files = int(m.succeed("ls -1 /var/log/atop | wc -l"))
-          assert files >= 1, "Expected at least 1 data file"
-        # def check_files(_):
-        #   files = int(m.succeed("ls -1 /var/log/atop | wc -l"))
-        #   return files >= 1
-        # retry(check_files)
-
-    def a_atoprotate(m, present=True):
-      m.require_unit_state("atop-rotate.timer", "active" if present else "inactive")
-
-    # atopacct.service should make kernel write to /run/pacct_source and make dir
-    # /run/pacct_shadow.d
-    def a_atopacct(m, present=True):
-      m.require_unit_state("atopacct.service", "active" if present else "inactive")
-      if present:
-        m.succeed("test -f /run/pacct_source")
-        files = int(m.succeed("ls -1 /run/pacct_shadow.d | wc -l"))
-        assert files >= 1, "Expected at least 1 pacct_shadow.d file"
-
-    def a_atoprc(m, contents):
-      if contents:
-        f = m.succeed("cat /etc/atoprc")
-        assert f == contents, "/etc/atoprc contents: `{}`, expected `{}`".format(f, contents)
-      else:
-        m.succeed("test ! -e /etc/atoprc")
-
-    def assert_all(m, setuid, atopsvc, atoprotate, atopacct, netatop, atopgpu, atoprc):
-      a_version(m)
-      a_setuid(m, setuid)
-      a_atopsvc(m, atopsvc)
-      a_atoprotate(m, atoprotate)
-      a_atopacct(m, atopacct)
-      a_netatop(m, netatop)
-      a_atopgpu(m, atopgpu)
-      a_atoprc(m, atoprc)
-
-    assert_all(defaults, False, True, True, True, False, False, False)
-    assert_all(minimal, False, False, False, False, False, False, False)
-    assert_all(minimal_with_setuid, True, False, False, False, False, False, False)
-    assert_all(atoprc_and_netatop, False, True, True, True, True, False,
-      "flags faf1\ninterval 2\n")
-    assert_all(atopgpu, False, True, True, True, False, True, False)
-  '';
-})
+      programs.atop = {
+        enable = true;
+        settings = {
+          flags = "faf1";
+          interval = 2;
+        };
+        setuidWrapper.enable = true;
+        netatop.enable = true;
+        atopgpu.enable = true;
+      };
+    };
+    testScript = with assertions; builtins.concatStringsSep "\n" [
+      version
+      (atoprc "flags faf1\\ninterval 2\\n")
+      (wrapper true)
+      (atopService true)
+      (atopRotateTimer true)
+      (atopacctService true)
+      (netatop true)
+      (atopgpu true)
+    ];
+  };
+}

From 526bc6a4d59d87b5fd90984f0aee607620036962 Mon Sep 17 00:00:00 2001
From: Paul Schyska <paul@schyska.net>
Date: Sun, 16 May 2021 21:43:20 +0200
Subject: [PATCH 06/11] nixos/atop: Add a note about netatop tainting the
 kernel

---
 nixos/modules/programs/atop.nix | 1 +
 1 file changed, 1 insertion(+)

diff --git a/nixos/modules/programs/atop.nix b/nixos/modules/programs/atop.nix
index 3e69e882eae..2010254a35d 100644
--- a/nixos/modules/programs/atop.nix
+++ b/nixos/modules/programs/atop.nix
@@ -30,6 +30,7 @@ in
           default = false;
           description = ''
             Whether to install and enable the netatop kernel module.
+            Note: this sets the kernel taint flag "O" for loading out-of-tree modules.
           '';
         };
         package = mkOption {

From 563ba07543cb900eee5681a65259dd6c3533248b Mon Sep 17 00:00:00 2001
From: Paul Schyska <paul@schyska.net>
Date: Sun, 16 May 2021 22:00:24 +0200
Subject: [PATCH 07/11] nixos/atop: Split up restart triggers between atop and
 netatop

---
 nixos/modules/programs/atop.nix | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/nixos/modules/programs/atop.nix b/nixos/modules/programs/atop.nix
index 2010254a35d..47597a0a4d4 100644
--- a/nixos/modules/programs/atop.nix
+++ b/nixos/modules/programs/atop.nix
@@ -107,7 +107,6 @@ in
           (cfg.package.override { withAtopgpu = true; })
         else
           cfg.package;
-      packages = [ atop (lib.mkIf cfg.netatop.enable cfg.netatop.package) ];
     in
     {
       environment.etc = mkIf (cfg.settings != { }) {
@@ -118,13 +117,13 @@ in
             '')
             cfg.settings);
       };
-      environment.systemPackages = packages;
+      environment.systemPackages = [ atop (lib.mkIf cfg.netatop.enable cfg.netatop.package) ];
       boot.extraModulePackages = [ (lib.mkIf cfg.netatop.enable cfg.netatop.package) ];
       systemd =
         let
-          mkSystemd = type: cond: name: {
+          mkSystemd = type: cond: name: restartTriggers: {
             ${name} = lib.mkIf cond {
-              restartTriggers = packages;
+              inherit restartTriggers;
               wantedBy = [ (if type == "services" then "multi-user.target" else if type == "timers" then "timers.target" else null) ];
             };
           };
@@ -132,13 +131,13 @@ in
           mkTimer = mkSystemd "timers";
         in
         {
-          inherit packages;
+          packages = [ atop (lib.mkIf cfg.netatop.enable cfg.netatop.package) ];
           services =
-            mkService cfg.atopService.enable "atop"
-            // mkService cfg.atopacctService.enable "atopacct"
-            // mkService cfg.netatop.enable "netatop"
-            // mkService cfg.atopgpu.enable "atopgpu";
-          timers = mkTimer cfg.atopRotateTimer.enable "atop-rotate";
+            mkService cfg.atopService.enable "atop" [ atop ]
+            // mkService cfg.atopacctService.enable "atopacct" [ atop ]
+            // mkService cfg.netatop.enable "netatop" [ cfg.netatop.package ]
+            // mkService cfg.atopgpu.enable "atopgpu" [ atop ];
+          timers = mkTimer cfg.atopRotateTimer.enable "atop-rotate" [ atop ];
         };
       security.wrappers =
         lib.mkIf cfg.setuidWrapper.enable { atop = { source = "${atop}/bin/atop"; }; };

From 27d323d2813bf44473cb7a4f2d5e0f4acc84c5ff Mon Sep 17 00:00:00 2001
From: Paul Schyska <paul@schyska.net>
Date: Sun, 16 May 2021 22:07:24 +0200
Subject: [PATCH 08/11] netatop: Split out path fixing and PIDFile changes to
 separate patches and comment

---
 pkgs/os-specific/linux/netatop/default.nix           |  7 ++++++-
 pkgs/os-specific/linux/netatop/fix-paths.patch       | 11 +++++++++++
 pkgs/os-specific/linux/netatop/netatop.service.patch | 11 ++---------
 3 files changed, 19 insertions(+), 10 deletions(-)
 create mode 100644 pkgs/os-specific/linux/netatop/fix-paths.patch

diff --git a/pkgs/os-specific/linux/netatop/default.nix b/pkgs/os-specific/linux/netatop/default.nix
index 242fcbf681e..28f989929a4 100644
--- a/pkgs/os-specific/linux/netatop/default.nix
+++ b/pkgs/os-specific/linux/netatop/default.nix
@@ -16,7 +16,12 @@ stdenv.mkDerivation {
 
   hardeningDisable = [ "pic" ];
 
-  patches = [ ./netatop.service.patch ];
+  patches = [
+    # fix paths in netatop.service
+    ./fix-paths.patch
+    # Specify PIDFile in /run, not /var/run to silence systemd warning
+    ./netatop.service.patch
+  ];
   preConfigure = ''
     patchShebangs mkversion
     sed -i -e 's,^KERNDIR.*,KERNDIR=${kernel.dev}/lib/modules/${kernel.modDirVersion}/build,' \
diff --git a/pkgs/os-specific/linux/netatop/fix-paths.patch b/pkgs/os-specific/linux/netatop/fix-paths.patch
new file mode 100644
index 00000000000..0e71c4efdd3
--- /dev/null
+++ b/pkgs/os-specific/linux/netatop/fix-paths.patch
@@ -0,0 +1,11 @@
+--- a/netatop.service
++++ b/netatop.service
+@@ -8,5 +8,5 @@
+ Type=oneshot
+-ExecStartPre=/sbin/modprobe netatop
+-ExecStart=/usr/sbin/netatopd
+-ExecStopPost=/sbin/rmmod netatop
++ExecStartPre=@kmod@/bin/modprobe netatop
++ExecStart=@out@/bin/netatopd
++ExecStopPost=@kmod@/bin/rmmod netatop
+ PIDFile=/var/run/netatop.pid
diff --git a/pkgs/os-specific/linux/netatop/netatop.service.patch b/pkgs/os-specific/linux/netatop/netatop.service.patch
index b7fae336be9..c7c798ee06b 100644
--- a/pkgs/os-specific/linux/netatop/netatop.service.patch
+++ b/pkgs/os-specific/linux/netatop/netatop.service.patch
@@ -1,14 +1,7 @@
 --- a/netatop.service
 +++ b/netatop.service
-@@ -7,8 +7,8 @@
- [Service]
- Type=oneshot
--ExecStartPre=/sbin/modprobe netatop
--ExecStart=/usr/sbin/netatopd
--ExecStopPost=/sbin/rmmod netatop
+@@ -11,3 +11,3 @@
+ ExecStopPost=@kmod@/bin/rmmod netatop
 -PIDFile=/var/run/netatop.pid
-+ExecStartPre=@kmod@/bin/modprobe netatop
-+ExecStart=@out@/bin/netatopd
-+ExecStopPost=@kmod@/bin/rmmod netatop
 +PIDFile=/run/netatop.pid
  RemainAfterExit=yes

From 76554beed86e6935fefee25cdd8c78bf524aac4c Mon Sep 17 00:00:00 2001
From: Paul Schyska <paul@schyska.net>
Date: Sun, 16 May 2021 22:08:05 +0200
Subject: [PATCH 09/11] atop: Merge all path-fixing to a single patch, comment
 the remaining patches

---
 pkgs/os-specific/linux/atop/atop-pm.sh.patch  | 13 -----
 .../linux/atop/atop-rotate.service.patch      |  7 ---
 .../os-specific/linux/atop/atop.service.patch |  8 +---
 .../linux/atop/atopacct.service.patch         |  8 +---
 .../linux/atop/atopgpu.service.patch          |  9 ----
 pkgs/os-specific/linux/atop/default.nix       |  9 ++--
 pkgs/os-specific/linux/atop/fix-paths.patch   | 48 +++++++++++++++++++
 7 files changed, 58 insertions(+), 44 deletions(-)
 delete mode 100644 pkgs/os-specific/linux/atop/atop-pm.sh.patch
 delete mode 100644 pkgs/os-specific/linux/atop/atop-rotate.service.patch
 delete mode 100644 pkgs/os-specific/linux/atop/atopgpu.service.patch
 create mode 100644 pkgs/os-specific/linux/atop/fix-paths.patch

diff --git a/pkgs/os-specific/linux/atop/atop-pm.sh.patch b/pkgs/os-specific/linux/atop/atop-pm.sh.patch
deleted file mode 100644
index 9b66b260992..00000000000
--- a/pkgs/os-specific/linux/atop/atop-pm.sh.patch
+++ /dev/null
@@ -1,13 +0,0 @@
---- a/atop-pm.sh
-+++ b/atop-pm.sh
-@@ -2,8 +2,8 @@
-
- case "$1" in
--	pre)	/usr/bin/systemctl stop atop
-+	pre)	@systemd@/bin/systemctl stop atop
- 		exit 0
- 		;;
--	post)	/usr/bin/systemctl start atop
-+	post)	@systemd@/bin/systemctl start atop
- 		exit 0
- 		;;
diff --git a/pkgs/os-specific/linux/atop/atop-rotate.service.patch b/pkgs/os-specific/linux/atop/atop-rotate.service.patch
deleted file mode 100644
index 951a3c8c837..00000000000
--- a/pkgs/os-specific/linux/atop/atop-rotate.service.patch
+++ /dev/null
@@ -1,7 +0,0 @@
---- a/atop-rotate.service
-+++ b/atop-rotate.service
-@@ -4,3 +4,3 @@
- [Service]
- Type=oneshot
--ExecStart=/usr/bin/systemctl try-restart atop.service
-+ExecStart=@systemd@/bin/systemctl try-restart atop.service
diff --git a/pkgs/os-specific/linux/atop/atop.service.patch b/pkgs/os-specific/linux/atop/atop.service.patch
index 5467b512229..3ef59e60cbc 100644
--- a/pkgs/os-specific/linux/atop/atop.service.patch
+++ b/pkgs/os-specific/linux/atop/atop.service.patch
@@ -1,14 +1,10 @@
 --- a/atop.service
 +++ b/atop.service
-@@ -9,7 +9,8 @@
+@@ -9,5 +9,6 @@
  Environment=LOGPATH=/var/log/atop
 -EnvironmentFile=/etc/default/atop
 +EnvironmentFile=-/etc/default/atop
  ExecStartPre=/bin/sh -c 'test -n "$LOGINTERVAL" -a "$LOGINTERVAL" -eq "$LOGINTERVAL"'
  ExecStartPre=/bin/sh -c 'test -n "$LOGGENERATIONS" -a "$LOGGENERATIONS" -eq "$LOGGENERATIONS"'
--ExecStart=/bin/sh -c 'exec /usr/bin/atop ${LOGOPTS} -w "${LOGPATH}/atop_$(date +%%Y%%m%%d)" ${LOGINTERVAL}'
--ExecStartPost=/usr/bin/find "${LOGPATH}" -name "atop_*" -mtime +${LOGGENERATIONS} -exec rm -v {} \;
 +ExecStartPre=/bin/sh -c 'mkdir -p "${LOGPATH}"'
-+ExecStart=/bin/sh -c 'exec @out@/bin/atop ${LOGOPTS} -w "${LOGPATH}/atop_$(date +%%Y%%m%%d)" ${LOGINTERVAL}'
-+ExecStartPost=@findutils@/bin/find "${LOGPATH}" -name "atop_*" -mtime +${LOGGENERATIONS} -exec rm -v {} \;
- KillSignal=SIGUSR2
+ ExecStart=/bin/sh -c 'exec @out@/bin/atop ${LOGOPTS} -w "${LOGPATH}/atop_$(date +%%Y%%m%%d)" ${LOGINTERVAL}'
diff --git a/pkgs/os-specific/linux/atop/atopacct.service.patch b/pkgs/os-specific/linux/atop/atopacct.service.patch
index cbecbef90ec..9f2cd8f2e9c 100644
--- a/pkgs/os-specific/linux/atop/atopacct.service.patch
+++ b/pkgs/os-specific/linux/atop/atopacct.service.patch
@@ -1,11 +1,7 @@
 --- a/atopacct.service
 +++ b/atopacct.service
-@@ -8,6 +8,6 @@
- [Service]
+@@ -9,3 +9,3 @@
  Type=forking
 -PIDFile=/var/run/atopacctd.pid
--ExecStart=/usr/sbin/atopacctd
 +PIDFile=/run/atopacctd.pid
-+ExecStart=@out@/bin/atopacctd
-
- [Install]
+ ExecStart=@out@/bin/atopacctd
diff --git a/pkgs/os-specific/linux/atop/atopgpu.service.patch b/pkgs/os-specific/linux/atop/atopgpu.service.patch
deleted file mode 100644
index 30c38b67cad..00000000000
--- a/pkgs/os-specific/linux/atop/atopgpu.service.patch
+++ /dev/null
@@ -1,9 +0,0 @@
---- a/atopgpu.service
-+++ b/atopgpu.service
-@@ -6,5 +6,5 @@
-
- [Service]
--ExecStart=/usr/sbin/atopgpud
-+ExecStart=@out@/bin/atopgpud
- Type=oneshot
- RemainAfterExit=yes
diff --git a/pkgs/os-specific/linux/atop/default.nix b/pkgs/os-specific/linux/atop/default.nix
index 00510c91a74..61a76a50586 100644
--- a/pkgs/os-specific/linux/atop/default.nix
+++ b/pkgs/os-specific/linux/atop/default.nix
@@ -35,11 +35,14 @@ stdenv.mkDerivation rec {
   ];
 
   patches = [
-    ./atop-pm.sh.patch
-    ./atop-rotate.service.patch
+    # Fix paths in atop.service, atop-rotate.service, atopgpu.service, atopacct.service,
+    # and atop-pm.sh
+    ./fix-paths.patch
+    # Don't fail on missing /etc/default/atop, make sure /var/log/atop exists pre-start
     ./atop.service.patch
+    # Specify PIDFile in /run, not /var/run to silence systemd warning
     ./atopacct.service.patch
-  ] ++ (if withAtopgpu then [ ./atopgpu.service.patch ] else [ ]);
+  ];
 
   preConfigure = ''
     for f in *.{sh,service}; do
diff --git a/pkgs/os-specific/linux/atop/fix-paths.patch b/pkgs/os-specific/linux/atop/fix-paths.patch
new file mode 100644
index 00000000000..e6cd631d3c1
--- /dev/null
+++ b/pkgs/os-specific/linux/atop/fix-paths.patch
@@ -0,0 +1,48 @@
+--- a/atop.service
++++ b/atop.service
+@@ -12,4 +12,4 @@
+ ExecStartPre=/bin/sh -c 'test -n "$LOGGENERATIONS" -a "$LOGGENERATIONS" -eq "$LOGGENERATIONS"'
+-ExecStart=/bin/sh -c 'exec /usr/bin/atop ${LOGOPTS} -w "${LOGPATH}/atop_$(date +%%Y%%m%%d)" ${LOGINTERVAL}'
+-ExecStartPost=/usr/bin/find "${LOGPATH}" -name "atop_*" -mtime +${LOGGENERATIONS} -exec rm -v {} \;
++ExecStart=/bin/sh -c 'exec @out@/bin/atop ${LOGOPTS} -w "${LOGPATH}/atop_$(date +%%Y%%m%%d)" ${LOGINTERVAL}'
++ExecStartPost=@findutils@/bin/find "${LOGPATH}" -name "atop_*" -mtime +${LOGGENERATIONS} -exec rm -v {} \;
+ KillSignal=SIGUSR2
+
+--- a/atop-rotate.service
++++ b/atop-rotate.service
+@@ -4,3 +4,3 @@
+ [Service]
+ Type=oneshot
+-ExecStart=/usr/bin/systemctl try-restart atop.service
++ExecStart=@systemd@/bin/systemctl try-restart atop.service
+
+--- a/atopgpu.service
++++ b/atopgpu.service
+@@ -6,5 +6,5 @@
+
+ [Service]
+-ExecStart=/usr/sbin/atopgpud
++ExecStart=@out@/bin/atopgpud
+ Type=oneshot
+ RemainAfterExit=yes
+
+--- a/atopacct.service
++++ b/atopacct.service
+@@ -10,3 +10,3 @@
+ PIDFile=/var/run/atopacctd.pid
+-ExecStart=/usr/sbin/atopacctd
++ExecStart=@out@/bin/atopacctd
+
+--- a/atop-pm.sh
++++ b/atop-pm.sh
+@@ -2,8 +2,8 @@
+
+ case "$1" in
+-	pre)	/usr/bin/systemctl stop atop
++	pre)	@systemd@/bin/systemctl stop atop
+ 		exit 0
+ 		;;
+-	post)	/usr/bin/systemctl start atop
++	post)	@systemd@/bin/systemctl start atop
+ 		exit 0
+ 		;;

From 3b0bc541316bb581f94c549f8ed2ac132e1b594f Mon Sep 17 00:00:00 2001
From: Paul Schyska <pschyska@gmail.com>
Date: Mon, 17 May 2021 01:26:22 +0200
Subject: [PATCH 10/11] atop:, nixos/atop: Apply style suggestions

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
---
 pkgs/os-specific/linux/atop/default.nix | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/pkgs/os-specific/linux/atop/default.nix b/pkgs/os-specific/linux/atop/default.nix
index 61a76a50586..84a5870833a 100644
--- a/pkgs/os-specific/linux/atop/default.nix
+++ b/pkgs/os-specific/linux/atop/default.nix
@@ -6,7 +6,7 @@
 , findutils
 , systemd
 , python3
-  # makes the package unfree via pynvml
+# makes the package unfree via pynvml
 , withAtopgpu ? false
 }:
 
@@ -19,9 +19,9 @@ stdenv.mkDerivation rec {
     sha256 = "nsLKOlcWkvfvqglfmaUQZDK8txzCLNbElZfvBIEFj3I=";
   };
 
-  nativeBuildInputs = if withAtopgpu then [ python3.pkgs.wrapPython ] else [ ];
-  buildInputs = [ zlib ncurses ] ++ (if withAtopgpu then [ python3 ] else [ ]);
-  pythonPath = if withAtopgpu then [ python3.pkgs.pynvml ] else [ ];
+  nativeBuildInputs = lib.optionals withAtopgpu [ python3.pkgs.wrapPython ];
+  buildInputs = [ zlib ncurses ] ++ lib.optionals withAtopgpu [ python3 ];
+  pythonPath = lib.optionals withAtopgpu [ python3.pkgs.pynvml ];
 
   makeFlags = [
     "DESTDIR=$(out)"
@@ -59,14 +59,11 @@ stdenv.mkDerivation rec {
   '';
   postInstall = ''
     # remove extra files we don't need
-    rm -rf $out/{var,etc}
-    rm -rf $out/bin/atop{sar,}-${version}
+    rm -r $out/{var,etc} $out/bin/atop{sar,}-${version}
   '' + (if withAtopgpu then ''
     wrapPythonPrograms
   '' else ''
-    rm $out/lib/systemd/system/atopgpu.service
-    rm $out/bin/atopgpud
-    rm $out/share/man/man8/atopgpud.8
+    rm $out/lib/systemd/system/atopgpu.service $out/bin/atopgpud $out/share/man/man8/atopgpud.8
   '');
 
   meta = with lib; {

From 033cdffa9ba0a609e2511471b50b27de13ac8b64 Mon Sep 17 00:00:00 2001
From: Paul Schyska <paul@schyska.net>
Date: Mon, 17 May 2021 13:46:55 +0200
Subject: [PATCH 11/11] atop: use substituteInPlace instead of sed

---
 pkgs/os-specific/linux/atop/default.nix | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pkgs/os-specific/linux/atop/default.nix b/pkgs/os-specific/linux/atop/default.nix
index 84a5870833a..50a3e3e6316 100644
--- a/pkgs/os-specific/linux/atop/default.nix
+++ b/pkgs/os-specific/linux/atop/default.nix
@@ -49,8 +49,8 @@ stdenv.mkDerivation rec {
       findutils=${findutils} systemd=${systemd} substituteAllInPlace "$f"
     done
 
-    sed -e 's/chown/true/g' -i Makefile
-    sed -e 's/chmod 04711/chmod 0711/g' -i Makefile
+    substituteInPlace Makefile --replace 'chown' 'true'
+    substituteInPlace Makefile --replace 'chmod 04711' 'chmod 0711'
   '';
 
   installTargets = [ "systemdinstall" ];