Repository: MarkHedleyJones/dmenu-extended (language: python)

Issue:
systemd timer interval not being set correctly by install script
When trying to start the timer the following error occurs:
```
Failed to start dmenu-extended-update-db.timer: Unit dmenu-extended-update-db.timer has a bad unit file setting.
See user logs and 'systemctl --user status dmenu-extended-update-db.timer' for details.
```
Looking at the details shows
```
Failed to parse calendar specification, ig>
Jul 30 17:24:39 laptop systemd[7712]: dmenu-extended-update-db.timer: Timer unit lacks value setting. Refusing.
```
Looking at the timer value in the `.timer` file shows:
```
OnCalendar=*:0/None
```


Discussion / hints:

Fix patch:
diff --git a/pyproject.toml b/pyproject.toml
index 9d13c60..9810ff3 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "dmenu_extended"
-version = "1.0.0"
+version = "1.0.1"
 authors = [
   { name="Mark Hedley Jones", email="markhedleyjones@gmail.com" },
 ]
diff --git a/src/dmenu_extended/install_systemd_service.py b/src/dmenu_extended/install_systemd_service.py
index ceb3aa4..0976c5d 100755
--- a/src/dmenu_extended/install_systemd_service.py
+++ b/src/dmenu_extended/install_systemd_service.py
@@ -1,181 +1,191 @@
 #!/usr/bin/env python3
 
 import argparse
-import grp
 import os
 import subprocess
 
 
-def parse_args():
-    parser = argparse.ArgumentParser(
-        description="Install dmenu-extended systemd services"
-    )
-    parser.add_argument("--remove", action="store_true", help="Remove systemd services")
-    parser.add_argument("--start", action="store_true", help="Start systemd services")
-    parser.add_argument(
-        "--rebuild-interval-mins",
-        type=int,
-        help="How often to update the cache (in minutes)",
-    )
-    return parser.parse_args()
+def run_systemd_command(user_command, silent=False):
+    command = ["systemctl", "--user"] + user_command
+    if silent:
+        return (
+            subprocess.call(
+                command, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL
+            )
+            == 0
+        )
+    else:
+        return subprocess.call(command) == 0
 
 
-def get_executable_path(username):
-    executable_name = "dmenu_extended_cache_build"
-    executable_paths = ["/usr/local/bin", "/usr/bin", f"/home/{username}/.local/bin"]
+def find_executable(executable_name):
+    executable_paths = [
+        "/usr/local/bin",
+        "/usr/bin",
+        os.path.expanduser("~/.local/bin"),
+    ]
     for executable_path in executable_paths:
         path = os.path.join(executable_path, executable_name)
         if os.path.isfile(path):
             return path
 
 
-def target_files(rebuild_interval_mins=20):
-    username = os.environ["USER"]
-    groupid = grp.getgrnam(username).gr_gid
-    executable_path = get_executable_path(username)
-    if not executable_path:
-        raise Exception("Could not find dmenu_extended_cache_build executable")
-
-    return {
-        "dmenu-extended-update-db.service": "\n".join(
-            [
-                "[Unit]",
-                "Description=Update dmenu-extended cache",
-                "Wants=dmenu-extended-update-db.timer",
-                "",
-                "[Service]",
-                "Type=oneshot",
-                f"User={username}",
-                f"Group={groupid}",
-                "ExecStart=" + executable_path,
-                "",
-                "[Install]",
-                "WantedBy=multi-user.target",
-                "",
-            ]
-        ),
-        "dmenu-extended-update-db.timer": "\n".join(
-            [
-                "[Unit]",
-                "Description=Run update-dmenu-extended-db.service every 20 minutes",
-                "Requires=dmenu-extended-update-db.service",
-                "",
-                "[Timer]",
-                "Unit=dmenu-extended-update-db.service",
-                f"OnCalendar=*:0/{rebuild_interval_mins}",
-                "",
-                "[Install]",
-                "WantedBy=timers.target",
-                "",
-            ]
-        ),
-    }
-
-
-def get_possible_install_paths():
-    possible_install_paths = []
+def detect_systemd_user_paths():
+    systemd_user_paths = []
     if "XDG_CONFIG_HOME" in os.environ:
