diff --git a/CHANGELOG.md b/CHANGELOG.md index 98ec0b8c..c36f3a91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,38 @@ All notable changes to the Lager platform are documented here. For detailed rele and false of the file that grants `apt-get` one line above them. Treat anyone holding the box login account's SSH key as holding root on that box. +- **Cold box image builds spend less time installing things nothing uses.** + Three changes to `box.Dockerfile`, all of them about build time rather than + behavior: + + Node and npm now come from the official upstream tarball, verified against + its `SHASUMS256.txt` before extraction, instead of Debian's `nodejs npm` + meta-packages — which pull in roughly 400 `node-*` packages the box never + touches. `start_box.sh` needs npm only to install the packages named in + `box_config.npm_packages`, which the tarball provides. + + `cryptography` moves from 38.0.4 to 43.0.3. The old pin has no cp312 wheel, + so every cold pip layer compiled it from Rust source; 43.0.3 ships a + manylinux wheel for the image's Python. The BluFi cipher, its only consumer + in this tree, switches from `algorithms.AES128` to `algorithms.AES` — stable + across both versions and byte-identical for BluFi's fixed 16-byte key. + + `flex` and `bison` move into the uldaq layer, the only stage whose + `autoreconf` needs them. `ccache` and `ninja-build` are dropped outright: + nothing in this repo invokes either, and no build here was wired to use + them. + + **A box carrying globally-installed npm packages should be updated once with + `--force`.** Node's major version moves from 18 to 20, and the + `lager-npm-global` volume holding those packages survives an ordinary image + rebuild — only `--force` wipes it. Any package with a compiled native module + needs reinstalling under the new ABI. + +- **The update progress bar names what the container build is currently + doing** — `Building container... [pip install ...]` — instead of holding one + unchanging label for the several minutes a cold build takes. Parsed from + BuildKit's own step output; `--verbose` is unchanged. + ### Fixed - **Every successful `lager update` that rebuilt the container warned that its @@ -117,6 +149,30 @@ All notable changes to the Lager platform are documented here. For detailed rele gate would rebuild. `--check`'s exit code now accounts for a pending flatten too, so a box needing one no longer reports `Nothing to do`. +- **`lager update --check` still promised a cached build when the ref it was + about to check out changed the image recipe.** The companion to the case + above, and the more common one. The probe measured the Dockerfile, + requirements and box source in the box's *current* working tree — which on a + box a long way behind its target still matched `/etc/lager/build-hash` + exactly. The preview printed `Estimated: ~90s (cached build)`; the pull then + landed a different Dockerfile and the update took the full six minutes. + + `--check` now reads those same build inputs at the target ref, straight out + of the box's git object database via `git cat-file` / `git show` — no + checkout, no mutation, and one extra SSH round-trip on the `--check` path + only. The snippet is composed to emit a byte-identical digest to the + working-tree hasher for an identical tree, so the two are comparable by + construction rather than by coincidence; tests execute both under `sh` + against a fixture repo and assert the digests agree. + + When the target ref can be measured it replaces the working tree as the + basis for the whole preview, which also turns the old + `unknown until pull (older ref may differ)` guess on rollbacks and branch + switches into a measured answer. When it cannot be measured — a sparse + checkout, an odd ref — the preview says so rather than falling back to the + pre-pull tree and calling the cache valid. A pending flatten still forces a + rebuild whatever the target digest says. + - **The sudoers bootstrap snippet printed by `lager box-config mount` wrote a strict subset of the file it was overwriting.** It teed a single `NOPASSWD: /bin/mkdir, /bin/chown` line over `/etc/sudoers.d/lager-box-config`, diff --git a/box/lager/blufi/security/aes.py b/box/lager/blufi/security/aes.py index 2bcd4cc3..5d7972de 100644 --- a/box/lager/blufi/security/aes.py +++ b/box/lager/blufi/security/aes.py @@ -8,9 +8,9 @@ # path with removal upstream calls imminent but has not scheduled. from cryptography.hazmat.decrepit.ciphers.modes import CFB except ImportError: - # Older installs predate the decrepit package entirely -- the box - # runtime pins cryptography==38.0.4 (box.Dockerfile) and the unit - # floor is >=42. Same class either way, byte-identical output. + # Older installs predate the decrepit package entirely. Box runtime + # pins cryptography==43.0.3 (box.Dockerfile); unit floor is >=42. + # Same class either way, byte-identical output. from cryptography.hazmat.primitives.ciphers.modes import CFB # https://cryptography.io/en/latest/hazmat/primitives/symmetric-encryption/#cryptography.hazmat.primitives.ciphers.algorithms.AES @@ -19,7 +19,10 @@ class BlufiAES(object): def __init__(self, key, iv): self.key = key self.iv = iv - self.cipher = Cipher(algorithms.AES128(self.key), CFB(self.iv)) + # AES128 was a fixed-size alias; AES(key) is the stable API across + # cryptography versions (and what manylinux wheels for 3.12 expect). + # BluFi always uses a 16-byte key. + self.cipher = Cipher(algorithms.AES(self.key), CFB(self.iv)) self.encryptor = self.cipher.encryptor() self.decryptor = self.cipher.decryptor() diff --git a/box/lager/docker/box.Dockerfile b/box/lager/docker/box.Dockerfile index c8f456b0..0a36d031 100644 --- a/box/lager/docker/box.Dockerfile +++ b/box/lager/docker/box.Dockerfile @@ -5,16 +5,21 @@ ENV PYTHONDONTWRITEBYTECODE=1 ENV PYTHONUNBUFFERED=1 ENV PYTHONPATH=/app/lager +# flex/bison are not here: they moved to the uldaq stage below, the only +# place that needs them (autoreconf). ccache/ninja-build are dropped +# outright -- nothing in this repo invokes either and no build here was +# wired to use them, so they were pure image weight; a container script +# that wants them has to install them itself. Node comes from the official +# tarball below rather than Debian's `nodejs npm` meta-packages, which pull +# ~400 unused `node-*` packages and dominate cold-build time. RUN apt-get update && apt-get install -y ca-certificates libusb-1.0-0-dev libudev-dev \ libhidapi-dev git gcc python-dev-is-python3 python3-pip python3-venv xz-utils build-essential bluetooth ssh openssh-client \ cups-client lpr zlib1g-dev wget libjpeg-dev libpng-dev libfreetype6-dev \ fswebcam automake g++ libtool libleptonica-dev make pkg-config libpango1.0-dev gdb-multiarch tesseract-ocr libtesseract-dev \ - flex bison ccache ninja-build \ libturbojpeg0-dev v4l-utils \ wireless-tools \ tini \ gnupg \ - nodejs npm \ openocd \ && wget -qO /usr/share/keyrings/phidgets.gpg https://www.phidgets.com/gpgkey/pubring.gpg \ && echo deb [signed-by=/usr/share/keyrings/phidgets.gpg] http://www.phidgets.com/debian bookworm main > /etc/apt/sources.list.d/phidgets.list \ @@ -27,14 +32,33 @@ RUN apt-get update && apt-get install -y ca-certificates libusb-1.0-0-dev libude virtualenv \ && : +# Node.js + npm from the official binary tarball (~50MB) instead of apt's +# `nodejs` meta-package (~500MB of unused node-* tooling). Needed at runtime +# so start_box.sh can `npm install -g` box_config.npm_packages. Multi-arch: +# dpkg arch → node dist arch. +# Pin + verify: Debian-signed apt packages are replaced by an upstream tarball, +# so HTTPS alone is not enough — check SHASUMS256.txt before extracting. +ENV NODE_VERSION=20.18.1 +RUN arch="$(dpkg --print-architecture)" \ + && case "$arch" in \ + amd64) node_arch=x64 ;; \ + arm64) node_arch=arm64 ;; \ + armhf) node_arch=armv7l ;; \ + *) echo "unsupported arch for Node.js: $arch" >&2; exit 1 ;; \ + esac \ + && cd /tmp \ + && tarball="node-v${NODE_VERSION}-linux-${node_arch}.tar.xz" \ + && wget -q "https://nodejs.org/dist/v${NODE_VERSION}/${tarball}" \ + && wget -q "https://nodejs.org/dist/v${NODE_VERSION}/SHASUMS256.txt" \ + && grep " ${tarball}$" SHASUMS256.txt | sha256sum -c - \ + && tar -xJf "${tarball}" --strip-components=1 -C /usr/local \ + && rm -f "${tarball}" SHASUMS256.txt \ + && node --version && npm --version + # PicoScope SDK is mounted from host at runtime via start_box.sh # The SDK library is at /opt/picoscope/lib/libps2000.so on the host # This avoids the systemd reload error that occurs when installing in Docker - - - - # MCC uldaq C library -- required by uldaq Python bindings for USB-202 DAQ devices # See: https://github.com/mccdaq/uldaq # @@ -56,7 +80,10 @@ RUN apt-get update && apt-get install -y ca-certificates libusb-1.0-0-dev libude # a memory-safety bug rather than close it, so the warnings stay: noisy once # per image build, and honest. Fixing it means patching upstream source at # build time or dropping the library -- upstream has not released since 2022. -RUN apt-get update && apt-get install -y autoconf automake libtool libusb-1.0-0-dev && rm -rf /var/lib/apt/lists/* \ +# +# flex/bison stay in this layer only (autoreconf); not needed at runtime. +RUN apt-get update && apt-get install -y autoconf automake libtool libusb-1.0-0-dev flex bison \ + && rm -rf /var/lib/apt/lists/* \ && git clone --depth 1 --branch v1.2.1 https://github.com/mccdaq/uldaq.git /tmp/uldaq \ && cd /tmp/uldaq \ && autoreconf -i \ @@ -111,7 +138,9 @@ RUN --mount=type=cache,target=/root/.cache/pip,sharing=locked \ 'simplejson==3.18.0' \ 'labjack-ljm==1.23.0' \ 'pygdbmi==0.11.0.0' \ - 'cryptography==38.0.4' \ + # 38.0.4 has no cp312 wheel — every cold pip layer compiled Rust from + # source (~2-3 min). >=42 ships manylinux wheels for python 3.12. + 'cryptography==43.0.3' \ 'psycopg2-binary==2.9.9' \ 'pyvisa-py==0.5.2' \ 'PyVISA==1.11.3' \ diff --git a/cli/commands/utility/update.py b/cli/commands/utility/update.py index 6323c730..baf0eee1 100644 --- a/cli/commands/utility/update.py +++ b/cli/commands/utility/update.py @@ -205,10 +205,23 @@ def _flatten_shell_cmd(): # Files whose contents legitimately invalidate the cached Docker image. # Dockerfile is the universal one; requirements.txt may not exist on every box # revision, so we sha256sum it only when present. +# +# Working-tree paths (post-flatten layout on the box) vs git blob paths (repo +# keeps the `box/` prefix). Target-ref hashing uses the git paths so --check +# can compare the upcoming pull's build inputs to /etc/lager/build-hash without +# checking it out. Individual blobs are listed separately from the source tree +# because `_build_hash_shell_cmd` hashes `_BUILD_HASH_INPUTS` first and then +# walks `_BUILD_HASH_SOURCE_DIRS` — Dockerfile/requirements therefore appear +# twice in the aggregate, and the at-ref hasher must match that composition. _BUILD_HASH_INPUTS = [ '~/box/lager/docker/box.Dockerfile', '~/box/lager/requirements.txt', ] +_BUILD_HASH_GIT_BLOBS = [ + # (git path under ~/box, absolute working-tree path after flatten) + ('box/lager/docker/box.Dockerfile', '~/box/lager/docker/box.Dockerfile'), + ('box/lager/requirements.txt', '~/box/lager/requirements.txt'), +] # Source trees whose contents also invalidate the cached image. The Dockerfile # `COPY`s these into the image, so a pure-Python change must wipe it: without @@ -220,6 +233,9 @@ def _flatten_shell_cmd(): _BUILD_HASH_SOURCE_DIRS = [ '~/box/lager', ] +# Git-tree prefix corresponding to `_BUILD_HASH_SOURCE_DIRS` after flatten +# (`box/lager/...` blob → `$HOME/box/lager/...` working-tree path). +_BUILD_HASH_GIT_SOURCE_PREFIX = 'box/lager' def _build_hash_shell_cmd(): @@ -267,6 +283,59 @@ def _build_hash_shell_cmd(): ) +def _build_hash_at_ref_shell_cmd(git_ref): + """Shell snippet: hash build inputs at ``git_ref`` without checking out. + + Emits the same aggregate sha256 as `_build_hash_shell_cmd` when the + working tree matches ``git_ref`` for those files — ``sha256sum`` lines use + the post-flatten absolute paths so they compose identically (including the + deliberate double-count of Dockerfile/requirements: once via the individual + inputs list, once via the source-tree walk). Missing blobs at the ref are + skipped (same as a missing working-tree file). Empty output means nothing + was measurable. + """ + # Sanitize: only allow refs that git will accept as a single argument + # (branch, tag, SHA, origin/main). Reject shell metacharacters. + if not git_ref or any(c in git_ref for c in ' \t\n\r;|&$`\\"\'<>(){}[]'): + return 'echo ""' + # POSIX only — the remote login shell may be dash, and the repo's probe + # tests execute these snippets under `sh`. No bash pattern substitution + # and no `printf -v`. + # + # `git show | sha256sum` prints " -"; the sed rewrites the "-" to + # the absolute working-tree path so each line is byte-identical to what + # `sha256sum ` produces in `_build_hash_shell_cmd`. The aggregate + # then uses the same `out=$(...)` + `echo "$out" | sha256sum` composition, + # so a ref whose blobs match the working tree yields the same digest. + clauses = [] + for git_path, abs_tilde in _BUILD_HASH_GIT_BLOBS: + abs_shell = abs_tilde.replace('~', '$HOME', 1) + # `&&` inside a clause (skip missing blobs), `;` between clauses so a + # missing requirements.txt does not suppress the Dockerfile line. + clauses.append( + f'git cat-file -e {git_ref}:{git_path} 2>/dev/null && ' + f'git show {git_ref}:{git_path} | sha256sum | ' + f'sed "s| -$| {abs_shell}|"' + ) + # Source-tree walk: same files as `find ~/box/lager ... | sort -z`, but + # read from the git object database. Paths are mapped from the pre-flatten + # git prefix (`box/lager/...`) to the post-flatten absolute path. + src_prefix = _BUILD_HASH_GIT_SOURCE_PREFIX + clauses.append( + f'git ls-tree -r --name-only {git_ref} {src_prefix} 2>/dev/null | ' + f'grep -v "/__pycache__/" | grep -v "\\.pyc$" | sort | ' + f'while IFS= read -r path; do ' + f'abs="$HOME/box/${{path#box/}}"; ' + f'git show {git_ref}:"$path" | sha256sum | sed "s| -$| $abs|"; ' + f'done' + ) + return ( + 'out=$(cd "$HOME/box" 2>/dev/null && { ' + + '; '.join(clauses) + + '; }); [ -n "$out" ] && echo "$out" | sha256sum | cut -d" " -f1' + ) + + def _read_build_hash(ssh_runner): """Return the sha256 of the box's *current* docker-build inputs. @@ -279,6 +348,12 @@ def _read_build_hash(ssh_runner): return r.stdout.strip() if r.returncode == 0 else '' +def _read_build_hash_at_ref(ssh_runner, git_ref): + """Return the sha256 of docker-build inputs at ``git_ref`` (no checkout).""" + r = ssh_runner(_build_hash_at_ref_shell_cmd(git_ref)) + return r.stdout.strip() if r.returncode == 0 else '' + + def _read_box_source_version(ssh_runner): """Return the `__version__` string declared in `cli/__init__.py` at the box's current HEAD, or empty. @@ -447,6 +522,49 @@ def _pull_shell_script(target_version, git_ref): ) +def _docker_build_line_summary(line): + """Extract a short human label from a BuildKit / docker build log line. + + Returns None for noise (blank, pure progress hashes, cache hits with no + payload). Used to feed ``ProgressBar.set_detail`` during the container + build so the bar shows *what* is slow. + """ + s = (line or '').strip() + if not s: + return None + # BuildKit: "#15 3.2 Setting up nodejs (20.x)" or "#12 [5/20] RUN pip ..." + if s.startswith('#'): + # Drop the leading "#N" / "#N M.M" prefix. + rest = s.lstrip('#').split(None, 1) + if len(rest) < 2: + return None + payload = rest[1] + # "#12 0.5 " timed lines — strip leading seconds if present. + parts = payload.split(None, 1) + if parts and parts[0].replace('.', '', 1).isdigit() and len(parts) > 1: + payload = parts[1] + payload = payload.strip() + if not payload or payload.startswith('DONE ') or payload == 'CACHED': + return None + return payload[:60] + # Classic builder / pip noise that still indicates progress. + for prefix in ( + 'Step ', + 'Collecting ', + 'Downloading ', + 'Building wheel ', + 'Installing collected', + 'Setting up ', + 'Unpacking ', + 'Compiling ', + 'Downloading crates', + 'Installing ', + ): + if s.startswith(prefix): + return s[:60] + return None + + def _build_hash_mismatch(new_hash, stored_hash): """True when the docker-build inputs changed relative to the last successful build. @@ -486,7 +604,7 @@ def _rebuild_gate_verdict(facts, *, git_sync_confirmed, needs_pull, def _deps_preview(new_hash, stored_hash, *, force, needs_flatten, - is_rollback, commits_ahead): + is_rollback, commits_ahead, target_hash='', needs_pull=False): """What `--check` reports about the docker-build cache. Returns ``(status_text, rebuild_certain, unmeasured)``. @@ -498,7 +616,7 @@ def _deps_preview(new_hash, stored_hash, *, force, needs_flatten, "Deps: cache valid (no rebuild) / Estimated: ~90s (cached build)" immediately before a ten-minute clean rebuild. - Two distinct failures were behind that: + Three distinct failures were behind that: * `needs_flatten` was ignored here, though the gate treats it as a definite rebuild trigger. The flatten moves every source file, and the @@ -506,36 +624,60 @@ def _deps_preview(new_hash, stored_hash, *, force, needs_flatten, its digest — so a moved file necessarily changes it. * `_build_hash_mismatch` returns False both for "measured, unchanged" and for "could not measure", and this rendered both as "cache valid". + * The only digest on hand was the *pre-pull* working tree's, so a forward + jump whose current tree still matched the stored hash read as "cache + valid" however much the target ref changed the image recipe. + + ``target_hash`` closes the third: the same aggregate digest, measured + against the ref about to be checked out and read straight out of the + object DB without a checkout (`_read_build_hash_at_ref`). When a pull is + coming and that measurement succeeded it — not the working tree — is what + the build will be keyed on, which also turns the old "older ref may + differ" guess into a measured answer. An empty ``target_hash`` means the + measurement failed (sparse checkout, odd ref) and the pre-pull tree is + all there is; that is reported as unknown rather than as cache-valid. `unmeasured` deliberately does NOT imply a rebuild: the gate does not treat it as one either, so predicting one would over-estimate exactly as badly as the old text under-estimated. It only stops the preview claiming knowledge it does not have. """ - unmeasured = not new_hash or not stored_hash - rebuild_certain = _build_hash_mismatch(new_hash, stored_hash) or needs_flatten + # Whichever digest the upcoming build will actually be keyed on. + at_target = bool(needs_pull and target_hash) + measured = target_hash if at_target else new_hash + + unmeasured = not measured or not stored_hash + rebuild_certain = _build_hash_mismatch(measured, stored_hash) or needs_flatten if force: return ('forced clean rebuild (--force: image + cargo/npm volumes wiped)', rebuild_certain, unmeasured) - if _build_hash_mismatch(new_hash, stored_hash): - return ('will trigger fresh build (Dockerfile, requirements or box source changed)', + if _build_hash_mismatch(measured, stored_hash): + what = 'target Dockerfile' if at_target else 'Dockerfile' + return (f'will trigger fresh build ({what}, requirements or box source changed)', rebuild_certain, unmeasured) if needs_flatten: return ('will rebuild (flatten moves every source path)', rebuild_certain, unmeasured) - if is_rollback or commits_ahead > 0: + if needs_pull and not target_hash: + # A pull is coming and the target ref's build inputs could not be + # read, so the pre-pull tree matching the stored hash proves nothing + # about what the pull is going to land. + return ('unknown until pull (could not hash target build inputs)', + rebuild_certain, unmeasured) + if not at_target and (is_rollback or commits_ahead > 0): # Probe measured the *current* (pre-pull) Dockerfile/requirements, so # a backward jump can still trigger a rebuild we can't predict # without actually pulling. Be honest about the unknown. return ('unknown until pull (older ref may differ)', rebuild_certain, unmeasured) if unmeasured: - why = ('no successful build recorded yet' if new_hash + why = ('no successful build recorded yet' if measured else 'box source not where the probe looks') return (f'cache state unknown, auto-invalidation skipped ({why})', rebuild_certain, unmeasured) - return ('cache valid (no rebuild)', rebuild_certain, unmeasured) + return ('cache valid (target matches last build)' if at_target + else 'cache valid (no rebuild)', rebuild_certain, unmeasured) def _deployed_version_stale(tree_version, etc_version_raw): @@ -594,6 +736,7 @@ def __init__(self, total_steps): self.total_steps = total_steps self.current_step = 0 self.current_task = "" + self._base_task = "" self.start_time = time.time() self._stop_event = threading.Event() self._render_thread = None @@ -629,6 +772,7 @@ def _periodic_render(self): def update(self, task_name): """Advance to the next step and render.""" self.current_step += 1 + self._base_task = task_name self.current_task = task_name self._render() # Lazy-start the periodic thread on first step so we don't tick a @@ -636,6 +780,23 @@ def update(self, task_name): if self._tty: self._start_periodic_thread() + def set_detail(self, detail): + """Refresh the in-flight label with a live detail suffix. + + Used during long steps (Docker build) so the bar shows what is + actually running, e.g. ``Building container... [pip install ...]``. + Does not advance the step counter. No-op when ``detail`` is empty. + """ + detail = (detail or '').strip() + if not detail: + return + # Keep the suffix short so _layout can still fit the base label. + if len(detail) > 50: + detail = detail[:47] + '...' + with self._lock: + self.current_task = f'{self._base_task} [{detail}]' + self._render() + def _format_elapsed_time(self): """Format elapsed time as human-readable string.""" elapsed = int(time.time() - self.start_time) @@ -1409,11 +1570,19 @@ def _parse_fetch_result(result): current_version_raw = facts.get('ETC_VERSION', '').strip() current_box_version = current_version_raw.split('|', 1)[0] if current_version_raw else '(unknown)' - # --check does no git pull, so the probe's build-hash inputs still - # reflect the tree that would be built — no extra round-trip needed. - _check_new_hash = facts.get('BUILD_HASH_NEW', '') + # --check does no git pull. Hash the *target* ref's Dockerfile when a + # pull is pending so a forward jump (e.g. 0.31 → main) that changes + # the image recipe is not misreported as "~90s (cached)" from the + # pre-pull working tree matching /etc/lager/build-hash. + _check_working_hash = facts.get('BUILD_HASH_NEW', '') _check_stored_hash = facts.get('BUILD_HASH_STORED', '') - deps_will_change = _build_hash_mismatch(_check_new_hash, _check_stored_hash) + _check_target_hash = '' + if needs_pull and git_sync_confirmed: + _check_target_hash = _read_build_hash_at_ref( + run_ssh_command_with_output, git_ref, + ) + # The preview itself is rendered further down, once `needs_flatten` + # and the container/deploy facts are in hand -- see `_deps_preview`. container_down = facts.get('LAGER_RUNNING', '') == '0' @@ -1450,11 +1619,13 @@ def _parse_fetch_result(result): ) deps_status, deps_rebuild_certain, deps_unmeasured = _deps_preview( - _check_new_hash, _check_stored_hash, + _check_working_hash, _check_stored_hash, force=force, needs_flatten=needs_flatten, is_rollback=is_rollback, commits_ahead=commits_ahead or 0, + target_hash=_check_target_hash, + needs_pull=needs_pull, ) if force: @@ -1479,7 +1650,12 @@ def _parse_fetch_result(result): # in the Dockerfile cache; assume a real build. An unmeasured hash # lands here too — over-estimating a cached build costs the # operator nothing, while under-estimating a fresh one is how - # "~90s" preceded ten minutes of waiting. + # "~90s" preceded ten minutes of waiting. Deliberately kept as a + # floor even when a measured `target_hash` cleared + # `rebuild_certain`: the digest proves the build *inputs* match, + # not that Docker still holds the layers built from them — a + # `docker builder prune` between updates rebuilds everything with + # the digest unchanged. container_status = 'will restart' est = '~6 min (fresh build possible)' else: @@ -2264,37 +2440,28 @@ def _docker_supports_buildkit(): 'DOCKER_BUILDKIT=1 docker build -f docker/box.Dockerfile -t lager .']) build_output_lines = [] - if verbose: - # Stream output in verbose mode - process = subprocess.Popen( - ssh_cmd, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - text=True, - encoding='utf-8', - errors='replace', - bufsize=1 - ) - if process.stdout: - for line in process.stdout: + # Always read the build stream so non-verbose mode can still surface a + # live detail on the progress bar (and so failures still get the last + # 20 lines). Verbose additionally echoes each line to the terminal. + process = subprocess.Popen( + ssh_cmd, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + encoding='utf-8', + errors='replace', + bufsize=1, + ) + if process.stdout: + for line in process.stdout: + build_output_lines.append(line.rstrip()) + if verbose: click.echo(f' {line}', nl=False) - build_output_lines.append(line.rstrip()) - return_code = process.wait(timeout=600) - else: - # Silent mode - capture output for error reporting - process = subprocess.Popen( - ssh_cmd, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - text=True, - encoding='utf-8', - errors='replace', - ) - # Read and store output for potential error reporting - if process.stdout: - for line in process.stdout: - build_output_lines.append(line.rstrip()) - return_code = process.wait(timeout=600) + elif progress: + summary = _docker_build_line_summary(line) + if summary: + progress.set_detail(summary) + return_code = process.wait(timeout=600) if return_code != 0: if progress: diff --git a/cli/tests/test_update_gate.py b/cli/tests/test_update_gate.py index 3f3e0e76..1576181b 100644 --- a/cli/tests/test_update_gate.py +++ b/cli/tests/test_update_gate.py @@ -6,15 +6,20 @@ mismatch predicate, and the early-exit verdict (including container liveness). """ import os +import shutil import stat import subprocess import pytest from cli.commands.utility.update import ( + _build_hash_at_ref_shell_cmd, _build_hash_mismatch, + _build_hash_shell_cmd, _deployed_version_stale, + _docker_build_line_summary, _parse_probe_output, + _deps_preview, _probe_shell_script, _pull_shell_script, _rebuild_gate_verdict, @@ -191,6 +196,202 @@ def test_old_probe_output_leaves_host_keys_absent(self): assert 'HOST_CLI_VERSION' not in facts +class TestDepsPreviewAtTargetRef: + """`--check` must hash the *target* ref when a pull is pending. + + Field repro: a box ~120 commits behind whose pre-pull Dockerfile still + matched the stored hash. The preview read that as "cache valid / ~90s"; + the pull then changed the Dockerfile and the real update took ~6 min. + """ + + # Everything that is not about the target ref, held constant. + BASE = dict(force=False, needs_flatten=False, is_rollback=False, + commits_ahead=0) + + def test_forward_jump_target_mismatch_reports_fresh_build(self): + status, rebuild_certain, _ = _deps_preview( + SHA_A, SHA_A, # pre-pull tree still matches stored + target_hash=SHA_B, # ref about to be checked out differs + needs_pull=True, + **self.BASE, + ) + assert rebuild_certain is True + assert 'target Dockerfile' in status + + def test_forward_jump_target_match_reports_cache_valid(self): + status, rebuild_certain, _ = _deps_preview( + SHA_A, SHA_A, target_hash=SHA_A, needs_pull=True, **self.BASE, + ) + assert rebuild_certain is False + assert 'target matches' in status + + def test_unmeasurable_target_is_unknown_not_cache_valid(self): + status, _, _ = _deps_preview( + SHA_A, SHA_A, target_hash='', needs_pull=True, **self.BASE, + ) + assert 'unknown until pull' in status + assert 'cache valid' not in status + + def test_in_sync_uses_working_tree_hash(self): + status, rebuild_certain, _ = _deps_preview( + SHA_B, SHA_A, target_hash='', needs_pull=False, **self.BASE, + ) + assert rebuild_certain is True + assert 'Dockerfile, requirements or box source changed' in status + + def test_flatten_still_wins_over_a_matching_target_hash(self): + # Regression guard for the reconciliation of this change with the + # flatten fix. A box on the old `box/` subdir layout rebuilds + # whatever the target digest says, because the flatten moves every + # source path. Reading the target ref must not resurrect the "cache + # valid" claim that `needs_flatten` exists to prevent. + status, rebuild_certain, _ = _deps_preview( + SHA_A, SHA_A, + target_hash=SHA_A, + needs_pull=True, + force=False, needs_flatten=True, is_rollback=False, + commits_ahead=0, + ) + assert rebuild_certain is True + assert 'cache valid' not in status + assert 'flatten' in status + + +class TestBuildHashAtRefShellCmd: + def test_emits_git_show_for_dockerfile_blob(self): + script = _build_hash_at_ref_shell_cmd('origin/main') + assert 'git show origin/main:box/lager/docker/box.Dockerfile' in script + assert 'git cat-file -e origin/main:box/lager/docker/box.Dockerfile' in script + + def test_emits_source_tree_walk(self): + # Must match main's `_BUILD_HASH_SOURCE_DIRS` composition or --check + # spuriously reports a rebuild against stored hashes that include + # every file under ~/box/lager. + script = _build_hash_at_ref_shell_cmd('origin/main') + assert 'git ls-tree -r --name-only origin/main box/lager' in script + + def test_rejects_metacharacters(self): + assert _build_hash_at_ref_shell_cmd('main; rm -rf /') == 'echo ""' + assert _build_hash_at_ref_shell_cmd('') == 'echo ""' + + def test_uses_only_posix_shell_constructs(self): + # The remote login shell may be dash. Bash pattern substitution and + # `printf -v` both silently changed the digest under /bin/sh. + script = _build_hash_at_ref_shell_cmd('origin/main') + assert 'printf -v' not in script + assert '/#' not in script + + +@pytest.mark.skipif( + shutil.which('sha256sum') is None, + reason='needs GNU sha256sum (present on boxes and CI; macOS ships shasum)', +) +class TestBuildHashAtRefMatchesWorkingTree: + """Execute both hashers under ``sh`` against a fake box layout. + + This is the invariant #12 depends on: the target-ref digest must equal the + working-tree digest that `/etc/lager/build-hash` stores, or every --check + would report a spurious rebuild. Verified end-to-end (real git, real + sha256sum, dash-compatible) rather than by asserting on substrings. + """ + + DOCKERFILE_GIT_PATH = 'box/lager/docker/box.Dockerfile' + DOCKERFILE_TREE_PATH = 'lager/docker/box.Dockerfile' + SOURCE_GIT_PATH = 'box/lager/nets/net.py' + SOURCE_TREE_PATH = 'lager/nets/net.py' + + def _fake_box(self, tmp_path, dockerfile_body, source_body='print("ok")\n'): + """Build $HOME/box as the boxes have it: git tracks the `box/` prefix, + the working tree is the flattened layout. Includes a source file so + the `_BUILD_HASH_SOURCE_DIRS` walk is exercised (not just Dockerfile). + """ + home = tmp_path / 'home' + box = home / 'box' + for rel, body in ( + (self.DOCKERFILE_GIT_PATH, dockerfile_body), + (self.DOCKERFILE_TREE_PATH, dockerfile_body), + (self.SOURCE_GIT_PATH, source_body), + (self.SOURCE_TREE_PATH, source_body), + ): + path = box / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(body) + env = dict(os.environ, HOME=str(home)) + run = lambda *args: subprocess.run( + args, cwd=box, env=env, capture_output=True, text=True, timeout=30, + ) + run('git', 'init', '-q', '-b', 'main') + run('git', 'config', 'user.email', 'test@example.com') + run('git', 'config', 'user.name', 'test') + run('git', 'add', self.DOCKERFILE_GIT_PATH, self.SOURCE_GIT_PATH) + commit = run('git', 'commit', '-q', '-m', 'dockerfile') + assert commit.returncode == 0, commit.stderr + return home, env + + def _sh(self, snippet, env, cwd): + result = subprocess.run( + ['sh'], input=snippet, text=True, capture_output=True, + env=env, cwd=cwd, timeout=30, + ) + return result.stdout.strip() + + def test_ref_digest_equals_working_tree_digest(self, tmp_path): + home, env = self._fake_box(tmp_path, 'FROM python:3.12-slim\n') + working = self._sh(_build_hash_shell_cmd(), env, home / 'box') + at_ref = self._sh(_build_hash_at_ref_shell_cmd('HEAD'), env, home) + assert working, 'working-tree hasher produced nothing' + assert at_ref == working + + def test_ref_digest_differs_when_target_dockerfile_changes(self, tmp_path): + home, env = self._fake_box(tmp_path, 'FROM python:3.12-slim\n') + before = self._sh(_build_hash_at_ref_shell_cmd('HEAD'), env, home) + box = home / 'box' + (box / self.DOCKERFILE_GIT_PATH).write_text('FROM python:3.13-slim\n') + subprocess.run( + ['git', 'commit', '-qam', 'bump base'], cwd=box, env=env, + capture_output=True, text=True, timeout=30, + ) + after = self._sh(_build_hash_at_ref_shell_cmd('HEAD'), env, home) + # The forward-jump case: working tree still matches the stored hash + # while the ref about to be checked out does not. + working = self._sh(_build_hash_shell_cmd(), env, box) + assert after != before + assert after != working + + def test_ref_digest_differs_when_source_file_changes(self, tmp_path): + home, env = self._fake_box(tmp_path, 'FROM python:3.12-slim\n') + before = self._sh(_build_hash_at_ref_shell_cmd('HEAD'), env, home) + box = home / 'box' + (box / self.SOURCE_GIT_PATH).write_text('print("changed")\n') + subprocess.run( + ['git', 'commit', '-qam', 'touch source'], cwd=box, env=env, + capture_output=True, text=True, timeout=30, + ) + after = self._sh(_build_hash_at_ref_shell_cmd('HEAD'), env, home) + assert after != before + + def test_missing_ref_yields_empty_not_a_bogus_digest(self, tmp_path): + home, env = self._fake_box(tmp_path, 'FROM python:3.12-slim\n') + assert self._sh(_build_hash_at_ref_shell_cmd('no-such-ref'), env, home) == '' + + +class TestDockerBuildLineSummary: + def test_buildkit_run_line(self): + assert 'pip3 install' in ( + _docker_build_line_summary('#12 [8/20] RUN pip3 install cryptography') or '' + ) + + def test_buildkit_timed_setting_up(self): + assert 'Setting up' in ( + _docker_build_line_summary('#15 3.2 Setting up nodejs (20.x)') or '' + ) + + def test_ignores_cached_and_blank(self): + assert _docker_build_line_summary('#5 CACHED') is None + assert _docker_build_line_summary('') is None + assert _docker_build_line_summary(' ') is None + + class TestGateIgnoresHostCliFacts: def test_host_cli_mismatch_never_forces_rebuild(self): # A missing/stale host CLI reconciles on the fast path; it must never diff --git a/test/COVERAGE.md b/test/COVERAGE.md index 7e9734f4..51ab061a 100644 --- a/test/COVERAGE.md +++ b/test/COVERAGE.md @@ -36,13 +36,13 @@ are not. | Job (status context) | Path | Tests | |---|---|---:| -| `unit (cli)` | `test/unit/cli/` + `cli/tests/` | 1258 (+2 xfailed) | +| `unit (cli)` | `test/unit/cli/` + `cli/tests/` | 1274 (+2 xfailed) | | `unit (box)` | `test/unit/box/` | 1603 | | `unit (measurement)` | `test/unit/measurement/` | 105 | | `unit (blufi)` | `test/unit/blufi/` | 89 | | `unit (mcp)` | `test/mcp/unit/` | 166 | | `unit (root)` | `test/unit/test_*.py`, `test/test_*.py` | 121 (+1 skipped) | -| | **Total gated** | **3342** | +| | **Total gated** | **3358** | Each suite gets its own job because they need incompatible `sys.modules` states for the name `lager`: `test/unit/measurement/conftest.py` registers a placeholder whose `__init__` never runs