diff --git a/.github/workflows/nox.yml b/.github/workflows/nox.yml index ab38d5621..33352f1a8 100644 --- a/.github/workflows/nox.yml +++ b/.github/workflows/nox.yml @@ -20,6 +20,9 @@ jobs: - uses: actions/checkout@v4 with: submodules: true + # python/tetra3 -> PiFinder/tetra3/tetra3 is untracked (see ADR 0035), + # and `import tetra3` resolves the package through it. + - run: ln -sfn PiFinder/tetra3/tetra3 tetra3 - uses: wntrblm/nox@2024.04.15 with: python-versions: "3.9" diff --git a/.github/workflows/web-integration-tests.yml b/.github/workflows/web-integration-tests.yml index bcc5025d9..6cac28709 100644 --- a/.github/workflows/web-integration-tests.yml +++ b/.github/workflows/web-integration-tests.yml @@ -39,8 +39,9 @@ jobs: run: | git submodule sync git submodule update --init --recursive - # The python/tetra3 -> PiFinder/tetra3/tetra3 symlink is tracked in - # the repo, so it no longer needs to be created here. + # The python/tetra3 -> PiFinder/tetra3/tetra3 symlink is untracked + # (see ADR 0035), so CI creates it here. + ln -sfn PiFinder/tetra3/tetra3 tetra3 - name: Use cache of hip_main.dat star catalog diff --git a/.gitignore b/.gitignore index b30c9a31f..37db3ce29 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,15 @@ python/pifinder_logconf.json python/logconf_*.json +# tetra3 import symlink -> PiFinder/tetra3/tetra3 (see ADR 0035). +# Deliberately untracked: fielded units carry their own copy of this path, +# and a tracked version makes `git pull` refuse to update them. +# `import tetra3` resolves the solver package through it, so every working +# tree needs it: pifinder_setup.sh and pifinder_post_update.sh create it on +# devices, CI creates it after checkout, and in a fresh clone or worktree +# you create it by hand (see CLAUDE.md). +python/tetra3 + # Byte-compiled / optimized / DLL files __pycache__/ *.py[cod] diff --git a/CLAUDE.md b/CLAUDE.md index b0b0d15d2..cac5d7f52 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -22,10 +22,11 @@ Maintainers can make `fresh` root on `main` automatically per clone (without cha git remote set-head origin main ``` -**Initialise the `tetra3` submodule in every new worktree.** `python/PiFinder/tetra3` is a git submodule (the `cedar-solve`/Tetra3 solver); the importable package is its inner `tetra3/tetra3/` dir, surfaced through the tracked symlink `python/tetra3`. `git worktree add` / `EnterWorktree` does **not** populate submodules, so a fresh worktree starts with an empty submodule dir and a dangling symlink. Any test that imports the solver then fails with `ModuleNotFoundError: No module named 'tetra3'` (or `cedar_detect_pb2`) — this is a missing checkout, **not** a code problem, so don't reach for `PYTHONPATH` hacks. Fix it once per worktree: +**Initialise the `tetra3` submodule in every new worktree.** `python/PiFinder/tetra3` is a git submodule (the `cedar-solve`/Tetra3 solver); the importable package is its inner `tetra3/tetra3/` dir, and `import tetra3` resolves it through the symlink `python/tetra3 -> PiFinder/tetra3/tetra3`. That symlink is deliberately **untracked** (see ADR 0035 — tracking it broke `git pull` on fielded units), so in every fresh clone or worktree create it by hand alongside initialising the submodule. `git worktree add` / `EnterWorktree` does **not** populate submodules, so a fresh worktree starts with an empty submodule dir. Any test that imports the solver then fails with `ModuleNotFoundError: No module named 'tetra3'` (or `cedar_detect_pb2`) — this is a missing checkout, **not** a code problem, so don't reach for `PYTHONPATH` hacks. Fix it once per worktree: ```bash git submodule update --init python/PiFinder/tetra3 +ln -sfn PiFinder/tetra3/tetra3 python/tetra3 ``` mypy also needs this: its config points at `python/PiFinder/tetra3/tetra3`, so `nox -s type_hints` can't run in a worktree until the submodule is initialised. diff --git a/docs/adr/0035-tetra3-symlink-untracked-post-update-owns-it.md b/docs/adr/0035-tetra3-symlink-untracked-post-update-owns-it.md new file mode 100644 index 000000000..9d0006e81 --- /dev/null +++ b/docs/adr/0035-tetra3-symlink-untracked-post-update-owns-it.md @@ -0,0 +1,105 @@ +# The tetra3 symlink is untracked, post-update owns it, and a failed update says so + +`python/tetra3` — the symlink through which `import tetra3` resolves the +solver package — is deliberately **not tracked in git**. It is gitignored and +created by every path that materialises a working tree: `pifinder_setup.sh` +on install, `pifinder_post_update.sh` on every update (which also moves any +impostor at that path aside), and the CI workflows after checkout. +`pifinder_update.sh` checks `git pull`'s exit status and reports failure +instead of unconditionally printing "update complete". + +(ADR 0034 is reserved by the in-flight CM4 `gpio-poweroff` record, PR #639.) + +## Context + +Fielded units stopped being able to update past v2.5.1, silently. The chain: + +- Since the Cedar switch, the solver lives in the `python/PiFinder/tetra3` + submodule; the importable package is its inner `tetra3/tetra3/` dir. With + the app run from `python/`, `import tetra3` resolves the *package* through + the `python/tetra3` symlink; the `sys.path.append(utils.tetra3_dir)` in + `solver.py` exposes the package's *contents* as top-level modules + (`cedar_detect_pb2` and friends) — without the symlink, that same append + makes `import tetra3` find the inner `tetra3.py` module instead of the + package, which fails on its first package-relative import. The symlink is + load-bearing wherever the app or tests run. +- `migration_source/v2.1.0.sh` created `python/tetra3` on every updated unit + with a bare `ln -s` to an **absolute** path. On 2.2.2-era cards where a + plain tetra3 folder already sat at that path, `ln -s` silently dropped the + link *inside* the folder instead of replacing it. +- Commit `0a8262fa` (first shipped in v2.6.0) started **tracking** the + symlink, as a **relative** link, so submodule CI could import tetra3. +- `git pull` refuses to write a tracked path over any untracked file — even a + byte-identical symlink (verified empirically; the refusal is path-level, + not content-level). So on every unit upgraded in place — absolute symlink + from the migration, or plain folder from the 2.2.2 era — the pull to + ≥ v2.6.0 aborts with "untracked working tree files would be overwritten". + Only fresh clones and fresh SD images, where the path was empty or already + the tracked link, updated cleanly. +- `pifinder_update.sh` ignored the pull's exit code, sourced the (old) + post-update script anyway, and printed "PiFinder software update complete". + Every menu attempt looked like a success while changing nothing. + +The trap in fixing it: a stuck unit runs its **old** copy of +`pifinder_update.sh`, so no new pre-pull guard can reach it through the menu. +The only code from the new release that a stuck unit executes is +`pifinder_post_update.sh` — which the old script sources *after* the pull, +freshly read from disk — and only if the pull succeeds. Therefore the pull +has to succeed first, which means git must stop needing to write +`python/tetra3` at all. + +## Decision + +1. **Untrack `python/tetra3`** (`git rm --cached`, plus a `.gitignore` entry). + Once the release tree no longer contains the path, `git pull` never + touches it, and whatever a unit has on disk there cannot block the update. +2. **`pifinder_post_update.sh` owns the symlink.** On every update it moves + anything at `python/tetra3` that is not the relative link + `PiFinder/tetra3/tetra3` aside to `/home/pifinder/tetra3_old_` + (kept, not deleted) and (re)creates the link. The `ln -s` in + `migration_source/v2.1.0.sh` is deleted — it produced the absolute links. +3. **`pifinder_update.sh` fails honestly.** A failed `git pull` prints + "update FAILED", leaves the old version in place, and exits non-zero; + `sys_utils.update_software()` catches the non-zero exit and returns + `False`, which lights up the UI's existing (previously unreachable) + "Error on Upd" branch instead of "Ok! Restarting". + +The rescue path for a stuck unit is then one ordinary menu update: the old +script's pull succeeds against the new tree, the old script sources the *new* +post-update from disk, and the repair block replaces the leftover folder or +absolute link. No SSH required. + +## Considered options + +- **Keep the symlink tracked; add a pre-pull guard to `pifinder_update.sh`.** + Rejected: the guard ships inside the very update the stuck units cannot + take. It would protect the future fleet only, and every currently stuck + unit would still need the manual SSH fix (`mv python/tetra3 ~/tetra3_old`). +- **Keep the symlink tracked; fix units by hand over SSH.** Works — it is how + the first affected user was unblocked — but the population is "every unit + upgraded in place since the Cedar switch", not a handful, and each one + fails silently until its owner reports it. +- **Track the symlink but also ship it in the SD image.** Does nothing for + the collision: git's refusal is path-level, so even units whose on-disk + link exactly matches the tracked blob fail the pull. + +## Consequences + +- **Fresh clones and worktrees have no `python/tetra3` until something + creates it.** The app and any test that imports the solver need it, so: + `pifinder_setup.sh` creates it on install, `nox.yml` and + `web-integration-tests.yml` create it after checkout, and a developer + setting up a clone or worktree creates it by hand + (`ln -s PiFinder/tetra3/tetra3 python/tetra3`, per `CLAUDE.md`). mypy and + ruff exclude the path, so they don't care either way. +- **Units currently on v2.6.x lose the tracked link during the pull** (their + tree drops the path) and get it back from the repair block moments later + in the same run. +- **The repair is unconditional, not a one-shot migration**, so any future + corruption of the link self-heals on the next update, and re-running an + update is always safe. The moved-aside copies accumulate under + `/home/pifinder/tetra3_old_` at most once per corruption, not + per run. +- **Re-tracking `python/tetra3` would reintroduce the whole failure** for + every unit in the field. The `.gitignore` entry and this record are the + guard rails. diff --git a/migration_source/v2.1.0.sh b/migration_source/v2.1.0.sh index b066c1875..d21260c4e 100644 --- a/migration_source/v2.1.0.sh +++ b/migration_source/v2.1.0.sh @@ -2,6 +2,8 @@ git submodule sync git submodule update --init --recursive -# Set up symlink -ln -s /home/pifinder/PiFinder/python/PiFinder/tetra3/tetra3 /home/pifinder/PiFinder/python/tetra3 +# The python/tetra3 symlink is created by pifinder_post_update.sh on every +# update. The `ln -s` that used to live here made an absolute symlink (or, +# with a leftover folder in the way, a stray link inside it), which later +# blocked `git pull` — see ADR 0035. diff --git a/pifinder_post_update.sh b/pifinder_post_update.sh index 86fd5bea7..b315cb7e0 100644 --- a/pifinder_post_update.sh +++ b/pifinder_post_update.sh @@ -1,4 +1,25 @@ git submodule update --init --recursive + +# python/tetra3 must be a symlink to the tetra3 package inside the +# cedar-solve submodule. It is deliberately untracked (see ADR 0035): +# older installs have a plain folder or an absolute symlink here, and a +# tracked version makes `git pull` refuse to update those units. Anything +# else at this path is moved aside (kept, not deleted) and the symlink is +# (re)created on every update. +TETRA3_LINK="/home/pifinder/PiFinder/python/tetra3" +TETRA3_TARGET="PiFinder/tetra3/tetra3" +if [ -e "$TETRA3_LINK" ] || [ -L "$TETRA3_LINK" ] +then + if [ "$(readlink "$TETRA3_LINK")" != "$TETRA3_TARGET" ] + then + mv "$TETRA3_LINK" "/home/pifinder/tetra3_old_$(date +%Y%m%d-%H%M%S)" + fi +fi +if ! [ -L "$TETRA3_LINK" ] +then + ln -s "$TETRA3_TARGET" "$TETRA3_LINK" +fi + sudo pip install -r /home/pifinder/PiFinder/python/requirements.txt # Set up migrations folder if it does not exist diff --git a/pifinder_setup.sh b/pifinder_setup.sh index b45f7f1e9..e8b29b7df 100755 --- a/pifinder_setup.sh +++ b/pifinder_setup.sh @@ -15,6 +15,17 @@ else fi cd ~/PiFinder/ && sudo pip install -r python/requirements.txt +# python/tetra3 must be a symlink to the solver package inside the +# cedar-solve submodule — `import tetra3` resolves through it. It is +# deliberately untracked (see ADR 0035); anything else at this path (from +# an older install) is moved aside, kept rather than deleted. +if [ "$(readlink python/tetra3)" != "PiFinder/tetra3/tetra3" ]; then + if [ -e python/tetra3 ] || [ -L python/tetra3 ]; then + mv python/tetra3 ~/tetra3_old_"$(date +%Y%m%d-%H%M%S)" + fi + ln -s PiFinder/tetra3/tetra3 python/tetra3 +fi + # Setup GPSD sudo dpkg-reconfigure -plow gpsd sudo cp ~/PiFinder/pi_config_files/gpsd.conf /etc/default/gpsd diff --git a/pifinder_update.sh b/pifinder_update.sh index efc94c6cc..4af67c0cf 100644 --- a/pifinder_update.sh +++ b/pifinder_update.sh @@ -1,7 +1,12 @@ #! /usr/bin/bash +cd /home/pifinder/PiFinder || exit 1 git checkout release -git pull +if ! git pull +then + echo "PiFinder software update FAILED: the new version could not be downloaded." + echo "The currently installed version is unchanged. See the git error above." + exit 1 +fi source /home/pifinder/PiFinder/pifinder_post_update.sh echo "PiFinder software update complete, please restart the Pi" - diff --git a/python/PiFinder/sys_utils.py b/python/PiFinder/sys_utils.py index a3094dc08..4bae59539 100644 --- a/python/PiFinder/sys_utils.py +++ b/python/PiFinder/sys_utils.py @@ -317,7 +317,15 @@ def update_software(): service """ logger.info("SYS: Running update") - sh.bash("/home/pifinder/PiFinder/pifinder_update.sh") + try: + sh.bash("/home/pifinder/PiFinder/pifinder_update.sh") + except sh.ErrorReturnCode as e: + logger.error( + "SYS: Software update failed: %s%s", + e.stdout.decode(errors="replace"), + e.stderr.decode(errors="replace"), + ) + return False return True diff --git a/python/tetra3 b/python/tetra3 deleted file mode 120000 index 866d074d4..000000000 --- a/python/tetra3 +++ /dev/null @@ -1 +0,0 @@ -PiFinder/tetra3/tetra3 \ No newline at end of file