-        possible_install_paths.append(os.environ["XDG_CONFIG_HOME"] + "/systemd/user")
+        systemd_user_paths.append(f"{os.environ['XDG_CONFIG_HOME']}/systemd/user")
     if "HOME" in os.environ:
-        possible_install_paths.append(os.environ["HOME"] + "/.config/systemd/user")
-        possible_install_paths.append(os.environ["HOME"] + "/.local/share/systemd/user")
+        systemd_user_paths.append(f"{os.environ['HOME']}/.config/systemd/user")
+        systemd_user_paths.append(f"{os.environ['HOME']}/.local/share/systemd/user")
     if "XDG_RUNTIME_DIR" in os.environ:
-        possible_install_paths.append(os.environ["XDG_RUNTIME_DIR"] + "/systemd/user")
+        systemd_user_paths.append(f"{os.environ['XDG_RUNTIME_DIR']}/systemd/user")
     if "XDG_DATA_HOME" in os.environ:
-        possible_install_paths.append(os.environ["XDG_DATA_HOME"] + "/systemd/user")
-    return possible_install_paths
-
-
-def get_install_path():
-    if "XDG_DATA_HOME" in os.environ:
-        return os.environ["XDG_DATA_HOME"] + "/systemd/user"
-    elif "HOME" in os.environ:
-        return os.environ["HOME"] + "/.local/share/systemd/user"
-
-
-def install(rebuild_interval_mins):
-    install_path = get_install_path()
-    if not os.path.isdir(install_path):
-        print("Creating systemd user directory: " + install_path)
-        os.makedirs(install_path)
-
-    for filename, contents in target_files(rebuild_interval_mins).items():
-        path = os.path.join(install_path, filename)
-        print("Installing " + filename + " to " + install_path)
-        with open(path, "w") as f:
-            f.write(contents)
-
-
-def run_systemd_command(user_command, silent=False):
-    command = ["systemctl", "--user"] + user_command
-    if silent:
-        return (
-            subprocess.call(
-                command, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL
+        systemd_user_paths.append(f"{os.environ['XDG_DATA_HOME']}/systemd/user")
+    return systemd_user_paths
+
+
+class ServiceInstaller:
+    def __init__(self):
+        self.filenames = {
+            key: f"dmenu-extended-update-db.{key}" for key in ["service", "timer"]
+        }
+        self.filenames["target"] = "dmenu_extended_cache_build"
+
+    def generate_install_files(self, rebuild_interval_mins):
+        if rebuild_interval_mins is None or rebuild_interval_mins < 1:
+            raise Exception("Invalid rebuild interval")
+        return {
+            self.filenames["service"]: "\n".join(
+                [
+                    "[Unit]",
+                    "Description=Update dmenu-extended cache",
+                    f"Wants={self.filenames['timer']}",
+                    "",
+                    "[Service]",
+                    "Type=oneshot",
+                    f"ExecStart={self.filenames['target']}",
+                    "",
+                    "[Install]",
+                    "WantedBy=multi-user.target",
+                    "",
+                ]
+            ),
+            self.filenames["timer"]: "\n".join(
+                [
+                    "[Unit]",
+                    f"Description=Run {self.filenames['service']} every {rebuild_interval_mins} minutes",
+                    f"Requires={self.filenames['service']}",
+                    "",
+                    "[Timer]",
+                    f"Unit={self.filenames['service']}",
+                    f"OnCalendar=*:0/{rebuild_interval_mins}",
+                    "",
+                    "[Install]",
+                    "WantedBy=timers.target",
+                    "",
+                ]
+            ),
+        }
+
+    @staticmethod
+    def get_install_path():
+        if "XDG_DATA_HOME" in os.environ:
+            return f"{os.environ['XDG_DATA_HOME']}/systemd/user"
+        elif "HOME" in os.environ:
+            return f"{os.environ['HOME']}/.local/share/systemd/user"
+
+    def install(self, rebuild_interval_mins=20):
+        executable_path = find_executable(self.filenames["target"])
+        if not executable_path:
+            raise Exception(
+                f"Could not find target executable ({self.filenames['target']})"
             )
-            == 0
-        )
-    else:
-        return subprocess.call(command) == 0
 
+        install_path = self.get_install_path()
+        if not os.path.isdir(install_path):
+            print(f"Creating systemd user directory: {install_path}")
+            os.makedirs(install_path)
 
-def remove():
-    run_systemd_command(["daemon-reload"])
-    removed_files = []
-    for filename in target_files().keys():
-        if filename.endswith(".timer") and run_systemd_command(
-            ["is-enabled", filename], silent=True
-        ):
-            print("Stopping " + filename + "...")
-            run_systemd_command(["stop", filename])
-            run_systemd_command(["disable", filename])
-
-        for install_path in get_possible_install_paths():
+        for filename, contents in self.generate_install_files(
+            rebuild_interval_mins
+        ).items():
             path = os.path.join(install_path, filename)
-            if os.path.exists(path):
-                removed_files.append(path)
-                os.remove(path)
-    if removed_files:
-        for path in removed_files:
-            print("Removed " + path)
-    else:
-        print("No systemd files found to remove.")
+            print(f"Installing {filename} to {install_path}")
+            with open(path, "w") as f:
+                f.write(contents)
 
-
-def prompt_to_start():
-    print("The systemd service has been installed.")
-    if input("Do you want to enable it now? [y/N] ").lower() == "y":
+    def remove(self):
+        run_systemd_command(["daemon-reload"])
+        removed_files = []
+        for filename in self.generate_install_files().keys():
+            if filename.endswith(".timer") and run_systemd_command(
+                ["is-enabled", filename], silent=True
+            ):
+                print(f"Stopping {filename} ...")
+                run_systemd_command(["stop", filename])
+                run_systemd_command(["disable", filename])
+
+            for possible_install_path in detect_systemd_user_paths():
+                path = os.path.join(possible_install_path, filename)
+                if os.path.exists(path):
+                    removed_files.append(path)
+                    os.remove(path)
+        if removed_files:
+            for path in removed_files:
+                print(f"Removed {path}")
+        else:
+            print("No systemd files found to remove")
+
+    def start(self):
         run_systemd_command(["daemon-reload"])
-        target = "dmenu-extended-update-db.timer"
+        target = f"{self.filenames}.timer"
         if run_systemd_command(["start", target]) and run_systemd_command(
             ["enable", target]
         ):
             print(f"Started {target}")
 
 
-def start_service():
-    run_systemd_command(["daemon-reload"])
-    target = "dmenu-extended-update-db.timer"
-    if run_systemd_command(["start", target]) and run_systemd_command(
-        ["enable", target]
-    ):
-        print(f"Started {target}")
-
-
 def run():
-    def user_is_root():
-        return os.geteuid() == 0
-
-    args = parse_args()
-    if user_is_root():
+    def parse_args():
+        parser = argparse.ArgumentParser(
+            description="Install dmenu-extended systemd services"
+        )
+        parser.add_argument(
+            "--remove", action="store_true", help="Remove systemd services"
+        )
+        parser.add_argument(
+            "--start", action="store_true", help="Start systemd services"
+        )
+        parser.add_argument(
+            "--rebuild-interval-mins",
+            type=int,
+            default=20,
+            help="How often to update the cache (in minutes)",
+        )
+        return parser.parse_args()
+
+    def prompt_to_start(service_target):
+        print("The systemd service has been installed.")
+        if input("Do you want to enable it now? [y/N] ").lower() == "y":
+            run_systemd_command(["daemon-reload"])
+            if run_systemd_command(["start", service_target]) and run_systemd_command(
+                ["enable", service_target]
+            ):
+                print(f"Started {service_target}")
+
+    if os.geteuid() == 0:
         print("This script must be run as a non-root user.")
         exit(1)
+
+    args = parse_args()
+    service_installer = ServiceInstaller()
+
     if args.remove:
-        remove()
+        service_installer.remove()
     else:
-        install(args.rebuild_interval_mins)
-        if args.start or prompt_to_start():
-            start_service()
+        service_installer.install(args.rebuild_interval_mins)
+        if args.start or prompt_to_start(service_installer.filenames["timer"]):
+            service_installer.start()
 
 
 if __name__ == "__main__":


Test patch:
diff --git a/test.sh b/test.sh
new file mode 100755
index 0000000..f5adf56
--- /dev/null
+++ b/test.sh
@@ -0,0 +1,4 @@
+#!/usr/bin/env sh
+
+cd src/dmenu_extended
+python3 -m pytest ../../tests
diff --git a/tests/test_install_systemd_service.py b/tests/test_install_systemd_service.py
new file mode 100755
index 0000000..1c26588
--- /dev/null
+++ b/tests/test_install_systemd_service.py
@@ -0,0 +1,70 @@
+#!/usr/bin/env python3
+
+import install_systemd_service
+import mock
+import os
+import pytest
+import shutil
+
+test_path = os.path.dirname(os.path.abspath(__file__))
+
+
+def test_generate_valid_service_files():
+    def check_service_file(lines, _):
+        expected_lines = [
+            "[Unit]",
+            "Description=Update dmenu-extended cache",
+            "Wants=dmenu-extended-update-db.timer",
+            "[Service]",
+            "Type=oneshot",
+            "ExecStart=dmenu_extended_cache_build",
+            "[Install]",
+            "WantedBy=multi-user.target",
+        ]
+        for line, expected_line in zip(lines, expected_lines):
+            assert line == expected_line
+
+    def check_timer_file(lines, expected_mins):
+        expected_lines = [
+            "[Unit]",
+            f"Description=Run dmenu-extended-update-db.service every {expected_mins} minutes",
+            "Requires=dmenu-extended-update-db.service",
+            "[Timer]",
+            "Unit=dmenu-extended-update-db.service",
+            f"OnCalendar=*:0/{expected_mins}",
+            "[Install]",
+            "WantedBy=timers.target",
+        ]
+        for line, expected_line in zip(lines, expected_lines):
+            assert line == expected_line
+
+    installer = install_systemd_service.ServiceInstaller()
+    path_test_install = os.path.join(test_path, "test_install_dir")
+    with mock.patch.object(
+        installer,
+        "get_install_path",
+        new=lambda: path_test_install,
+    ):
+        file_checker_functions = {
+            "service": check_service_file,
+            "timer": check_timer_file,
+        }
+
+        test_install_parameters = [(20, 20), (15, 15), (None, 20), (-5, 20)]
+        for rebuild_interval_parameter, expected_mins in test_install_parameters:
+            if rebuild_interval_parameter is None or rebuild_interval_parameter < 1:
+                with pytest.raises(Exception):
+                    installer.install(rebuild_interval_parameter)
+            else:
+                installer.install(rebuild_interval_parameter)
+                files = {}
+                for filetype, checker_func in file_checker_functions.items():
+                    path = os.path.join(
+                        path_test_install, installer.filenames[filetype]
+                    )
+                    with open(path) as f:
+                        files[filetype] = [
+                            line for line in f.read().split("\n") if line.strip() != ""
+                        ]
+                    checker_func(files[filetype], expected_mins)
+                shutil.rmtree(path_test_install)