From 94f70eab31dbced605af569a39adb969cdb6cc17 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Wed, 9 Sep 2026 18:17:35 -0400 Subject: [PATCH] fix(install): render the systemd units from their templates, not from heredocs The installers carried their own inline copies of units that also exist as templates under systemd/, and the copies drifted. install_service.sh renders ledmatrix.service from the template correctly, then wrote ledmatrix-web.service from a heredoc that predated it -- missing Wants=network-online.target, RestartSec=10, SyslogIdentifier, CacheDirectory, CacheDirectoryMode and Environment=USE_THREADING=1. install_web_service.sh had a third copy, and install_wifi_monitor.sh a fourth, that one already differing from its template (syslog where the template says journal). startup_validator.py compares the installed unit against the template, so a rig installed this way warned on every boot -- and the remedy the warning names, "re-run scripts/install/install_service.sh", reinstalled the same stale copy. The warning could never clear. Reproduced on a live rig running exactly that unit. All three installers now render systemd/*.service through the same placeholder substitution. The template gains a __USER__ placeholder rather than hardcoding User=root, because the web interface runs as whoever installed it. That last point was a second, independent cause of a permanent warning: the validator substituted a fixed "root", so any non-root install reported drift forever. It now reads User= from the installed unit -- an install-time decision, not something the template dictates -- and compares everything else strictly. first_time_install.sh already reads the installed User= the same way. Tests cover a non-root web unit not warning, a genuinely changed directive in that unit still warning, the User= fallback, and a grep-based guard that no installer under scripts/install/ contains an inline unit body. That guard is what found the install_wifi_monitor.sh copy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --- scripts/install/install_service.sh | 62 ++++++++--------- scripts/install/install_web_service.sh | 41 ++++-------- scripts/install/install_wifi_monitor.sh | 30 +++------ src/startup_validator.py | 33 +++++++-- systemd/ledmatrix-web.service | 16 +++-- test/test_systemd_unit_drift.py | 89 +++++++++++++++++++++++++ 6 files changed, 176 insertions(+), 95 deletions(-) diff --git a/scripts/install/install_service.sh b/scripts/install/install_service.sh index cd4144cc..36ea994d 100755 --- a/scripts/install/install_service.sh +++ b/scripts/install/install_service.sh @@ -20,10 +20,12 @@ echo "Installing LED Matrix Display Service for user: $ACTUAL_USER" echo "Using home directory: $USER_HOME" echo "Project root directory: $PROJECT_ROOT_DIR" -# Create a temporary service file for the main display with the correct paths -# Assuming ledmatrix.service template exists and uses /home/ledpi as a placeholder for user home +# Render the main display unit from its template. The display service runs as +# root (it needs GPIO), so __USER__ is always root here -- unlike the web unit +# below, which runs as whoever installed it. if [ -f "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" ]; then - sed "s|/home/ledpi|$USER_HOME|g; s|__PROJECT_ROOT_DIR__|$PROJECT_ROOT_DIR|g; s|__USER__|root|g" "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" > /tmp/ledmatrix.service.tmp + sed "s|__PROJECT_ROOT_DIR__|$PROJECT_ROOT_DIR|g; s|__USER__|root|g" \ + "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" > /tmp/ledmatrix.service.tmp # Copy the service file to the systemd directory sudo cp /tmp/ledmatrix.service.tmp /etc/systemd/system/ledmatrix.service # Clean up @@ -48,42 +50,36 @@ fi # === LEDMatrix Web Interface service (ledmatrix-web.service) === echo "Installing LEDMatrix Web Interface service (ledmatrix-web.service)..." -WEB_SERVICE_FILE_CONTENT=$(cat < /dev/null +# Rendered from systemd/ledmatrix-web.service, the same template +# install_web_service.sh uses. This was an inline heredoc until it drifted from +# the template: it had lost Wants=network-online.target, RestartSec, +# SyslogIdentifier, CacheDirectory and Environment=USE_THREADING. Because +# src/startup_validator.py compares the installed unit against the template, +# every boot warned "re-run install_service.sh" -- and doing so reinstalled the +# same stale copy, so the warning could never clear. +if [ -f "$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service" ]; then + sed "s|__PROJECT_ROOT_DIR__|$PROJECT_ROOT_DIR|g; s|__USER__|$ACTUAL_USER|g" \ + "$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service" \ + | sudo tee /etc/systemd/system/ledmatrix-web.service > /dev/null +else + echo "WARNING: ledmatrix-web.service template not found at $PROJECT_ROOT_DIR/systemd/ledmatrix-web.service. Web interface service not configured." +fi echo "Reloading systemd daemon for web service..." sudo systemctl daemon-reload -echo "Enabling ledmatrix-web.service to start on boot..." -sudo systemctl enable ledmatrix-web.service +if [ -f "/etc/systemd/system/ledmatrix-web.service" ]; then + echo "Enabling ledmatrix-web.service to start on boot..." + sudo systemctl enable ledmatrix-web.service -echo "Starting ledmatrix-web.service..." -sudo systemctl start ledmatrix-web.service + echo "Starting ledmatrix-web.service..." + sudo systemctl start ledmatrix-web.service -echo "LEDMatrix Web Interface service (ledmatrix-web.service) installation complete." -echo "It will start based on the 'web_display_autostart' setting in config/config.json." + echo "LEDMatrix Web Interface service (ledmatrix-web.service) installation complete." + echo "It will start based on the 'web_display_autostart' setting in config/config.json." +else + echo "Skipping enable/start for ledmatrix-web.service as it was not configured." +fi # === End of LEDMatrix Web Interface service === diff --git a/scripts/install/install_web_service.sh b/scripts/install/install_web_service.sh index 6df16d71..de8d9af2 100755 --- a/scripts/install/install_web_service.sh +++ b/scripts/install/install_web_service.sh @@ -26,37 +26,20 @@ if [ "$EUID" -ne 0 ]; then exit 1 fi -# Generate the service file dynamically with the correct paths -echo "Generating service file with dynamic paths..." -WEB_SERVICE_FILE_CONTENT=$(cat < /etc/systemd/system/ledmatrix-web.service +sed "s|__PROJECT_ROOT_DIR__|$PROJECT_ROOT_DIR|g; s|__USER__|$ACTUAL_USER|g" \ + "$TEMPLATE" > /etc/systemd/system/ledmatrix-web.service # Ensure cache directory exists with proper permissions # This is a fallback for older systemd versions that don't support CacheDirectory diff --git a/scripts/install/install_wifi_monitor.sh b/scripts/install/install_wifi_monitor.sh index 5d224c03..f4e8093e 100755 --- a/scripts/install/install_wifi_monitor.sh +++ b/scripts/install/install_wifi_monitor.sh @@ -64,30 +64,18 @@ if [ ${#MISSING_PACKAGES[@]} -gt 0 ]; then echo "✓ Package installation completed" fi -# Create service file with correct paths +# Render the unit from systemd/ledmatrix-wifi-monitor.service rather than +# inlining a second copy here. The copy this replaced had already drifted -- +# it wrote StandardOutput/StandardError=syslog where the template says journal. echo "" echo "Creating systemd service file..." -SERVICE_FILE_CONTENT=$(cat < /dev/null diff --git a/src/startup_validator.py b/src/startup_validator.py index 79275dc2..36065bf0 100644 --- a/src/startup_validator.py +++ b/src/startup_validator.py @@ -111,17 +111,25 @@ class StartupValidator: if not template.is_file() or not installed.is_file(): continue - # The template carries placeholders the installer substitutes, - # so compare the substituted form rather than the raw file. - expected = template.read_text(encoding="utf-8") - expected = expected.replace("__PROJECT_ROOT_DIR__", str(project_root)) - expected = expected.replace("__USER__", "root") - try: actual = installed.read_text(encoding="utf-8") except PermissionError: continue + # The template carries placeholders the installer substitutes, + # so compare the substituted form rather than the raw file. + expected = template.read_text(encoding="utf-8") + expected = expected.replace("__PROJECT_ROOT_DIR__", str(project_root)) + # User= is an install-time decision, not something the template + # dictates: the installers write whoever ran them, which on a + # non-root install is never "root". Substituting a fixed "root" + # here reported drift on every such install, permanently -- and + # re-running the installer, which is what the warning tells you + # to do, could not clear it. Taking the installed unit's own + # value keeps the comparison on the directives the template + # actually controls. + expected = expected.replace("__USER__", self._installed_user(actual)) + if self._unit_body(expected) != self._unit_body(actual): self.warnings.append( f"{installed.name} differs from {template_rel}; the " @@ -132,6 +140,19 @@ class StartupValidator: except OSError as e: self.logger.debug("Could not compare systemd units: %s", e) + @staticmethod + def _installed_user(unit_text: str) -> str: + """The installed unit's ``User=``, or "root" when it does not set one. + + systemd itself defaults to root for a system unit with no User=, so that + is the right fallback rather than an empty string. + """ + for line in unit_text.splitlines(): + stripped = line.strip() + if stripped.startswith("User="): + return stripped.split("=", 1)[1].strip() + return "root" + @staticmethod def _unit_body(text: str) -> str: """A unit's meaningful lines, in order: no comments, no blanks. diff --git a/systemd/ledmatrix-web.service b/systemd/ledmatrix-web.service index 4dcb9fd7..3b74b569 100644 --- a/systemd/ledmatrix-web.service +++ b/systemd/ledmatrix-web.service @@ -1,9 +1,13 @@ -# This is a template file. The actual service file is generated dynamically -# by install_web_service.sh with the correct paths based on where the project -# is installed. Do not use this file directly - use install_web_service.sh instead. +# This is a template. Do not install it directly -- run +# scripts/install/install_web_service.sh (or install_service.sh, which +# renders this same file). Both substitute: +# __PROJECT_ROOT_DIR__ -> the directory the project is installed in +# __USER__ -> the user the web interface runs as # -# Template - paths will be replaced dynamically: -# __PROJECT_ROOT_DIR__ will be replaced with the actual project directory +# This file is the only description of the unit. It used to be duplicated as +# an inline heredoc in both installers, which drifted apart; src/startup_validator.py +# compares the installed unit against this template, so a second copy means a +# permanent, unclearable drift warning on every boot. [Unit] Description=LED Matrix Web Interface Service @@ -12,7 +16,7 @@ Wants=network-online.target [Service] Type=simple -User=root +User=__USER__ WorkingDirectory=__PROJECT_ROOT_DIR__ Environment=USE_THREADING=1 ExecStart=/usr/bin/python3 __PROJECT_ROOT_DIR__/scripts/utils/start_web_conditionally.py diff --git a/test/test_systemd_unit_drift.py b/test/test_systemd_unit_drift.py index 21cf56e0..4b7f5562 100644 --- a/test/test_systemd_unit_drift.py +++ b/test/test_systemd_unit_drift.py @@ -150,3 +150,92 @@ def test_a_missing_installed_unit_is_silent(validator, tmp_path): validator._validate_systemd_units() assert not validator.warnings assert not validator.errors + + +# --- the web unit: one template, three copies, and a warning that never cleared --- +# +# install_service.sh and install_web_service.sh each carried their own inline +# heredoc of ledmatrix-web.service. install_service.sh's had drifted -- no +# Wants=network-online.target, RestartSec, SyslogIdentifier or CacheDirectory -- +# and that is what was installed on real rigs. The validator correctly reported +# the drift and told the user to re-run install_service.sh, which reinstalled the +# same stale copy, so the warning could never clear. Separately, the template +# hardcoded User=root while the installers write whoever ran them, so even the +# *correct* installer produced a permanent warning on any non-root install. + + +def _render(template_text, project_root, user): + """Exactly what the installers' sed does.""" + return (template_text + .replace("__PROJECT_ROOT_DIR__", str(project_root)) + .replace("__USER__", user)) + + +def test_the_web_unit_installed_as_a_non_root_user_is_not_drift(validator, tmp_path): + """The web interface runs as whoever installed it, not as root. + + This is the case that warned forever: nothing the user could do would make + an installed `User=pi` match a template that said `User=root`. + """ + project_root = Path("src/startup_validator.py").resolve().parent.parent + template_rel = "systemd/ledmatrix-web.service" + template = project_root / template_rel + if not template.is_file(): + pytest.skip("repo unit template not present") + + installed = tmp_path / "ledmatrix-web.service" + installed.write_text( + _render(template.read_text(encoding="utf-8"), project_root, "hdpi"), + encoding="utf-8") + + validator._UNITS = ((template_rel, str(installed)),) + validator._validate_systemd_units() + assert not validator.warnings, ( + f"a correctly installed non-root web unit warned: {validator.warnings}") + + +def test_the_web_unit_still_reports_a_real_changed_directive(validator, tmp_path): + """Ignoring User= must not make the check blind to everything else.""" + project_root = Path("src/startup_validator.py").resolve().parent.parent + template_rel = "systemd/ledmatrix-web.service" + template = project_root / template_rel + if not template.is_file(): + pytest.skip("repo unit template not present") + + rendered = _render(template.read_text(encoding="utf-8"), project_root, "hdpi") + # Drop RestartSec -- one of the directives the stale heredoc was missing. + stale = "\n".join(l for l in rendered.splitlines() if not l.startswith("RestartSec=")) + installed = tmp_path / "ledmatrix-web.service" + installed.write_text(stale + "\n", encoding="utf-8") + + validator._UNITS = ((template_rel, str(installed)),) + validator._validate_systemd_units() + assert validator.warnings, "a web unit missing RestartSec= produced no warning" + assert not validator.errors + + +def test_installed_user_falls_back_to_root(): + """systemd defaults a system unit with no User= to root, so we must too.""" + assert StartupValidator._installed_user("[Service]\nExecStart=/x\n") == "root" + assert StartupValidator._installed_user("[Service]\nUser=pi\n") == "pi" + assert StartupValidator._installed_user("[Service]\n User=hdpi \n") == "hdpi" + + +def test_no_installer_carries_its_own_copy_of_a_unit(): + """The regression guard. + + Both installers used to inline the unit as a heredoc, and the two copies + drifted from the template and from each other. A unit body in a shell script + is the bug, so assert there isn't one rather than asserting the current + contents match -- matching contents is exactly what silently stops being + true. + """ + project_root = Path("src/startup_validator.py").resolve().parent.parent + offenders = [] + for script in sorted((project_root / "scripts" / "install").glob("*.sh")): + text = script.read_text(encoding="utf-8", errors="replace") + if "[Unit]" in text and "Description=" in text: + offenders.append(script.name) + assert not offenders, ( + f"{offenders} contain an inline systemd unit; render " + f"systemd/*.service instead so there is one source of truth")