diff --git a/CHANGELOG.md b/CHANGELOG.md index 3919454c..ee62d1c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,46 @@ true until the next version shipped. regression cannot hide behind a correct answer. Each of the five one-token strategy mutations was proved to fail its corresponding assertion. +- The source fingerprint has one implementation, in Python, and both harnesses + call it. + + `test/lib.sh` and `test/pytest/pgc_cluster.py` each carried their own answer to + "what was this binary built from". In one day the pair produced four defects, + two in each copy, and not one was found by whoever wrote that copy: the Python + side never walked `objstore/*.c`, then mixed in each file's bare name so + `src/module.c` and `objstore/module.c` were interchangeable, then omitted each + build directory's `Makefile`; the shell side hashed `xargs -0 cat | md5sum`, a + stream with no per-file boundaries, so moving bytes between two files left the + hash unchanged while the source no longer compiled. The Python docstring + asserted parity with the shell throughout all four. It was false when written + and stayed false through two rounds of fixing. + + `test/pgc_fingerprint.py` is now the only implementation. It is Python rather + than shell because the portable half should be the one that survives: bash is + largely a GNU thing, while Python is present on FreeBSD and Windows where bash + is not. It uses the standard library only and runs on the system interpreter, + never the pytest virtualenv, so a freshness gate cannot depend on the test + dependencies of a harness it gates. + + It is also faster. The shell forked `md5sum` once per file; the module starts + one interpreter: + + shell, forking md5sum per file 239 ms per call + the module 26 ms per call + across 261 suites, twice each 124 s -> 13 s + + Unifying them closed a fifth defect that neither implementation had been + suspected of. `sort -z` orders by LOCALE COLLATION and nothing in the harness + pinned a locale, so one tree fingerprinted two ways depending on the machine: + + LC_ALL=C 6d122a7158d5 + LC_ALL=en_US.UTF-8 0b59bd75fa4f + + `en_US.UTF-8` is a common desktop default, so a developer could stamp a tree + and have CI read it back and call the binary stale. The module sorts bytes, + which is what `LC_ALL=C` produced and what every stamp already on disk was + written with, so no existing stamp is invalidated. + - The test harness refuses to measure a binary that was not built from the source under test. diff --git a/test/lib.sh b/test/lib.sh index 8b3b8349..3ea4d61f 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -671,106 +671,85 @@ pgc_freshness_report() { # pgc_freshness_report DIR -> the manifest, annotated echo " ($n files, under $dir)" } +# THE ONE IMPLEMENTATION LIVES IN test/pgc_fingerprint.py. +# +# This file and test/pytest/pgc_cluster.py each used to carry their own, and the +# pair produced four defects in one day -- two in each copy, and not one found by +# whoever wrote that copy (#907). The Python docstring asserted parity with this +# function throughout all four; it was false when written and stayed false through +# two rounds of fixing. +# +# PYTHON RATHER THAN SHELL, which is the opposite of what was first proposed here. +# The single implementation belongs in the more portable language: bash is largely +# a GNU thing, while Python is present on FreeBSD and Windows where bash is not +# (jd). This file already requires bash, so calling a more portable interpreter +# from it cannot cost portability. It is also not a cost at all -- the shell forked +# md5sum once per file, and one interpreter start beats 64 forks: +# +# shell, forking md5sum per file 239 ms/call +# the module 26 ms/call +# across 261 suites x 2 calls 124 s -> 13 s +# +# It fixed a defect on the way, which is the argument for one implementation in +# miniature: `sort -z` uses LOCALE COLLATION and no locale is pinned anywhere in +# this harness, so the same tree fingerprinted two ways depending on whose desktop +# it was -- +# +# LC_ALL=C 6d122a7158d5 +# LC_ALL=en_US.UTF-8 0b59bd75fa4f +# +# A developer on an en_US.UTF-8 default stamping a tree that CI then reads under +# C.UTF-8 is a FATAL naming a stale binary against a clean tree. The module sorts +# bytes, which is what LC_ALL=C did and what every stamp on disk was written with. +_pgc_fp_module() { + printf '%s\n' "$(dirname "${BASH_SOURCE[0]}")/pgc_fingerprint.py" +} + +# SYSTEM python3, NOT the pytest venv. test/pytest/README.md records that the +# interpreter is EXTERNALLY-MANAGED and that pytest runs from a virtualenv; a +# freshness gate that needed those test dependencies would make every suite in +# this directory unrunnable until somebody had installed pytest. The module +# imports nothing outside the standard library and nothing from test/pytest/. +_pgc_fp_python() { + command -v python3 2>/dev/null +} + +# A MISSING python3 IS REPORTED, NOT SWALLOWED. Returning empty alone would make +# the verdict `unknown` and print "freshness UNVERIFIED", which is deliberately +# not a failure -- and the whole gate would then be off with nothing saying so. +# Loud but not fatal: the asymmetry this controller is built on is that a false +# UNVERIFIED costs a line of output while a false FATAL costs a matrix. +_pgc_fp_warn_once() { + [ -n "${_pgc_fp_warned:-}" ] && return 0 + _pgc_fp_warned=1 + echo "-- python3 not found: the freshness check cannot run (see test/pgc_fingerprint.py)" >&2 +} + pgc_source_manifest() { # pgc_source_manifest DIR -> "relpath digest" per file, or EMPTY - local dir="${1:-.}" - local d - # THE MANIFEST IS THE THING; THE FINGERPRINT IS ITS HASH. - # - # Twelve hex characters cannot say which file moved. Two CI failures reported - # `source now a735c673b129, binary built from 6d122a7158d5` and nothing else, - # identically, across two branches and two majors -- so a bare hash gave us a - # second sample of a mystery rather than an answer. The controller's FATAL - # path prints this, and a diff of two manifests names the file. - # ONE TREE HASHES ONE WAY, HOWEVER THE PATH IS SPELLED. `${f#"$dir"/}` below - # strips a prefix that has to match character for character, so `$dir` with a - # trailing slash, or with a `/./` segment, or reached through a symlink, put - # the full ABSOLUTE path into the digest instead of the tree-relative one and - # the same tree hashed three different ways (@OffgridwithJD). Canonicalised - # ONCE here rather than defended at each call site, so a future caller cannot - # reintroduce it: the writer and the reader reach the tree by different routes - # and a disagreement between them is a FATAL about nothing. - # - # pgc_norm_path rather than a second `cd && pwd -P` of my own. This file has - # spent the day proving that two implementations of one idea drift, and a - # private copy here would be the third normaliser in one tree. Its fallback - # hands back the raw path for a directory that cannot be entered, which needs - # no special case: nothing is found under it, `out` is empty, and the guard - # below returns no fingerprint. - dir="$(pgc_norm_path "$dir")" - { - while IFS= read -r d; do - [ -n "$d" ] || continue - find "$d" -maxdepth 1 -type f \( -name '*.c' -o -name '*.h' \ - -o -name 'Makefile' \) -print0 2>/dev/null - done < <(pgc_source_build_dirs "$dir") - find "$dir" -maxdepth 1 -type f \( -name 'Makefile' -o -name '*.control' \ - -o -name '*.sql' \) -print0 2>/dev/null - } | sort -z | while IFS= read -r -d '' _pgc_fp_f; do - # EACH FILE'S PATH AND ITS OWN DIGEST, not the concatenated stream. - # - # `xargs -0 cat | md5sum` hashed the bytes of every file run together, - # so it could not see a change that PRESERVES the stream while moving - # bytes between translation units. Two files, `static int x=1;` and - # `static int x=2;`, both compile; move the second into the first and - # empty it and the source no longer compiles, while the hash does not - # move (@linuxhikerpm, #898 review): - # - # before_hash=bfce474cc159 after_hash=bfce474cc159 - # initial_compile=0 repartitioned_compile=1 - # error: redefinition of 'x' - # - # A skip-build run then printed "matches the binary under test" for - # source that cannot produce any binary at all. The path makes the - # partition part of the input, and the per-file digest is an - # unambiguous boundary between one file's bytes and the next's. - # - # A DIGEST THAT FAILED MUST NOT LOOK LIKE ONE THAT SUCCEEDED. This was - # `$(md5sum < "$f" 2>/dev/null | cut -d' ' -f1)` inline, so a failed - # md5sum -- a fork that hits EAGAIN, an OOM kill, a loaded runner -- - # contributed an EMPTY digest and the function returned a confident - # WRONG hash with status 0. One stubbed failure among many, on one - # unchanged tree, gave three different answers: - # - # baseline c8e6b23db1c9 - # one digest empty (call 2) 22897add806e - # one digest empty (call 3) 58c76fdab962 - # - # On the READ side that costs one suite at random, which is what #902's - # PG18 leg showed. On the WRITE side it is worse and deterministic: a - # failed digest while the controller stamps bakes a wrong hash, and every - # suite in the batch then reports `stale` -- a FATAL naming a stale binary - # -- against a tree that is perfectly clean (@OffgridwithJD, measured: - # 5 of 5 suites stale on a correct tree). - _pgc_fp_h="$(md5sum < "$_pgc_fp_f" 2>/dev/null | cut -d' ' -f1)" - [ -n "$_pgc_fp_h" ] || { printf '%s\n' "$_pgc_fp_failed"; break; } - printf '%s %s\n' "${_pgc_fp_f#"$dir"/}" "$_pgc_fp_h" - done + local py out rc + py="$(_pgc_fp_python)" || true + [ -n "$py" ] || { _pgc_fp_warn_once; printf '%s\n' "$_pgc_fp_failed"; return 0; } + out="$("$py" "$(_pgc_fp_module)" manifest "${1:-.}" 2>/dev/null)" + rc=$? + # A DIGEST THAT FAILED MUST NOT LOOK LIKE ONE THAT SUCCEEDED. The module exits + # 1 and prints nothing when a file could not be read, so the marker is emitted + # here and pgc_source_fingerprint below turns it into no fingerprint at all. + # Before this was true anywhere, a failed md5sum contributed an EMPTY digest + # and the function returned a confident WRONG hash at status 0, which cost a + # matrix (@OffgridwithJD, measured: 5 of 5 suites stale on a correct tree). + [ "$rc" -eq 0 ] || { printf '%s\n' "$_pgc_fp_failed"; return 0; } + [ -n "$out" ] || return 0 + printf '%s\n' "$out" } pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash, or EMPTY if it could not be computed - local out - out="$(pgc_source_manifest "${1:-.}")" - # Empty rather than a hash, which pgc_freshness_verdict turns into `unknown` - # and the controller prints as "freshness UNVERIFIED" -- already designed, and - # already deliberately not a failure. The asymmetry is the whole argument: a - # false UNVERIFIED costs a line of output, a false FATAL costs a matrix AND - # teaches people to re-run past a freshness check, which is the failure this - # controller exists to prevent. - case "$out" in *"$_pgc_fp_failed"*) printf ''; return 0 ;; esac - # A tree with no hashable file cannot be verified, so it gets no fingerprint - # either -- and md5 of an empty stream is a stable, comparable value that - # would have made two empty trees "match". Observed rather than hypothetical: - # under process pressure the whole manifest came back empty and the old code - # returned md5("") = d41d8cd98f00 in 11 runs of 20 (@OffgridwithJD). - [ -n "$out" ] || { printf ''; return 0; } - # THE TRAILING NEWLINE IS LOAD-BEARING. The old code piped the loop straight - # into md5sum, so the stream md5sum saw ended with one; `$(...)` strips it. - # Without `printf '%s\n'` here the same unchanged tree hashes differently - # before and after this change, every stamp already on disk reads `stale`, and - # a fix for false FATALs becomes a false FATAL for everyone holding a built - # worktree (@OffgridwithJD, caught before it shipped). The matrix could not - # have caught it: it copies a fresh tree and re-stamps every run. - printf '%s\n' "$out" | md5sum | cut -c1-12 + local py out rc + py="$(_pgc_fp_python)" || true + [ -n "$py" ] || { _pgc_fp_warn_once; printf ''; return 0; } + out="$("$py" "$(_pgc_fp_module)" fingerprint "${1:-.}" 2>/dev/null)" + rc=$? + [ "$rc" -eq 0 ] || { printf ''; return 0; } + printf '%s\n' "$out" } # fresh the binary was built from this source diff --git a/test/pgc_fingerprint.py b/test/pgc_fingerprint.py new file mode 100755 index 00000000..d170ca86 --- /dev/null +++ b/test/pgc_fingerprint.py @@ -0,0 +1,196 @@ +#!/usr/bin/env python3 +"""What a build reads, as a manifest, and the fingerprint that is its hash. + +THE ONE IMPLEMENTATION. `test/lib.sh` and `test/pytest/pgc_cluster.py` each used +to carry their own, and on 2026-09-09 the pair produced four defects in a day -- +two in each copy, and not one found by whoever wrote that copy (#907): + + objstore/*.c never walked python found by @linuxhikerpm + the bare NAME instead of the path python found while fixing the above + `xargs -0 cat | md5sum`, no bounds shell found by @linuxhikerpm + each build dir's Makefile omitted python found while writing the twin + +The Python docstring asserted "the same input set as pgc_source_fingerprint in +test/lib.sh" throughout all four. It was false when written and stayed false +through two rounds of fixing. A prose claim of agreement is not a mechanism, and +it is worse than saying nothing because it is what stops the next person checking. + +WHY PYTHON RATHER THAN SHELL, which is the opposite of what was first proposed. +The single implementation goes in the more portable language, not the less: bash +is largely a GNU thing, while Python is present on FreeBSD and Windows where bash +is not (jd). `test/lib.sh` already requires bash, so calling a more portable +interpreter from it cannot cost portability -- and it is not a cost at all: + + shell, forking md5sum once per file 239 ms/call + this module, one interpreter start 26 ms/call + +STDLIB ONLY, AND IT MUST RUN ON THE SYSTEM INTERPRETER. test/pytest/README.md +records that the interpreter is EXTERNALLY-MANAGED and that pytest runs from a +venv. lib.sh must never need that venv: a freshness gate that depended on the +pytest test dependencies would make every bash suite unrunnable until somebody +had installed pytest. Nothing here imports outside the standard library, and +nothing here imports from test/pytest/. +""" + +import hashlib +import os +import pathlib +import sys + +__all__ = ["norm_path", "build_dirs", "manifest", "fingerprint"] + +SOURCE_SUFFIXES = (".c", ".h") +ROOT_SUFFIXES = (".control", ".sql") + + +def norm_path(d): + """The tree's physical path, or the argument unchanged if it cannot be entered. + + One tree hashes one way however the path is spelled. The relative path below + is produced by stripping a prefix, so `$dir` with a trailing slash, with a + `/./` segment, or reached through a symlink used to put the full ABSOLUTE + path into the manifest and the same tree hashed four different ways. The + fallback matches the shell's `pgc_norm_path`: hand back the raw path, find + nothing under it, and let the caller return no fingerprint. + """ + try: + return pathlib.Path(os.path.realpath(str(d))) + except OSError: + return pathlib.Path(str(d)) + + +def _is_plain_file(p): + """A regular file, NOT a symlink to one. + + `find -type f` tests the link itself, so a symlinked source is not in the + shell's manifest. `pathlib.is_file()` FOLLOWS the link and would have added + one, which is a silent divergence of exactly the kind this module exists to + end -- and it would have appeared only on trees that use symlinks, which is + to say in somebody else's checkout rather than in CI. + """ + try: + return p.is_file() and not p.is_symlink() + except OSError: + return False + + +def build_dirs(root): + """Every directory the build compiles in: src, plus each recursed module. + + src unconditionally, whether or not it exists -- the shell prints it before + looking, and a missing src must not change the RESULT, only what is found in + it. Then any depth-2 Makefile's directory, which is how the top-level + Makefile reaches a separately built module such as objstore/. + """ + root = pathlib.Path(root) + out = [root / "src"] + try: + entries = sorted(root.iterdir(), key=lambda p: os.fsencode(str(p))) + except OSError: + return out + for d in entries: + try: + if not d.is_dir() or d.is_symlink(): + continue # find does not descend a symlinked directory + except OSError: + continue + if d == root / "src": + continue + if _is_plain_file(d / "Makefile"): + out.append(d) + return out + + +def _manifest_files(root): + paths = [] + for d in build_dirs(root): + try: + entries = list(d.iterdir()) + except OSError: + continue # find prints nothing for an unreadable directory + for f in entries: + if _is_plain_file(f) and (f.suffix in SOURCE_SUFFIXES or f.name == "Makefile"): + paths.append(f) + try: + entries = list(root.iterdir()) + except OSError: + entries = [] + for f in entries: + if _is_plain_file(f) and (f.name == "Makefile" or f.suffix in ROOT_SUFFIXES): + paths.append(f) + # Sorted as BYTES over the absolute path, which is what `sort -z` does under + # LC_ALL=C and what every stamp on disk was written with. It is NOT what + # `sort -z` does under a locale: no locale is pinned anywhere in the harness, + # and en_US.UTF-8 collation reorders `columnar_arrow.c` against + # `columnar-arrow.c`, so the same tree fingerprinted two ways depending on + # the developer's environment -- + # + # LC_ALL=C 6d122a7158d5 + # LC_ALL=en_US.UTF-8 0b59bd75fa4f + # + # A desktop default of en_US.UTF-8 stamping a tree that CI then reads under + # C.UTF-8 is a FATAL naming a stale binary against a clean tree. Sorting + # bytes here removes the environment from the answer. + return sorted(paths, key=lambda p: os.fsencode(str(p))) + + +def manifest(root): + """-> "relpath digest" per file, newline-joined, or None if one could not be read. + + None rather than a short manifest. A digest that FAILED must not look like + one that succeeded: the shell version contributed an empty digest for a failed + md5sum and returned a confident wrong hash at status 0, which cost a matrix. + """ + root = norm_path(root) + lines = [] + for f in _manifest_files(root): + try: + digest = hashlib.md5(f.read_bytes()).hexdigest() + except OSError: + return None + try: + rel = f.relative_to(root) + except ValueError: + return None + lines.append(f"{rel} {digest}") + return "\n".join(lines) + + +def fingerprint(root): + """-> 12 hex characters, or "" if it could not be computed. + + Empty rather than a hash, which pgc_freshness_verdict turns into `unknown` + and the controller prints as "freshness UNVERIFIED" -- deliberately not a + failure. A false UNVERIFIED costs a line of output; a false FATAL costs a + matrix and teaches people to re-run past a freshness check. + """ + out = manifest(root) + if out is None or out == "": + return "" + # THE TRAILING NEWLINE IS LOAD-BEARING. The shell piped its loop straight + # into md5sum, so the stream md5sum saw ended with one. Drop it and the same + # unchanged tree hashes differently, every stamp already on disk reads + # `stale`, and a portability fix becomes a false FATAL for everyone holding a + # built worktree. + return hashlib.md5((out + "\n").encode()).hexdigest()[:12] + + +def main(argv): + if len(argv) != 3 or argv[1] not in ("manifest", "fingerprint"): + print("usage: pgc_fingerprint.py {manifest|fingerprint} DIR", file=sys.stderr) + return 2 + if argv[1] == "manifest": + out = manifest(argv[2]) + # An unreadable digest prints nothing and exits 1, so a caller cannot + # mistake "could not be computed" for "the tree holds nothing". + if out is None: + return 1 + if out: + print(out) + return 0 + print(fingerprint(argv[2])) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index a170205a..68a5fea9 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -4,7 +4,7 @@ Reference for anyone reading, running, or adding to `test/pytest/`. The design a the decisions behind the harness are in `design/ISSUE_432_PYTEST_HARNESS.md`. This file covers the tests themselves. -**122 tests in 10 files.** One hundred and seven of them test the harness rather than the +**123 tests in 10 files.** One hundred and eight of them test the harness rather than the product, and they come first, because a harness that can report a false green makes every other result in this directory worthless. @@ -486,13 +486,27 @@ itself. The subject is the instrument every other arm in this section depends on if the fingerprint can be wrong, `never report on source you did not build` reports on nothing. -`test_a_failed_digest_yields_no_fingerprint_rather_than_a_wrong_one` drives the -real shell function with a **stub `md5sum` that is the real one except on its Nth -call**, where it fails with no output — a fork that hits `EAGAIN`, an OOM kill, a -loaded runner. The old code substituted that empty digest into the hash and -returned status 0, so one unchanged tree produced three different confident -answers. The premise arm requires the stub to agree with the real `md5sum` when -nothing is configured to fail, or the test would be measuring the stub. +`test_a_failed_digest_yields_no_fingerprint_rather_than_a_wrong_one` and +`test_a_failed_digest_gives_unverified_and_never_a_false_stale` are the two arms +here, and THE MECHANISM CHANGED WITH THE IMPLEMENTATION. They used to drive the real +shell function with a **stub `md5sum`** on `PATH`, because the shell forked one +per file. The digest now lives in `test/pgc_fingerprint.py` and uses `hashlib`, +which no `PATH` can reach, so the stub would have left both arms green while +testing nothing — the exact shape this corpus exists to refuse. + +A real read failure needs a real reader who is denied, and **root is denied +nothing**: `chmod 000` is invisible to it. Measured before the arms were +rewritten: + + as root 28a7149e07ae <- reads the mode-000 file regardless + as postgres (empty) <- the failure the arm needs + +So the tree is built outside any mode-0700 directory and read by a second user, +and where no such user exists the arm records `expect.cannot_run` rather than +passing. `test_one_tree_hashes_one_way_however_the_locale_is_set` pins the defect +the single implementation removed on the way: `sort -z` used locale collation and +nothing pinned a locale, so one tree hashed two ways — +`LC_ALL=C` gave `6d122a7158d5` and `LC_ALL=en_US.UTF-8` gave `0b59bd75fa4f`. `test_a_failed_digest_gives_unverified_and_never_a_false_stale` is the property that matters. `stale` is the FATAL; `unknown` prints `freshness UNVERIFIED` and @@ -969,3 +983,6 @@ complete. This is why the property comparison reads names. enclosing command line only while the plain word appears nowhere else in it. A probe whose body also contained `/tmp/pgc-pytest-*` counted its own invocation as a leaked process. Walking `/proc//cmdline` is the reliable instrument. + +`test_one_tree_hashes_one_way_however_the_locale_is_set` requires one tree to +give one fingerprint across every installed locale. diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index 43fce107..0dc126e8 100644 --- a/test/pytest/pgc_cluster.py +++ b/test/pytest/pgc_cluster.py @@ -18,6 +18,7 @@ """ import fcntl +import importlib.util import hashlib import os import re @@ -367,79 +368,51 @@ def build_and_install(srcdir, pg_config, major, runner=None): ) +# BOTH OF THESE NOW DELEGATE TO test/pgc_fingerprint.py. +# +# They used to be an independent implementation of what test/lib.sh does, and the +# pair produced four defects in one day -- two here, two there, and not one found +# by whoever wrote that copy (#907): +# +# objstore/*.c never walked here found by @linuxhikerpm +# the bare NAME instead of the path here found while fixing the above +# `xargs -0 cat | md5sum`, no bounds lib.sh found by @linuxhikerpm +# each build dir's Makefile omitted here found while writing the twin +# +# The docstring here asserted "the same input set as pgc_source_fingerprint in +# test/lib.sh" through all four. It was false when written and stayed false +# through two rounds of fixing, which is the case against a prose claim of +# agreement: it is not a mechanism, and it is worse than silence because it is +# exactly what stops the next person checking. +# +# Loaded BY PATH rather than by package import: test/ is not a package, and this +# module is imported by pytest from test/pytest/ while lib.sh runs it as a script +# from test/. Neither should have to know about the other's layout. +_FP_PATH = pathlib.Path(__file__).resolve().parent.parent / "pgc_fingerprint.py" +_fp_spec = importlib.util.spec_from_file_location("pgc_fingerprint", _FP_PATH) +_fp = importlib.util.module_from_spec(_fp_spec) +_fp_spec.loader.exec_module(_fp) + + def source_build_dirs(srcdir): - """Every directory the build compiles in: src/, plus any directory that - carries its own Makefile. + """Every directory the build compiles in: src/, plus any with its own Makefile. - DERIVED, NOT LISTED, and the same rule pgc_source_build_dirs uses in - test/lib.sh. Naming objstore/ here would fix today and fail the next time a - module is added; a directory with its own Makefile is what the top-level + DERIVED, NOT LISTED. Naming objstore/ would fix today and fail the next time a + module is added; a directory carrying its own Makefile is what the top-level Makefile recurses into, so that is the property to read. """ - srcdir = pathlib.Path(srcdir) - dirs = [srcdir / "src"] - for makefile in sorted(srcdir.glob("*/Makefile")): - if makefile.parent != srcdir / "src": - dirs.append(makefile.parent) - return dirs + return _fp.build_dirs(srcdir) def source_fingerprint(srcdir): - """A hash of everything a build reads, or None if the tree is unreadable. - - The same input set as pgc_source_fingerprint in test/lib.sh: the C sources - and headers of EVERY directory the build compiles in, the Makefile, the - control file and the SQL scripts. Content, not mtime, because a checkout or - a branch switch rewrites mtimes without changing what compiles, and - `git stash` does the reverse. - - THE FIRST VERSION READ src/ ONLY, and said in this docstring that it matched - lib.sh while it did not. objstore/ is a separately built shared library the - top-level Makefile reaches by recursion, so editing - objstore/columnar_objstore_module.c left the hash unchanged and build_once - certified a stale module as current (@linuxhikerpm, #897 review): - - objstore_before=2799803eaeac objstore_after=2799803eaeac - builds=1 second=already-built - - That is the same gap #898 closes in test/lib.sh. This is an INDEPENDENT - implementation, so rebasing #898 would not have fixed it -- which is the - argument for the two eventually becoming one, not two that agree today. + """A hash of everything a build reads, or None if it could not be computed. + + None rather than "" because that is the contract this harness already had, and + build_once distinguishes "no fingerprint" from a real one. The module returns + "" for the same condition; the mapping happens here rather than there so the + shell and Python callers each get the shape they already expect. """ - srcdir = pathlib.Path(srcdir) - paths = [] - for d in source_build_dirs(srcdir): - # EACH BUILD DIRECTORY'S Makefile TOO, not only its sources. The shell - # implementation hashes `*.c`, `*.h` AND `Makefile` per directory; this - # read only the sources, so editing `objstore/Makefile` -- which changes - # how that module is built -- moved the shell hash and not this one: - # - # baseline shell=45be41a5c47b python=bea88c7d79ca - # objstore/Makefile edited shell=cfb8f4553041 python=bea88c7d79ca - # - # `build_once` then certified a stale module as current. That is - # @linuxhikerpm's #897 finding one layer over: they found the module's - # sources missing here, and the module's Makefile was still missing - # after that was fixed. The docstring claimed parity throughout. - paths += list(d.glob("*.c")) + list(d.glob("*.h")) + list(d.glob("Makefile")) - paths = sorted( - paths - + [p for p in (srcdir / "Makefile",) if p.exists()] - + sorted(srcdir.glob("*.control")) + sorted(srcdir.glob("*.sql")) - ) - if not paths: - return None - h = hashlib.md5() - for path in paths: - try: - # The path relative to the tree, not just the name: with two build - # directories, src/module.c and objstore/module.c are different - # inputs and a bare name would make them interchangeable. - h.update(str(path.relative_to(srcdir)).encode()) - h.update(path.read_bytes()) - except (OSError, ValueError): - return None - return h.hexdigest()[:12] + return _fp.fingerprint(srcdir) or None def build_once(srcdir, pg_config, major, lock_path=None, runner=None): diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index 813c6137..ddd16a4e 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -21,6 +21,9 @@ """ import os +import tempfile +import shutil +import pwd import pathlib import re import subprocess @@ -442,10 +445,12 @@ def test_the_two_fingerprint_implementations_cover_the_same_inputs(tmp_path, exp missing from the Python side, and the Makefiles were still missing after it was fixed. - The two hashes are NOT required to be equal -- they are different digests - over the same files, used independently. What is required is that the same - edit moves both, which is what "the same input set" means and all the - docstring ever claimed. + THEY ARE NOW ONE IMPLEMENTATION (#907), so the requirement has strengthened + from "the same edit moves both" to "both are the same value". That is worth + asserting rather than merely allowing: equality is what makes the shell's + stamp readable by the Python harness and back, and if someone reintroduces a + private copy on either side this arm reddens on the first edit rather than on + the first edit that happens to diverge. """ t = _tree_with_module(tmp_path, "cover") for edit, path, body in ( @@ -464,6 +469,8 @@ def test_the_two_fingerprint_implementations_cover_the_same_inputs(tmp_path, exp path.write_text(old) expect.text(f"{sh_after != sh_before} {py_after != py_before}", "True True", f"editing {edit} moves both fingerprints") + expect.text(f"{sh_before} {sh_after}", f"{py_before} {py_after}", + f"and one implementation gives one value, editing {edit}") # --------------------------------------------------------------------------- @@ -500,66 +507,126 @@ def _fp_tree(tmp_path, name="t"): return t -def _md5_stub(tmp_path, fail_on): - """A stub md5sum: real, except that its Nth call fails with no output. +def _unprivileged_user(): + """A user that is not root, or None. - Models one transient failure -- a fork that hits EAGAIN, an OOM kill, a - loaded runner -- rather than a permanently broken md5sum, because the - permanent case is not the one that produced a wrong answer in CI. + THE MECHANISM HAD TO CHANGE WHEN THE IMPLEMENTATION DID. These arms used to + stub `md5sum` on PATH, because the shell forked it once per file. The digest + is now hashlib inside test/pgc_fingerprint.py, which no PATH can reach, so a + stub would have left both arms passing while testing nothing -- the exact + shape this suite exists to refuse. + + A real read failure needs a real reader who is denied, and root is denied + nothing: chmod 000 is invisible to it. Measured, before this was written: + + as root : 28a7149e07ae <- reads the mode-000 file anyway + as postgres : '' <- the failure the arm needs """ - b = tmp_path / f"bin{fail_on}" - b.mkdir(exist_ok=True) - counter = tmp_path / f"count{fail_on}" - counter.write_text("0") - (b / "md5sum").write_text( - "#!/bin/bash\n" - f'_n=$(( $(cat "{counter}" 2>/dev/null || echo 0) + 1 ))\n' - f'echo "$_n" > "{counter}"\n' - f'[ "$_n" = "{fail_on}" ] && exit 1\n' - 'exec /usr/bin/md5sum "$@"\n' - ) - (b / "md5sum").chmod(0o755) - return str(b) + if os.geteuid() != 0: + return "" # already unprivileged; read in this process + for name in ("postgres", "nobody"): + try: + pwd.getpwnam(name) + return name + except KeyError: + continue + return None + + +def _sh_fp_as(user, expr): + """Evaluate a lib.sh expression as USER ("" means this process).""" + script = f'. "{SRCDIR}/test/lib.sh" || exit 1; {expr}' + argv = ["bash", "-c", script] if not user else \ + ["runuser", "-u", user, "--", "bash", "-c", script] + p = subprocess.run(argv, capture_output=True, text=True) + return p.stdout.strip(), p.returncode -def test_a_failed_digest_yields_no_fingerprint_rather_than_a_wrong_one(tmp_path, expect): - """`$(md5sum ... 2>/dev/null)` substituted an EMPTY digest and returned rc=0. +def _readable_tree(name): + """A fingerprintable tree an unprivileged user can traverse. - One failed invocation among many silently changed the whole hash, so the - function gave three different confident answers for one unchanged tree. + Not under tmp_path: pytest's directories are mode 0700 and owned by the user + running the session, so a second user cannot walk into them and every arm + below would fail for the wrong reason. """ - t = _fp_tree(tmp_path) - base, rc = _sh_fp(f'pgc_source_fingerprint "{t}"') - expect.at_least(len(base), 12, "premise: the tree fingerprints at all") - - # Premise for the stub: with nothing configured to fail it must agree with - # the real md5sum, or the arms below measure the stub and not the fix. - quiet = _md5_stub(tmp_path, 0) - got, _ = _sh_fp(f'pgc_source_fingerprint "{t}"', path_env=quiet) - expect.text(got, base, "premise: the stub agrees with md5sum when nothing fails") + root = pathlib.Path(tempfile.mkdtemp(prefix="pgc-fpfail-")) + t = root / name + (t / "src").mkdir(parents=True) + for n, body in (("a.c", "int a;\n"), ("b.c", "int b;\n"), ("c.c", "int c;\n")): + (t / "src" / n).write_text(body) + (t / "Makefile").write_text("all:\n\ttrue\n") + (t / "pgcolumnar.control").write_text("x\n") + for d in (root, t, t / "src"): + d.chmod(0o755) + for f in t.rglob("*"): + if f.is_file(): + f.chmod(0o644) + return root, t - for n in (2, 3): - stub = _md5_stub(tmp_path, n) - got, _ = _sh_fp(f'pgc_source_fingerprint "{t}"', path_env=stub) - expect.text(got or "empty", "empty", - f"a failed digest on file {n} yields no fingerprint") +def test_a_failed_digest_yields_no_fingerprint_rather_than_a_wrong_one(expect): + """A digest that FAILED must not look like one that succeeded. -def test_a_failed_digest_gives_unverified_and_never_a_false_stale(tmp_path, expect): + The shell substituted an EMPTY digest for a failed md5sum and returned rc=0, + so one failed read among many silently changed the whole hash and the + function gave three different confident answers for one unchanged tree. + """ + user = _unprivileged_user() + if user is None: + expect.cannot_run("MISSING_DEPENDENCY", + "no non-root user to read as; root ignores chmod 000") + return + root, t = _readable_tree("t") + try: + base, _ = _sh_fp_as(user, f'pgc_source_fingerprint "{t}"') + expect.at_least(len(base), 12, "premise: the tree fingerprints at all") + + # Premise for the mechanism itself: the unprivileged reader must agree + # with a privileged one while nothing is denied, or the arm below would + # be measuring the user switch rather than the failure. + mine, _ = _sh_fp_as("", f'pgc_source_fingerprint "{t}"') + expect.text(base, mine, "premise: the unprivileged read agrees while readable") + + for name in ("b.c", "c.c"): + f = t / "src" / name + f.chmod(0o000) + got, _ = _sh_fp_as(user, f'pgc_source_fingerprint "{t}"') + f.chmod(0o644) + expect.text(got or "empty", "empty", + f"an unreadable {name} yields no fingerprint, not a wrong one") + + after, _ = _sh_fp_as(user, f'pgc_source_fingerprint "{t}"') + expect.text(after, base, "control: and the tree fingerprints again once readable") + finally: + shutil.rmtree(root, ignore_errors=True) + + +def test_a_failed_digest_gives_unverified_and_never_a_false_stale(expect): """The property that matters. `stale` is the FATAL; `unknown` is UNVERIFIED. A false UNVERIFIED costs a line of output. A false FATAL costs a matrix and - teaches people to re-run past a freshness check. + teaches people to re-run past a freshness check, which is the failure this + controller exists to prevent. """ - t = _fp_tree(tmp_path, "v") - base, _ = _sh_fp(f'pgc_source_fingerprint "{t}"') - stub = _md5_stub(tmp_path, 2) - verdict, _ = _sh_fp( - f'pgc_freshness_verdict "{base}" "$(pgc_source_fingerprint "{t}")"', - path_env=stub) - expect.text(verdict, "unknown", "a failed digest reads as unknown, not stale") - healthy, _ = _sh_fp(f'pgc_freshness_verdict "{base}" "$(pgc_source_fingerprint "{t}")"') - expect.text(healthy, "fresh", "control: an unstubbed run still reads fresh") + user = _unprivileged_user() + if user is None: + expect.cannot_run("MISSING_DEPENDENCY", + "no non-root user to read as; root ignores chmod 000") + return + root, t = _readable_tree("v") + try: + base, _ = _sh_fp_as(user, f'pgc_source_fingerprint "{t}"') + expect.at_least(len(base), 12, "premise: the tree fingerprints at all") + (t / "src" / "b.c").chmod(0o000) + verdict, _ = _sh_fp_as( + user, f'pgc_freshness_verdict "{base}" "$(pgc_source_fingerprint "{t}")"') + expect.text(verdict, "unknown", "a failed digest reads as unknown, not stale") + (t / "src" / "b.c").chmod(0o644) + healthy, _ = _sh_fp_as( + user, f'pgc_freshness_verdict "{base}" "$(pgc_source_fingerprint "{t}")"') + expect.text(healthy, "fresh", "control: a readable run still reads fresh") + finally: + shutil.rmtree(root, ignore_errors=True) def test_one_tree_hashes_one_way_however_the_path_is_spelled(tmp_path, expect): @@ -797,3 +864,54 @@ def offenders_in(text): "premise: and the INDIRECT form, which is the defect's own shape") expect.num(len(offenders_in('_p="$_bd_copy/objstore/p.c"\nprintf x > "$_p"\n')), 0, "control: a write into a COPY is not an offender") + + +def test_one_tree_hashes_one_way_however_the_locale_is_set(expect): + """The defect the single implementation removed on the way. + + The shell sorted its manifest with `sort -z`, which uses LOCALE COLLATION, + and no locale is pinned anywhere in this harness. So the same tree + fingerprinted differently depending on whose machine it was: + + LC_ALL=C 6d122a7158d5 + LC_ALL=en_US.UTF-8 0b59bd75fa4f + + en_US.UTF-8 is a common desktop default. A developer stamping a tree there + and a CI runner reading it under C.UTF-8 disagree, and the disagreement is a + FATAL naming a stale binary against a tree that is perfectly clean -- the + exact false FATAL this controller exists to prevent, arriving from the + environment rather than from the source. + + The filenames matter: `_` and `-` are what the two collations order + differently, and `columnar_arrow.c` beside `columnar-arrow.c` is not a + contrived pair in this tree. + """ + root = pathlib.Path(tempfile.mkdtemp(prefix="pgc-locale-")) + try: + t = root / "t" + (t / "src").mkdir(parents=True) + for n in ("columnar_arrow.c", "columnar-arrow.c", "columnarXarrow.c", + "Columnar.c", "columnar.c"): + (t / "src" / n).write_text(f"int x; /* {n} */\n") + (t / "Makefile").write_text("all:\n\ttrue\n") + (t / "pgcolumnar.control").write_text("x\n") + + available = subprocess.run(["locale", "-a"], capture_output=True, + text=True).stdout.lower() + wanted = [l for l in ("c", "c.utf8", "en_us.utf8") if l in available] + if len(wanted) < 2: + expect.cannot_run("MISSING_DEPENDENCY", + f"fewer than two locales installed: {wanted}") + return + + seen = {} + for loc in wanted: + got, _ = _sh_fp(f'pgc_source_fingerprint "{t}"', + env={"LC_ALL": loc, "LANG": loc}) + seen[loc] = got + expect.at_least(len(seen[wanted[0]]), 12, + "premise: the tree fingerprints at all") + expect.num(len(set(seen.values())), 1, + f"one tree, one fingerprint, across {len(wanted)} locales: {seen}") + finally: + shutil.rmtree(root, ignore_errors=True) diff --git a/test/selftest/340-the-binary-must-be-built-from.sh b/test/selftest/340-the-binary-must-be-built-from.sh index b272dc49..18ed61c2 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -512,55 +512,86 @@ unset _wr _wr_rc _wr_written _wr_expected _wr_f # deliberately does not fail. A false UNVERIFIED costs a line of output. A false # FATAL costs a red matrix and teaches people to re-run past the check. +# THE MECHANISM HAD TO CHANGE WHEN THE IMPLEMENTATION DID (#907). +# +# This block used to stub `md5sum` on PATH, because the shell forked it once per +# file. The digest is now hashlib inside test/pgc_fingerprint.py, which no PATH +# can reach -- so the stub would have left every arm below GREEN while testing +# nothing at all, which is the precise failure this file exists to refuse. +# +# A real read failure needs a real reader who is denied, and root is denied +# nothing: chmod 000 is invisible to it. Measured before this was written: +# +# as root 28a7149e07ae <- reads the mode-000 file regardless +# as postgres (empty) <- the failure these arms need +# +# The tree therefore lives outside any 0700 directory and is read by a second +# user. Where no such user exists the arms SKIP loudly rather than pass quietly: +# a guard that cannot run is not a guard that held. _fp="$(mktemp -d "${TMPDIR:-/tmp}/pgc-fpfail.XXXXXX")" -mkdir -p "$_fp/tree/src" "$_fp/bin" +mkdir -p "$_fp/tree/src" printf 'int a;\n' > "$_fp/tree/src/a.c" printf 'int b;\n' > "$_fp/tree/src/b.c" printf 'int c;\n' > "$_fp/tree/src/c.c" printf 'all:\n\ttrue\n' > "$_fp/tree/Makefile" printf 'x\n' > "$_fp/tree/pgcolumnar.control" +chmod -R a+rX "$_fp" + +_fp_user="" +if [ "$(id -u)" -ne 0 ]; then + _fp_user="-" # already unprivileged; read in this shell +else + for _fp_u in postgres nobody; do + id -u "$_fp_u" >/dev/null 2>&1 && { _fp_user="$_fp_u"; break; } + done +fi + +# _fp_as reads as the unprivileged user, or in this shell when already one. +_fp_as() { # _fp_as EXPR -> stdout + if [ "$_fp_user" = "-" ]; then + bash -c ". \"$PGC_TESTDIR/lib.sh\" || exit 1; $1" + else + runuser -u "$_fp_user" -- bash -c ". \"$PGC_TESTDIR/lib.sh\" || exit 1; $1" + fi +} -# A stub that behaves exactly like md5sum except on its Nth invocation, where it -# fails the way a fork failure or an OOM kill does: no output, non-zero status. -cat > "$_fp/bin/md5sum" <<'STUB' -#!/bin/bash -_n=$(( $(cat "$PGC_FP_COUNT" 2>/dev/null || echo 0) + 1 )) -echo "$_n" > "$PGC_FP_COUNT" -[ "$_n" = "${PGC_FP_FAIL_ON:-0}" ] && exit 1 -exec /usr/bin/md5sum "$@" -STUB -chmod +x "$_fp/bin/md5sum" - -_fp_base="$(pgc_source_fingerprint "$_fp/tree")" -check "premise: the tree fingerprints to something on an unstubbed run" \ - "$([ -n "$_fp_base" ] && echo yes || echo empty)" "yes" - -# Premise for the stub itself: with no failure configured it must agree with the -# real thing, or the arms below would be measuring the stub rather than the fix. -PGC_FP_COUNT="$_fp/count"; export PGC_FP_COUNT -PGC_FP_FAIL_ON=0; export PGC_FP_FAIL_ON -echo 0 > "$PGC_FP_COUNT" -check "premise: the stub agrees with the real md5sum when nothing fails" \ - "$(PATH="$_fp/bin:$PATH" pgc_source_fingerprint "$_fp/tree")" "$_fp_base" - -# THE ARM. One failed digest, and the answer must be EMPTY rather than a hash. -for _fp_n in 2 3; do - PGC_FP_FAIL_ON="$_fp_n"; export PGC_FP_FAIL_ON - echo 0 > "$PGC_FP_COUNT" - _fp_got="$(PATH="$_fp/bin:$PATH" pgc_source_fingerprint "$_fp/tree")" - check "a failed digest on file $_fp_n yields no fingerprint, not a wrong one" \ - "$([ -z "$_fp_got" ] && echo empty || echo "$_fp_got")" "empty" -done - -# And the verdict that follows from it, which is the property that matters: the -# controller must say UNVERIFIED, never `stale`. `stale` is the FATAL. -PGC_FP_FAIL_ON=2; export PGC_FP_FAIL_ON -echo 0 > "$PGC_FP_COUNT" -check "so the verdict is unknown -- UNVERIFIED -- and never stale" \ - "$(pgc_freshness_verdict "$_fp_base" \ - "$(PATH="$_fp/bin:$PATH" pgc_source_fingerprint "$_fp/tree")")" "unknown" - -unset PGC_FP_FAIL_ON PGC_FP_COUNT +if [ -z "$_fp_user" ]; then + echo "SKIP no non-root user to read as; root ignores chmod 000" +else + _fp_base="$(_fp_as "pgc_source_fingerprint \"$_fp/tree\"")" + check "premise: the tree fingerprints to something when it is readable" \ + "$([ -n "$_fp_base" ] && echo yes || echo empty)" "yes" + + # Premise for the mechanism: the unprivileged reader must agree with this + # shell while nothing is denied, or the arms below measure the user switch. + check "premise: the unprivileged read agrees while everything is readable" \ + "$_fp_base" "$(pgc_source_fingerprint "$_fp/tree")" + + # THE ARM. One unreadable file, and the answer must be EMPTY, not a hash. + for _fp_n in b.c c.c; do + chmod 000 "$_fp/tree/src/$_fp_n" + _fp_got="$(_fp_as "pgc_source_fingerprint \"$_fp/tree\"")" + chmod 644 "$_fp/tree/src/$_fp_n" + check "an unreadable $_fp_n yields no fingerprint, not a wrong one" \ + "$([ -z "$_fp_got" ] && echo empty || echo "$_fp_got")" "empty" + done + + check "control: and the tree fingerprints again once it is readable" \ + "$(_fp_as "pgc_source_fingerprint \"$_fp/tree\"")" "$_fp_base" + + # And the verdict that follows, which is the property that matters: the + # controller must say UNVERIFIED, never `stale`. `stale` is the FATAL. + chmod 000 "$_fp/tree/src/b.c" + check "so the verdict is unknown -- UNVERIFIED -- and never stale" \ + "$(pgc_freshness_verdict "$_fp_base" \ + "$(_fp_as "pgc_source_fingerprint \"$_fp/tree\"")")" "unknown" + chmod 644 "$_fp/tree/src/b.c" + check "control: a readable run still reads fresh" \ + "$(pgc_freshness_verdict "$_fp_base" \ + "$(_fp_as "pgc_source_fingerprint \"$_fp/tree\"")")" "fresh" +fi +unset _fp_user _fp_u _fp_got _fp_n +unset -f _fp_as # --------------------------------------------------------------------------- # ONE TREE HASHES ONE WAY, HOWEVER THE PATH TO IT IS SPELLED. @@ -766,3 +797,51 @@ _fr_hollow="$(mktemp -d "${TMPDIR:-/tmp}/pgc-rhollow.XXXXXX")" check "an empty manifest is reported as empty, not as silence" \ "$(pgc_freshness_report "$_fr_hollow" | grep -c 'empty -- nothing under')" "1" unset _fr _fr_hollow + + +# ---- one tree hashes one way, however the LOCALE is set --------------------- +# +# The defect the single implementation removed on the way (#907). The shell +# sorted its manifest with `sort -z`, which uses LOCALE COLLATION, and nothing in +# this harness pins a locale. So the same tree fingerprinted two ways depending +# on whose machine it was: +# +# LC_ALL=C 6d122a7158d5 +# LC_ALL=en_US.UTF-8 0b59bd75fa4f +# +# en_US.UTF-8 is a common desktop default, so this was a developer stamping a +# tree and CI reading it back under C.UTF-8 and calling the binary stale. The +# module sorts BYTES, which is what LC_ALL=C did and what every stamp already on +# disk was written with. +# +# `_` against `-` is what the two collations order differently, and +# columnar_arrow.c beside columnar-arrow.c is not a contrived pair in this tree. +_lc="$(mktemp -d "${TMPDIR:-/tmp}/pgc-locale.XXXXXX")" +mkdir -p "$_lc/tree/src" +for _lc_n in columnar_arrow.c columnar-arrow.c columnarXarrow.c Columnar.c columnar.c; do + printf 'int x; /* %s */\n' "$_lc_n" > "$_lc/tree/src/$_lc_n" +done +printf 'all:\n\ttrue\n' > "$_lc/tree/Makefile" +printf 'x\n' > "$_lc/tree/pgcolumnar.control" + +_lc_have="" +for _lc_l in C C.utf8 en_US.utf8; do + locale -a 2>/dev/null | grep -qx "$_lc_l" && _lc_have="$_lc_have $_lc_l" +done +_lc_count="$(printf '%s\n' $_lc_have | grep -c .)" + +check "premise: at least two locales are installed to compare" \ + "$([ "$_lc_count" -ge 2 ] && echo enough || echo "$_lc_count")" "enough" + +if [ "$_lc_count" -ge 2 ]; then + _lc_vals="" + for _lc_l in $_lc_have; do + _lc_vals="$_lc_vals $(LC_ALL="$_lc_l" LANG="$_lc_l" pgc_source_fingerprint "$_lc/tree")" + done + check "premise: the fixture fingerprints at all" \ + "$([ -n "$(printf '%s' $_lc_vals)" ] && echo yes || echo empty)" "yes" + check "one tree, one fingerprint, whatever the locale" \ + "$(printf '%s\n' $_lc_vals | sort -u | grep -c .)" "1" +fi +rm -rf "$_lc" +unset _lc _lc_n _lc_l _lc_have _lc_count _lc_vals diff --git a/test/selftest/380-the-pytest-cluster-helpers.sh b/test/selftest/380-the-pytest-cluster-helpers.sh index aa0eec0d..9eeec064 100644 --- a/test/selftest/380-the-pytest-cluster-helpers.sh +++ b/test/selftest/380-the-pytest-cluster-helpers.sh @@ -29,21 +29,47 @@ _pc_cl="$PGC_TESTDIR/pytest/pgc_cluster.py" check "premise: the pytest cluster helper is where this part thinks it is" \ "$([ -f "$_pc_cl" ] && echo yes || echo no)" "yes" -check "the fingerprint derives its build directories from a Makefile glob" \ - "$(grep -c 'glob("\*/Makefile")' "$_pc_cl")" "1" +# THE FINGERPRINT MOVED (#907). It was an independent Python implementation in +# pgc_cluster.py and an independent shell one in lib.sh; the pair produced four +# defects in a day. Both now delegate to test/pgc_fingerprint.py, so these arms +# follow the property to where it lives. Left pointed at pgc_cluster.py they +# would have gone GREEN by finding nothing to object to, which is why each one +# below is mirrored against a fixture that must make it red. +_pc_fp="$PGC_TESTDIR/pgc_fingerprint.py" -# A list would pass the arm above if someone added the glob AND kept a name. +check "premise: the one fingerprint implementation is where this part thinks it is" \ + "$([ -f "$_pc_fp" ] && echo yes || echo no)" "yes" + +check "the fingerprint derives its build directories from a Makefile on disk" \ + "$(grep -c '_is_plain_file(d / "Makefile")' "$_pc_fp")" "1" + +# A list would pass the arm above if someone added the derivation AND kept a name. check "and it names no module directory, so it is a derivation and not a list" \ - "$(grep -c '"objstore"' "$_pc_cl")" "0" + "$(grep -c '"objstore"' "$_pc_fp")" "0" # src/module.c and objstore/module.c are different inputs. Hashing the bare name -# would make them interchangeable, which is a collision the glob itself cannot -# prevent. +# would make them interchangeable, which is a collision the derivation cannot +# prevent by itself. check "the hash mixes in each file's path relative to the tree, not its name" \ - "$(grep -c 'relative_to(srcdir)' "$_pc_cl")" "1" + "$(grep -c 'relative_to(root)' "$_pc_fp")" "1" check "and no longer mixes in the bare filename" \ - "$(grep -c 'h.update(path.name.encode())' "$_pc_cl")" "0" + "$(grep -c 'h.update(path.name.encode())' "$_pc_fp")" "0" + +# AND NEITHER CALLER MAY KEEP A PRIVATE COPY. This is the arm that would catch +# #907 recurring: the whole point is one implementation, so a second one +# reappearing in either caller is the defect, not a detail. +check "the pytest helper keeps no private fingerprint implementation" \ + "$(grep -cE 'hashlib\.md5|glob\("\*/Makefile"\)' "$_pc_cl")" "0" + +check "and the shell keeps none either" \ + "$(grep -cE 'md5sum < |xargs -0 cat' "$PGC_TESTDIR/lib.sh")" "0" + +# STDLIB ONLY, AND NOTHING FROM test/pytest/. lib.sh runs this module with the +# SYSTEM interpreter; an import from the pytest tree or a third-party package +# would make every bash suite unrunnable until somebody installed pytest. +check "the module imports nothing from the pytest tree" \ + "$(grep -cE '^(import|from) +(pgc_|conftest|pytest|psycopg)' "$_pc_fp")" "0" # ---- and the lifecycle ------------------------------------------------------ # @@ -80,9 +106,19 @@ printf 'def make_cluster(a, b):\n root = mkdtemp()\n return cluster, root\ check "a make_cluster with no cleanup is caught" \ "$(grep -c 'shutil.rmtree(root' "$_pc_fix/noguard.py")" "0" -printf ' dirs = [srcdir / "src"]\n' > "$_pc_fix/srconly.py" +printf ' dirs = [root / "src"]\n' > "$_pc_fix/srconly.py" check "a fingerprint that reads src only is caught" \ - "$(grep -c 'glob("\*/Makefile")' "$_pc_fix/srconly.py")" "0" + "$(grep -c '_is_plain_file(d / "Makefile")' "$_pc_fix/srconly.py")" "0" + +# And the private-copy arms must be able to redden too, or "0" above means only +# that the pattern matches nothing anywhere. +printf 'import hashlib\nh = hashlib.md5(b"")\n' > "$_pc_fix/privatecopy.py" +check "a caller that reimplements the digest is caught" \ + "$(grep -cE 'hashlib\.md5' "$_pc_fix/privatecopy.py")" "1" + +printf 'from pgc_cluster import x\n' > "$_pc_fix/badimport.py" +check "an import from the pytest tree is caught" \ + "$(grep -cE '^(import|from) +(pgc_|conftest|pytest|psycopg)' "$_pc_fix/badimport.py")" "1" printf ' dirs = [srcdir / "src", srcdir / "objstore"]\n' > "$_pc_fix/listed.py" check "and a hard-coded module list is caught by the name arm" \ @@ -90,7 +126,12 @@ check "and a hard-coded module list is caught by the name arm" \ # The mirror: the same three greps on the REAL file, so a zero above is a missing # guard rather than a pattern that matches nothing anywhere. -check "premise: while the real helper satisfies the two it must" \ - "$(grep -cE 'glob\("\*/Makefile"\)|shutil.rmtree\(root' "$_pc_cl")" "2" +# The mirror: the same greps on the REAL files, so a zero above is a missing +# guard rather than a pattern that matches nothing anywhere. +check "premise: while the real module satisfies the derivation arm" \ + "$(grep -cE '_is_plain_file\(d / "Makefile"\)' "$_pc_fp")" "1" + +check "premise: and the real helper still carries its cleanup" \ + "$(grep -cE 'shutil.rmtree\(root' "$_pc_cl")" "1" -unset _pc_cl _pc_body _pc_fix +unset _pc_cl _pc_fp _pc_body _pc_fix