From 06f45e83c1dbf16f5f3c6131ea8615321453ae9a Mon Sep 17 00:00:00 2001 From: Frank Date: Wed, 21 Oct 2026 00:54:31 -0700 Subject: [PATCH] refactor(packages): one manifest parser, so a name means one thing to both tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit install.sh and pkg-inventory.sh carried byte-identical copies of read_manifest. That is the duplicated-code half of the problem; the worse half is untested — neither script had any test for the grammar that decides what gets installed. - scripts/lib/manifest.sh is the parser; both scripts source it - tests/manifest-parser.bats writes the contract first (comments, CRLF, first field only, unreadable file fails loudly instead of yielding "nothing desired") - an unreadable manifest now fails inside the helper rather than depending on whichever `set` options the caller happens to run under - install.sh check mode verified end to end against a stubbed pacman: a CRLF manifest with trailing comments resolves to bare names, installed names drop Refs #60 --- omp-docs/tools/packages.md | 13 +++++++--- scripts/install.sh | 7 +++--- scripts/lib/manifest.sh | 23 +++++++++++++++++ tests/manifest-parser.bats | 51 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 8 deletions(-) create mode 100755 scripts/lib/manifest.sh create mode 100755 tests/manifest-parser.bats diff --git a/omp-docs/tools/packages.md b/omp-docs/tools/packages.md index f685f13..c01abfd 100644 --- a/omp-docs/tools/packages.md +++ b/omp-docs/tools/packages.md @@ -25,9 +25,13 @@ if both directions are checked. ## Manifests `packages/common.pkgs` (every host) + `packages/.pkgs` (this host) + `packages/aur.pkgs` -(AUR-only, installed through `yay`). The parser is `sed 's/#.*//'` then the first field, so `#` -comments trail freely and one token per line is the whole grammar. An unknown `--host` gets the -common manifest only: a wrong-host run is a no-op, not a hazard. +(AUR-only, installed through `yay`). **One parser reads them for both tools**: +`scripts/lib/manifest.sh` — `sed 's/#.*//'` then the first field, so `#` comments trail freely and +one token per line is the whole grammar; an unreadable manifest is a loud failure rather than an +empty desired list. `tests/manifest-parser.bats` owns that contract. The two scripts carried +verbatim copies of the body until 2026-10-21, which is exactly how a manifest line can come to mean +two things to the two halves of the same check. An unknown `--host` gets the common manifest only: +a wrong-host run is a no-op, not a hazard. ## Rebuild a workstation @@ -110,5 +114,6 @@ never teach a fresh box to install. - [tools/index](index.md) · [dotfiles-deploy](dotfiles-deploy.md) (the config tier that follows) · [live-sync](live-sync.md) (the heartbeat that commits machine state) -- Tests: `tests/pkg-inventory.bats` (fixtures + stubbed `pacman`, no real package database touched). +- Tests: `tests/pkg-inventory.bats` (fixtures + stubbed `pacman`, no real package database touched) + and `tests/manifest-parser.bats` (the grammar above, observed as names in / names out). - Refs #51. diff --git a/scripts/install.sh b/scripts/install.sh index 4f787a2..dd2c001 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -17,7 +17,8 @@ # otherwise sudo prompts normally. The value is never stored in this repo. set -euo pipefail -REPO="$(git -C "$(dirname "$(realpath "$0")")" rev-parse --show-toplevel 2>/dev/null || true)" +HERE="$(dirname "$(realpath "$0")")" +REPO="$(git -C "$HERE" rev-parse --show-toplevel 2>/dev/null || true)" REPO="${INSTALL_REPO:-$REPO}" [ -n "$REPO" ] && [ -d "$REPO/packages" ] || { echo "install.sh: no packages/ under ${REPO:-}" >&2; exit 1; } @@ -36,9 +37,7 @@ SUDO() { else sudo "$@"; fi } -read_manifest() { # $1 = manifest path; emits package names, strips comments/blanks - sed 's/#.*//' "$1" | tr -d '\r' | awk '{if ($1) print $1}' -} +source "$HERE/lib/manifest.sh" # the same parser pkg-inventory.sh reads with # ---- desired-minus-installed delta ------------------------------------------ PKGS=() for m in common.pkgs "$HOST.pkgs"; do diff --git a/scripts/lib/manifest.sh b/scripts/lib/manifest.sh new file mode 100755 index 0000000..c20284e --- /dev/null +++ b/scripts/lib/manifest.sh @@ -0,0 +1,23 @@ +#!/usr/bin/env bash +# scripts/lib/manifest.sh — the one parser for packages/*.pkgs +# +# install.sh walks desired → installed; pkg-inventory.sh walks installed → declared. Both read the +# same hand-written manifests, so both must agree on what a line MEANS: if one forgives an inline +# comment and the other does not, the same name is simultaneously "desired" and "drift", and the +# pair stops being a check on the machine. Source it — do not paste the body into a caller again +# (Refs #60: it was pasted twice, in install.sh and in pkg-inventory.sh). +# +# Contract, and tests/manifest-parser.bats is that contract: +# name first field of a line, emitted as one bare token — callers hand it to pacman +# comment `#` and everything after it is dropped, whole-line or trailing +# CRLF stripped (the manifests are edited on more than one box) +# blank empty and whitespace-only rows vanish +# unreadable non-zero exit, no names: a silent empty list reads as "nothing desired", and a +# rebuild would then install nothing while reporting success +# +# Sourced, never executed; callers own `set -euo pipefail`. + +read_manifest() { # $1 = manifest path; prints one package name per line + [[ -r "$1" ]] || { printf 'read_manifest: cannot read %s\n' "$1" >&2; return 2; } + sed 's/#.*//' "$1" | tr -d '\r' | awk '{if ($1) print $1}' +} diff --git a/tests/manifest-parser.bats b/tests/manifest-parser.bats new file mode 100755 index 0000000..6641e15 --- /dev/null +++ b/tests/manifest-parser.bats @@ -0,0 +1,51 @@ +#!/usr/bin/env bats +# tests/manifest-parser.bats — the one manifest parser, shared by install.sh and pkg-inventory.sh +# +# `packages/*.pkgs` is hand-written, so the parser's whole job is to forgive the ways a hand-edited +# list is wrong. The same function decides what install.sh tries to install and what pkg-inventory +# calls drift, so a manifest line must mean the same thing to both — which is why it lives in one +# file instead of being pasted into each caller (Refs #60: it was pasted twice). +# +# Observed through the function's exit status and stdout only: +# names first field of a line, one bare token; `#` and everything after it is a comment +# CRLF tolerated (the manifests get edited on more than one box) +# blank lines vanish +# unreadable non-zero with no names — an empty desired list must never be a silent success, +# because "nothing desired" is exactly what a rebuild would install + +setup() { + TEST_TMP="$(mktemp -d)" + LIB="$BATS_TEST_DIRNAME/../scripts/lib/manifest.sh" +} + +teardown() { rm -rf "$TEST_TMP"; } + +parse() { # $1 = manifest contents, verbatim + printf '%s' "$1" > "$TEST_TMP/m.pkgs" + run bash -c 'source "$1"; read_manifest "$2"' _ "$LIB" "$TEST_TMP/m.pkgs" +} + +@test "inline notes, full-line comments, blank rows and CRLF come out as bare names" { + parse $'# desired state\r\nghostty # terminal\r\n\r\n \r\nzoxide\r\n' + [ "$status" -eq 0 ] + [ "$(printf '%s\n' "$output" | tr '\n' ' ')" = "ghostty zoxide " ] +} + +@test "only the first field of a line is a package" { + # install.sh hands every emitted token straight to pacman, so a stray second token on a line + # must never become an install target; whatever it is, the manifest did not declare it. + parse $'neovim extra tokens here\n' + [ "$output" = "neovim" ] +} + +@test "a comment that names a package is not a package" { + parse $'# vlc\n#typora\nbun\n' + [ "$output" = "bun" ] +} + +@test "an unreadable manifest fails loudly instead of yielding an empty desired list" { + run --separate-stderr bash -c 'source "$1"; read_manifest "$2"' _ "$LIB" "$TEST_TMP/nope.pkgs" + [ "$status" -ne 0 ] + [ -z "$output" ] + [[ "$stderr" == *nope.pkgs* ]] +}