From 3becfac66dc569e71d0f6085fb56e92cfa6d626a Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Fri, 24 Jul 2026 12:19:04 -0500 Subject: fix(installer): eight fixes from an overnight bug-hunt and its review I squashed these because the per-bug reasoning lives in todo.org, which this commit carries. Two could cost a machine. configure_initramfs_hook swapped the udev hook for systemd on a LUKS root, leaving a standalone encrypt hook under an init that never runs it. The rebuild succeeds and the installer exits clean, then the root won't unlock at the next boot. trim_firmware ran pacman -Rdd against twelve firmware packages behind a DMI gate reading product_name, where "Framework" never appears. That left it dead on the hardware it targets, and dangerous to fix the obvious way: this machine is a Framework Desktop whose Ryzen iGPU needs the amdgpu firmware. It refuses on PCI modalias evidence now. wipedisk discarded before it checked. blkdiscard ran with -f, which disables the exclusive open, so picking the wrong disk destroyed a live filesystem and then reported that nothing had happened. Four more are smaller. The NVIDIA preflight aborted dwm and headless installs over a driver floor they never need. zfs-replicate exited 0 after every dataset failed. Unattended installs blocked on two prompts, and the first fix for that inherited a [Y/n] default into passwordless console login. A fresh install left the dotfiles repo permanently dirty. The review found a pattern worth more than any single fix. Helpers had thorough tests and none proved they were called. Deleting the call left five suites green, including the guard on that pacman -Rdd. CALL_SITES now pins nine caller/callee pairs. The suite runs 341 tests at exit 0, with no new shellcheck findings. I proved every guard by deleting it and watching the intended test go red. --- .../installer-steps/test_mark_volatile_configs.py | 216 +++++++++++++++++++++ 1 file changed, 216 insertions(+) create mode 100644 tests/installer-steps/test_mark_volatile_configs.py (limited to 'tests/installer-steps/test_mark_volatile_configs.py') diff --git a/tests/installer-steps/test_mark_volatile_configs.py b/tests/installer-steps/test_mark_volatile_configs.py new file mode 100644 index 0000000..27eac61 --- /dev/null +++ b/tests/installer-steps/test_mark_volatile_configs.py @@ -0,0 +1,216 @@ +"""Tests for mark_volatile_configs, the tail of the dotfiles install. + +Four stowed configs get rewritten in place by the apps that own them (btop, +qalculate, calibre, waypaper). The dotfiles repo handles that with git's +skip-worktree bit, applied by its own `skip-volatile` make target, which +`make stow` runs as its last step. + +archsetup stows inline rather than through that Makefile, and it never ran the +step -- so a machine installed by archsetup alone starts with a dotfiles repo +that goes dirty as soon as those apps run, and every later `git pull --ff-only` +trips over paths the user never edited. Both daily drivers carry the bits +today, but archsetup contains no code that sets them: they were applied by a +hand-run `make stow`. + +The fix calls the dotfiles Makefile's target rather than reimplementing the +logic here, so the volatile list stays in one place. That is what these tests +pin: that archsetup delegates, and that it degrades quietly when the checkout +it was handed cannot answer. + +These tests exercise the REAL function body, extracted from the `archsetup` +script at run time (not a copy), against a fixture git repo, with sudo stubbed +on PATH. + +Run from repo root: + python3 -m unittest tests.installer-steps.test_mark_volatile_configs +""" + +import os +import shutil +import subprocess +import tempfile +import textwrap +import unittest + + +REPO_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..")) +ARCHSETUP = os.path.join(REPO_ROOT, "archsetup") + +# The real target, copied from the dotfiles Makefile. The point of the fixture +# is that archsetup calls *a* skip-volatile target rather than doing the work +# itself; the body here mirrors the real one so the assertions are meaningful. +MAKEFILE = textwrap.dedent("""\ + DOTFILES := $(shell pwd) + VOLATILE := $(shell sed 's/#.*//' $(DOTFILES)/volatile-configs 2>/dev/null) + + skip-volatile: + \t@for f in $(VOLATILE); do \\ + \t\tif git -C $(DOTFILES) ls-files --error-unmatch "$$f" >/dev/null 2>&1; then \\ + \t\t\tgit -C $(DOTFILES) update-index --skip-worktree "$$f" && echo " skip-worktree: $$f"; \\ + \t\telse \\ + \t\t\techo " WARN: not tracked, skipping: $$f"; \\ + \t\tfi; \\ + \tdone +""") + +# sudo isn't available (and isn't wanted) in a unit test. Drop `-u ` and +# exec the rest, so the function's real command line is what runs. +SUDO_STUB = textwrap.dedent("""\ + #!/bin/bash + # Record the argv archsetup handed sudo, then drop `-u ` and exec the + # rest, so the function's real command line is what runs. + printf '%s\\n' "$*" >> "$SUDO_CALLS" + if [ "$1" = "-u" ]; then shift 2; fi + exec "$@" +""") + + +class MarkVolatileConfigsHarness(unittest.TestCase): + """Source mark_volatile_configs out of the real archsetup script.""" + + def setUp(self): + self.tmp = tempfile.mkdtemp(prefix="volatile-test-") + self.repo = os.path.join(self.tmp, "dotfiles") + os.mkdir(self.repo) + self.bindir = os.path.join(self.tmp, "bin") + os.mkdir(self.bindir) + sudo = os.path.join(self.bindir, "sudo") + with open(sudo, "w") as f: + f.write(SUDO_STUB) + os.chmod(sudo, 0o755) + self.logfile = os.path.join(self.tmp, "install.log") + self.sudo_calls = os.path.join(self.tmp, "sudo-calls") + self.wrapper = os.path.join(self.tmp, "run.sh") + with open(self.wrapper, "w") as f: + f.write( + "#!/bin/bash\n" + 'ARCHSETUP="$1"; shift\n' + "source <(sed -n " + "'/^mark_volatile_configs() {/,/^}/p' \"$ARCHSETUP\")\n" + 'display() { echo "DISPLAY:$2"; }\n' + 'error_warn() { echo "WARN:$1"; return 1; }\n' + 'mark_volatile_configs "$REPO" testuser\n' + 'echo "RETURNED=$?"\n' + ) + os.chmod(self.wrapper, 0o755) + + def tearDown(self): + shutil.rmtree(self.tmp, ignore_errors=True) + + def git(self, *args): + return subprocess.run(["git", "-C", self.repo, *args], + capture_output=True, text=True) + + def make_repo(self, tracked=("common/.config/btop/btop.conf",), + volatile=None, makefile: "str | None" = MAKEFILE): + self.git("init", "-q") + self.git("config", "user.email", "t@example.com") + self.git("config", "user.name", "t") + for rel in tracked: + path = os.path.join(self.repo, rel) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as f: + f.write("original\n") + listed = tracked if volatile is None else volatile + with open(os.path.join(self.repo, "volatile-configs"), "w") as f: + f.write("# apps rewrite these in place\n") + f.write("".join(rel + "\n" for rel in listed)) + if makefile is not None: + with open(os.path.join(self.repo, "Makefile"), "w") as f: + f.write(makefile) + self.git("add", "-A") + self.git("commit", "-q", "-m", "fixture") + + def run_step(self): + env = dict(os.environ) + env["PATH"] = self.bindir + os.pathsep + env["PATH"] + env["REPO"] = self.repo + env["logfile"] = self.logfile + env["SUDO_CALLS"] = self.sudo_calls + return subprocess.run(["bash", self.wrapper, ARCHSETUP], + capture_output=True, text=True, env=env) + + def sudo_argv(self): + """The argument lines sudo was called with, one per invocation.""" + if not os.path.exists(self.sudo_calls): + return [] + with open(self.sudo_calls) as f: + return [line.rstrip("\n") for line in f if line.strip()] + + def skipped(self): + """Paths carrying the skip-worktree bit.""" + r = self.git("ls-files", "-v") + return sorted(line[2:] for line in r.stdout.splitlines() + if line.startswith("S ")) + + # ---------------------------------------------------------- normal ---- + def test_sets_skip_worktree_on_every_volatile_path(self): + paths = ["common/.config/btop/btop.conf", + "hyprland/.config/waypaper/config.ini"] + self.make_repo(tracked=paths) + self.assertEqual(self.skipped(), []) # the bit is not set by default + r = self.run_step() + self.assertIn("RETURNED=0", r.stdout, + "stdout=%r stderr=%r" % (r.stdout, r.stderr)) + self.assertEqual(self.skipped(), sorted(paths)) + + def test_runs_make_as_the_user_not_as_root(self): + # archsetup runs as root against a user-owned checkout. Letting root + # write .git/index leaves it root-owned, and the user's next git + # command then cannot update the index at all. + self.make_repo() + self.run_step() + self.assertEqual(self.sudo_argv(), + ["-u testuser make -C %s skip-volatile" % self.repo]) + + def test_leaves_non_volatile_tracked_files_alone(self): + self.make_repo(tracked=["common/.config/btop/btop.conf", + "common/.bashrc"], + volatile=["common/.config/btop/btop.conf"]) + self.run_step() + self.assertEqual(self.skipped(), ["common/.config/btop/btop.conf"]) + + # -------------------------------------------------------- boundary ---- + def test_no_makefile_is_a_quiet_no_op(self): + # DOTFILES_DIR can point at a checkout that predates the target, or at + # a fork. Nothing to delegate to is not an error. + self.make_repo(makefile=None) + r = self.run_step() + self.assertIn("RETURNED=0", r.stdout) + self.assertNotIn("WARN:", r.stdout) + self.assertEqual(self.skipped(), []) + + def test_empty_volatile_list_sets_nothing_and_does_not_warn(self): + self.make_repo(tracked=["common/.config/btop/btop.conf"], volatile=[]) + r = self.run_step() + self.assertIn("RETURNED=0", r.stdout) + self.assertNotIn("WARN:", r.stdout) + self.assertEqual(self.skipped(), []) + + def test_untracked_path_in_the_list_does_not_fail_the_step(self): + # The Makefile warns and carries on; a stale entry must not take the + # install down, and the tracked entries still get their bit. + self.make_repo(tracked=["common/.config/btop/btop.conf"], + volatile=["common/.config/btop/btop.conf", + "common/.config/gone/removed.conf"]) + r = self.run_step() + self.assertIn("RETURNED=0", r.stdout) + self.assertEqual(self.skipped(), ["common/.config/btop/btop.conf"]) + + # ----------------------------------------------------------- error ---- + def test_makefile_without_the_target_warns_and_returns(self): + self.make_repo(makefile="all:\n\t@true\n") + r = self.run_step() + self.assertIn("WARN:", r.stdout) + self.assertEqual(self.skipped(), []) + + def test_make_output_goes_to_the_logfile_not_the_console(self): + self.make_repo(tracked=["common/.config/btop/btop.conf"]) + r = self.run_step() + self.assertNotIn("skip-worktree:", r.stdout) + with open(self.logfile) as f: + self.assertIn("skip-worktree:", f.read()) + + +if __name__ == "__main__": + unittest.main() -- cgit v1.2.3