diff --git a/test/devloop.sh b/test/devloop.sh index 5be67552..2d995863 100755 --- a/test/devloop.sh +++ b/test/devloop.sh @@ -95,7 +95,7 @@ fi if ( . "$BUILD/test/lib.sh" pgc_write_source_stamp \ - "$(pgc_source_stamp_path "$BUILD" "$(pgc_major_of "$PGC")")" \ + "$(pgc_source_stamp_path "$BUILD" "$PGC")" \ "$(pgc_source_fingerprint "$BUILD")" ); then : diff --git a/test/lib.sh b/test/lib.sh index 762f42e3..9fc7baf3 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -200,7 +200,7 @@ pgc_build_and_install() { # against, so a suite measuring an edited tree reported "matches the binary # under test". A red arm caught it, which is the only reason this exists. pgc_write_source_stamp \ - "$(pgc_source_stamp_path "$_pgc_bi_src" "$_pgc_bi_major")" \ + "$(pgc_source_stamp_path "$_pgc_bi_src" "$_pgc_bi_cfg")" \ "$(pgc_source_fingerprint "$_pgc_bi_src")" return 0 } @@ -284,7 +284,7 @@ pgc_setup() { # And verify it, whether this run built or skipped. A skipped build is exactly # when the binary can be older than the source. _pgc_fresh_recorded="$(pgc_read_source_stamp \ - "$(pgc_source_stamp_path "$PGC_SRCDIR" "$PGC_MAJOR")")" + "$(pgc_source_stamp_path "$PGC_SRCDIR" "$PGC_PG_CONFIG")")" _pgc_fresh_current="$(pgc_source_fingerprint "$PGC_SRCDIR")" case "$(pgc_freshness_verdict "$_pgc_fresh_recorded" "$_pgc_fresh_current")" in fresh) @@ -656,7 +656,27 @@ pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash 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 | xargs -0 cat 2>/dev/null | md5sum | cut -c1-12 + } | 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. + printf '%s %s\n' "${_pgc_fp_f#"$dir"/}" \ + "$(md5sum < "$_pgc_fp_f" 2>/dev/null | cut -d' ' -f1)" + done | md5sum | cut -c1-12 } # fresh the binary was built from this source @@ -742,7 +762,18 @@ pgc_running_binary_verdict() { # pgc_running_binary_verdict SO_EPOCH PM_EPOCH } pgc_write_source_stamp() { # pgc_write_source_stamp FILE HASH - printf '%s\n' "${2:-}" > "${1:-/dev/null}" 2>/dev/null || true + # NO `|| true`. It was there, and it made both controllers' warning branches + # UNREACHABLE: run_all_versions.sh and devloop.sh each wrap this in `if (...)` + # and promise to say so when the stamp cannot be written, and each carries a + # comment saying "NOT || true" -- while the function they call swallowed the + # status (@linuxhikerpm, #898 review). Driven against an unwritable target: + # + # write_rc=0 exists=no + # + # The stamp absent, nothing warned, every child suite degraded to UNVERIFIED. + # A comment that argues for a guarantee the code does not provide is worse + # than no comment, because it stops the next person checking. + printf '%s\n' "${2:-}" > "${1:-/dev/null}" 2>/dev/null } pgc_read_source_stamp() { # pgc_read_source_stamp FILE -> hash or empty @@ -762,8 +793,37 @@ pgc_major_of() { # pgc_major_of PG_CONFIG -> major "$1" --version | sed -E 's/^[^0-9]*([0-9]+).*/\1/' } -pgc_source_stamp_path() { # pgc_source_stamp_path DIR MAJOR - printf '%s/.pgc_source_stamp.%s\n' "${1:-.}" "${2:-0}" +pgc_source_stamp_path() { # pgc_source_stamp_path DIR PG_CONFIG + # KEYED BY THE INSTALLATION, NOT ONLY THE MAJOR. The key was + # `.pgc_source_stamp.`, and the comment above it already said that one + # tree installs into several prefixes each with its own binary -- so the key + # discarded the distinction the comment drew (@linuxhikerpm, #898 review). + # + # Not hypothetical on this box: pg18a, pg18n and pg18_san are three PG18 + # installations with different pkglibdirs, and all three resolved to + # `.pgc_source_stamp.18`. Build current source into one prefix, then run + # PGC_SKIP_BUILD=1 against another, and the fingerprint matches while the + # binary is stale -- and the postmaster arm passes too, because the freshly + # started server is newer than the old .so. The run then reports fresh while + # executing the other prefix's binary, which is this file's whole subject. + # + # pkglibdir rather than the pg_config path, because that is where the .so + # actually lands: two pg_configs pointing at one prefix ARE the same + # installation and should share a stamp. + local _pgc_sp_dir="${1:-.}" _pgc_sp_cfg="${2:-}" + local _pgc_sp_major _pgc_sp_lib _pgc_sp_id + _pgc_sp_major="$(pgc_major_of "$_pgc_sp_cfg" 2>/dev/null)" + _pgc_sp_lib="$("$_pgc_sp_cfg" --pkglibdir 2>/dev/null)" + # An unreadable pg_config gets a key that matches nothing rather than one + # every broken config shares: `unknown` would alias them together, which is + # the defect being fixed, one level down. + if [ -n "$_pgc_sp_lib" ]; then + _pgc_sp_id="$(printf '%s' "$_pgc_sp_lib" | md5sum | cut -c1-8)" + else + _pgc_sp_id="nolib$(printf '%s' "$_pgc_sp_cfg" | md5sum | cut -c1-3)" + fi + printf '%s/.pgc_source_stamp.%s.%s\n' \ + "$_pgc_sp_dir" "${_pgc_sp_major:-0}" "$_pgc_sp_id" } pgc_write_build_stamp() { diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index b15d2ecd..1b455763 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. -**74 tests in 6 files.** Fifty-nine of them test the harness rather than the +**78 tests in 6 files.** Sixty-three 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. @@ -330,6 +330,10 @@ is where a wrong quote would hide. | `test_the_fingerprint_covers_a_separately_built_module` | an `objstore/` edit moves the hash | | `test_an_objstore_edit_forces_a_second_build` | and forces a rebuild, end to end | | `test_make_cluster_leaves_nothing_behind_when_setup_fails` | a failed setup leaks no directory | +| `test_the_stamp_writer_reports_failure` | `\|\| true` made both controllers' warnings unreachable | +| `test_two_installations_of_one_major_do_not_share_a_stamp` | the key names the installation, not just the major | +| `test_moving_bytes_between_files_moves_the_shell_fingerprint` | the digest sees a repartition | +| `test_the_two_fingerprint_implementations_cover_the_same_inputs` | **the two implementations move on the same edits** | ### The build/start ORDER, which is not a detail @@ -344,6 +348,40 @@ after the alpha4 rebase gave 15 cluster-start errors and the second run passed. **A flake that clears on a second run is what a stale-binary defect looks like from outside.** +### The twin of `selftest/340`, and the fourth instance of one defect + +These four drive the **shell** functions through `bash` rather than +reimplementing them, and they are the pytest half of `test/selftest/340`'s stamp +arms, owed under the twin rule and payable only once `test/pytest/` reached +`main` with #897. + +The last one is the interesting one. `source_fingerprint` in `pgc_cluster.py` +says in its own docstring that it uses *"the same input set as +`pgc_source_fingerprint` in `test/lib.sh`"*. It did not. The shell hashes each +build directory's `*.c`, `*.h` **and `Makefile`**; this side read only the +sources, so editing `objstore/Makefile` — which changes how that module builds — +moved one hash and not the other: + +``` +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 finding one layer over**: they found the module's *sources* +missing from this implementation, and the module's *Makefile* was still missing +after that was fixed. + +The arm asserts the property the docstring always claimed, and not more: the two +hashes are **not** required to be equal — they are different digests over the +same files, used independently — but **the same edit must move both**. It walks +five edits: a source, a module source, a module Makefile, the top-level +Makefile, and the control file. + +Two implementations of one idea have now been separately wrong, separately +fixed, and a third party had to find each. That is the argument for making them +one. + ### Two findings from @linuxhikerpm, both about infrastructure rather than coverage **The fingerprint read `src/` only.** `objstore/` is a separately built shared diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index 0db12d44..43fce107 100644 --- a/test/pytest/pgc_cluster.py +++ b/test/pytest/pgc_cluster.py @@ -409,7 +409,19 @@ def source_fingerprint(srcdir): srcdir = pathlib.Path(srcdir) paths = [] for d in source_build_dirs(srcdir): - paths += list(d.glob("*.c")) + list(d.glob("*.h")) + # 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()] diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index e0addc60..71cc9773 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -344,3 +344,114 @@ def test_make_cluster_leaves_nothing_behind_when_setup_fails(tmp_path, expect): leaked = sorted(set(glob.glob("/tmp/pgc-pytest-*")) - before) expect.text(", ".join(leaked) or "none", "none", "a failed make_cluster leaves no directory behind") + + +# --------------------------------------------------------------------------- +# THE PYTEST TWIN of test/selftest/340's stamp arms, owed under jd's rule of +# 2026-09-23... 2026-09-09: every test written twice. The .sh half could not +# have one until test/pytest/ existed on main, which it now does (#897). +# +# These drive the SHELL functions through bash rather than reimplementing them, +# for the reason that keeps being proved this week: a second implementation of +# one idea drifts, and the drift is invisible until someone diffs the two. + + +# The tree this corpus belongs to, derived the same way conftest.py derives it. +SRCDIR = pathlib.Path(__file__).resolve().parents[2] + + +def _sh(srcdir, expr): + """Evaluate one lib.sh expression against a tree, and return its stdout.""" + script = f'. "{SRCDIR}/test/lib.sh" || exit 1; {expr}' + p = subprocess.run(["bash", "-c", script], capture_output=True, text=True) + return p.stdout.strip(), p.returncode + + +def _tree_with_module(tmp_path, name): + t = tmp_path / name + (t / "src").mkdir(parents=True, exist_ok=True) + (t / "objstore").mkdir(parents=True, exist_ok=True) + (t / "src" / "a.c").write_text("int a;\n") + (t / "objstore" / "b.c").write_text("int b;\n") + (t / "objstore" / "Makefile").write_text("all:\n\ttrue\n") + (t / "Makefile").write_text("all:\n\t$(MAKE) -C objstore\n") + (t / "pgcolumnar.control").write_text("x\n") + return t + + +def test_the_stamp_writer_reports_failure(tmp_path, expect): + """`|| true` made both controllers' warning branches unreachable.""" + out, rc = _sh(tmp_path, 'pgc_write_source_stamp "/proc/pgc-twin" "deadbeef"') + expect.num(rc, 1, "the writer reports failure on an unwritable target") + ok, rc2 = _sh(tmp_path, f'pgc_write_source_stamp "{tmp_path}/s" "cafebabe"') + expect.num(rc2, 0, "control: and succeeds on a writable one") + + +def test_two_installations_of_one_major_do_not_share_a_stamp(tmp_path, expect): + """The stamp key must name the installation, not only the major.""" + cfgs = [] + for n in ("a", "b"): + c = tmp_path / f"pg_config.{n}" + c.write_text('#!/bin/sh\ncase "$1" in\n' + ' --version) echo "PostgreSQL 18.4" ;;\n' + f' --pkglibdir) echo "/usr/local/pg18{n}/lib" ;;\nesac\n') + c.chmod(0o755) + cfgs.append(c) + a, _ = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[0]}"') + b, _ = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[1]}"') + expect.text(str(a != b), "True", "two prefixes of one major get different stamps") + a2, _ = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[0]}"') + expect.text(a2, a, "control: the same pg_config twice gives the same path") + + +def test_moving_bytes_between_files_moves_the_shell_fingerprint(tmp_path, expect): + """`xargs -0 cat | md5sum` could not see a repartition.""" + t = _tree_with_module(tmp_path, "rp") + before, _ = _sh(t, f'pgc_source_fingerprint "{t}"') + expect.at_least(len(before), 12, "premise: the tree fingerprints at all") + (t / "src" / "a.c").write_text("int a;\nint b;\n") + (t / "objstore" / "b.c").write_text("") + after, _ = _sh(t, f'pgc_source_fingerprint "{t}"') + expect.text(str(after != before), "True", + "moving bytes between files moves the fingerprint") + + +def test_the_two_fingerprint_implementations_cover_the_same_inputs(tmp_path, expect): + """THE PROPERTY THE TWO IMPLEMENTATIONS MUST SHARE, and the one they did not. + + `source_fingerprint` in pgc_cluster.py says in its own docstring that it uses + "the same input set as pgc_source_fingerprint in test/lib.sh". It did not: + the shell hashes each build directory's `*.c`, `*.h` AND `Makefile`, while + the Python read only `*.c` and `*.h` there. Editing `objstore/Makefile` -- + which changes how that module builds -- moved the shell hash and not the + Python one, so `build_once` certified a stale module as current: + + baseline shell=45be41a5c47b python=bea88c7d79ca + objstore/Makefile edited shell=cfb8f4553041 python=bea88c7d79ca + + That is @linuxhikerpm's finding one layer over: they found the .c files + 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. + """ + t = _tree_with_module(tmp_path, "cover") + for edit, path, body in ( + ("a source file", t / "src" / "a.c", "int a = 2;\n"), + ("a module source", t / "objstore" / "b.c", "int b = 2;\n"), + ("a module Makefile", t / "objstore" / "Makefile", "all:\n\ttrue # x\n"), + ("the top-level Makefile", t / "Makefile", "all:\n\t$(MAKE) -C objstore # x\n"), + ("the control file", t / "pgcolumnar.control", "y\n"), + ): + sh_before, _ = _sh(t, f'pgc_source_fingerprint "{t}"') + py_before = source_fingerprint(t) + old = path.read_text() + path.write_text(body) + sh_after, _ = _sh(t, f'pgc_source_fingerprint "{t}"') + py_after = source_fingerprint(t) + path.write_text(old) + expect.text(f"{sh_after != sh_before} {py_after != py_before}", "True True", + f"editing {edit} moves both fingerprints") diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 4371be84..b6330593 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -726,7 +726,7 @@ for pgc in "${CONFIGS[@]}"; do if ( . "$builddir/test/lib.sh" pgc_write_source_stamp \ - "$(pgc_source_stamp_path "$builddir" "$major")" \ + "$(pgc_source_stamp_path "$builddir" "$pgc")" \ "$(pgc_source_fingerprint "$builddir")" ); then : 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 8064fdff..14366f49 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -178,3 +178,260 @@ check "an unreadable library is not a failure" "$_rb_rc3" "0" check "but it says which question went unanswered" \ "$(printf '%s\n' "$_rb_out3" | grep -c 'could not compare')" "1" rm -rf "$_rb_dir" + +# ---- three false-freshness paths @linuxhikerpm reproduced (#898 review) ------ +# +# All three are the same failure this file exists to prevent: the run reports +# FRESH while the binary is stale. Each was reproduced against the branch before +# it was fixed, and each arm below drives the REAL function rather than a copy. + +# 1. THE WRITER COULD NOT REPORT FAILURE. `|| true` on the redirect meant both +# controllers' `if (...)` warning branches were unreachable -- and both carry +# a comment saying "NOT || true", so the comments argued for a guarantee the +# code did not provide. Measured: write_rc=0 exists=no. +_fs_bad="/proc/pgc-source-stamp-selftest" # a path that cannot be created +pgc_write_source_stamp "$_fs_bad" "deadbeef" 2>/dev/null +check "the stamp writer reports failure on an unwritable target" \ + "$?" "1" + +check "premise: and the stamp really was not written, so the arm is not vacuous" \ + "$([ -f "$_fs_bad" ] && echo written || echo absent)" "absent" + +_fs_ok="$PGC_WORKDIR/stamp.ok" +pgc_write_source_stamp "$_fs_ok" "cafebabe" +check "control: and it still succeeds on a writable one" "$?" "0" + +check "control: writing the value it was given" "$(cat "$_fs_ok" 2>/dev/null)" "cafebabe" + +# 2. THE DIGEST COULD NOT SEE A REPARTITION. `xargs -0 cat | md5sum` hashed the +# concatenated stream, so moving bytes BETWEEN files left the hash unchanged +# while the source stopped compiling: +# before_hash=bfce474cc159 after_hash=bfce474cc159 +# initial_compile=0 repartitioned_compile=1 error: redefinition of 'x' +_fs_rp="$PGC_WORKDIR/repartition"; rm -rf "$_fs_rp"; mkdir -p "$_fs_rp/src" +printf 'all:\n\ttrue\n' > "$_fs_rp/Makefile" +printf 'static int x=1;\n' > "$_fs_rp/src/a.c" +printf 'static int x=2;\n' > "$_fs_rp/src/b.c" +_fs_before="$(pgc_source_fingerprint "$_fs_rp")" + +check "premise: the fixture fingerprints at all" \ + "$([ -n "$_fs_before" ] && echo yes || echo no)" "yes" + +# The same bytes, a different partition: a.c gains b.c's line and b.c is emptied. +printf 'static int x=1;\nstatic int x=2;\n' > "$_fs_rp/src/a.c" +: > "$_fs_rp/src/b.c" +check "moving bytes between files moves the fingerprint" \ + "$([ "$(pgc_source_fingerprint "$_fs_rp")" != "$_fs_before" ] && echo moved || echo SAME)" \ + "moved" + +# And back, so the arm above is about the partition rather than about any edit. +printf 'static int x=1;\n' > "$_fs_rp/src/a.c" +printf 'static int x=2;\n' > "$_fs_rp/src/b.c" +check "and restoring the partition restores the fingerprint" \ + "$(pgc_source_fingerprint "$_fs_rp")" "$_fs_before" + +# Renaming a file is also a repartition: same bytes, different translation unit. +mv "$_fs_rp/src/b.c" "$_fs_rp/src/c.c" +check "renaming a source file moves the fingerprint too" \ + "$([ "$(pgc_source_fingerprint "$_fs_rp")" != "$_fs_before" ] && echo moved || echo SAME)" \ + "moved" + +# 3. "KEYED BY MAJOR" ALIASED DISTINCT INSTALLATIONS. Two PG18 prefixes with +# different pkglibdirs both resolved to `.pgc_source_stamp.18`, so a build +# into one certified the other. This box really has pg18a, pg18n and +# pg18_san -- but the arm uses FAKE pg_configs so it does not depend on which +# majors happen to be installed here. +_fs_cfg="$PGC_WORKDIR/cfgs"; rm -rf "$_fs_cfg"; mkdir -p "$_fs_cfg" +for _fs_n in a b; do + { + printf '#!/bin/sh\n' + printf 'case "$1" in\n' + printf ' --version) echo "PostgreSQL 18.4" ;;\n' + printf ' --pkglibdir) echo "/usr/local/pg18%s/lib/postgresql" ;;\n' "$_fs_n" + printf 'esac\n' + } > "$_fs_cfg/pg_config.$_fs_n" + chmod 755 "$_fs_cfg/pg_config.$_fs_n" +done + +_fs_pa="$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.a")" +_fs_pb="$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.b")" + +check "premise: both fake configs report the same major, which is the whole point" \ + "$(pgc_major_of "$_fs_cfg/pg_config.a")=$(pgc_major_of "$_fs_cfg/pg_config.b")" "18=18" + +check "two installations of one major get different stamp paths" \ + "$([ "$_fs_pa" != "$_fs_pb" ] && echo different || echo "SAME:$_fs_pa")" "different" + +check "and the major is still readable in the name" \ + "$(printf '%s' "$_fs_pa" | grep -c '\.pgc_source_stamp\.18\.')" "1" + +# The same installation must still resolve to ONE stamp, or every run rebuilds +# and the check never has a recorded value to compare against. +check "control: the same pg_config twice gives the same path" \ + "$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.a")" "$_fs_pa" + +# Two pg_configs pointing at ONE prefix are the same installation and share a +# stamp: the key is pkglibdir, not the pg_config path. +cp "$_fs_cfg/pg_config.a" "$_fs_cfg/pg_config.a2" +check "and two pg_configs for one prefix share a stamp, keyed on pkglibdir" \ + "$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.a2")" "$_fs_pa" + +# An unreadable pg_config must not land every broken config on one key, which is +# the aliasing defect one level down. +printf '#!/bin/sh\nexit 1\n' > "$_fs_cfg/pg_config.broken"; chmod 755 "$_fs_cfg/pg_config.broken" +printf '#!/bin/sh\nexit 1\n' > "$_fs_cfg/pg_config.broken2"; chmod 755 "$_fs_cfg/pg_config.broken2" +check "two unreadable pg_configs do not alias onto one stamp" \ + "$([ "$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.broken")" \ + != "$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.broken2")" ] \ + && echo different || echo SAME)" "different" + +unset _fs_bad _fs_ok _fs_rp _fs_before _fs_cfg _fs_n _fs_pa _fs_pb + +# ---- every caller must pass a pg_config, not a major ------------------------ +# +# WHY THIS EXISTS, AND WHY IT IS A SWEEP RATHER THAN AN ARM ABOUT ONE LINE. +# Changing pgc_source_stamp_path from `DIR MAJOR` to `DIR PG_CONFIG` changes a +# CONTRACT, and a contract change is only as good as the sweep that finds every +# caller. @jdatcmd merged the two branches and found the case this misses +# (#903 review): #897 adds a NEW writer inside pgc_build_and_install, and +# `test/lib.sh` conflicts in pgc_setup while that new call site merges CLEANLY +# and is then wrong. The resolution you are shown is not the defect. +# +# What the mismatch costs, measured by driving the real function both ways: +# +# correct (PG_CONFIG): /t/.pgc_source_stamp.18.603d6145 +# a major passed instead: /t/.pgc_source_stamp.0.nolib6f4 +# +# `pgc_major_of 18` finds no version in the string "18", so the major becomes 0 +# and the id becomes a hash of the literal "18". The writer and the reader then +# address DIFFERENT FILES and never meet, so every suite reports +# "freshness UNVERIFIED" -- and unknown is deliberately not a failure, so +# NOTHING SAYS SO. Worse for the case this file exists for: a key derived from +# the literal "18" is the same for every PG18 prefix, so pg18a, pg18n and +# pg18_san collide again on the writer path. +# +# So this arm is about the CLASS. It reddens for any caller, in any file, that +# passes a major where a pg_config belongs -- including one that arrives through +# a clean merge in a hunk nobody was asked to resolve. + +_sc_sites="$(grep -rn 'pgc_source_stamp_path ' "$PGC_TESTDIR"/*.sh "$PGC_TESTDIR"/selftest/*.sh 2>/dev/null \ + | grep -v 'pgc_source_stamp_path() {')" + +# A sweep that found nothing reports "no bad callers" and looks exactly like a +# sweep that works. +check "premise: the sweep finds the call sites it is meant to police" \ + "$([ "$(printf '%s\n' "$_sc_sites" | grep -c .)" -ge 3 ] && echo enough \ + || printf '%s\n' "$_sc_sites" | grep -c .)" "enough" + +# The second argument, per call site. A major-shaped one is the defect: a bare +# integer, or a variable whose name ends in _major, or PGC_MAJOR. +_sc_bad=""; _sc_n=0 +while IFS= read -r _sc_l; do + [ -n "$_sc_l" ] || continue + _sc_arg="$(printf '%s' "$_sc_l" | sed -n 's/.*pgc_source_stamp_path "[^"]*" "\([^"]*\)".*/\1/p')" + [ -n "$_sc_arg" ] || continue + case "$_sc_arg" in + '$PGC_MAJOR' | *_major'}' | *_major | [0-9] | [0-9][0-9]) + _sc_n=$((_sc_n + 1)) + [ "$_sc_n" -le 4 ] && _sc_bad="$_sc_bad ${_sc_l%%:*}:$(printf '%s' "$_sc_l" | cut -d: -f2)" + ;; + esac +done < "$_sc_fix/bad.sh" +_sc_probe="$(sed -n 's/.*pgc_source_stamp_path "[^"]*" "\([^"]*\)".*/\1/p' "$_sc_fix/bad.sh")" +check "premise: the argument parser reads the second argument at all" \ + "$_sc_probe" '$_pgc_bi_major' + +check "a caller passing a major is caught" \ + "$(case "$_sc_probe" in '$PGC_MAJOR' | *_major'}' | *_major | [0-9] | [0-9][0-9]) echo caught ;; *) echo MISSED ;; esac)" \ + "caught" + +# The mirror: a correct caller must NOT be flagged, or the arm reddens on every +# healthy tree and gets switched off. +printf '\t\t"$(pgc_source_stamp_path "$_pgc_bi_src" "$_pgc_bi_cfg")" \\\n' \ + > "$_sc_fix/good.sh" +_sc_probe2="$(sed -n 's/.*pgc_source_stamp_path "[^"]*" "\([^"]*\)".*/\1/p' "$_sc_fix/good.sh")" +check "control: a caller passing a pg_config is not flagged" \ + "$(case "$_sc_probe2" in '$PGC_MAJOR' | *_major'}' | *_major | [0-9] | [0-9][0-9]) echo FLAGGED ;; *) echo clean ;; esac)" \ + "clean" + +unset _sc_sites _sc_bad _sc_n _sc_l _sc_arg _sc_fix _sc_probe _sc_probe2 + +# ---- the writer and the reader must address the SAME file ------------------- +# +# THE CLASS, NOT THE INSTANCE (@jdatcmd, #903 review). The arm above sweeps for +# a major passed where a pg_config belongs, which catches the shape. This one +# catches the CONSEQUENCE regardless of shape: it drives the real +# pgc_build_and_install and then asks the real reader for the value, so ANY +# disagreement between the two about which file the stamp lives in reddens here +# -- a renamed variable, a reordered argument, a third caller nobody swept. +# +# It is the property that actually matters. The writer and the reader agreeing +# on a path is what makes the freshness check a check; when they disagree the +# reader finds nothing, the verdict is `unknown`, and unknown is DELIBERATELY +# not a failure -- so the whole mechanism goes quiet and every suite prints +# "freshness UNVERIFIED" while looking healthy. Nothing else in this file +# asserts it. +# +# `make` is stubbed on PATH so this costs nothing: the subject is which path the +# stamp lands on, not whether the tree compiles. + +_wr="$PGC_WORKDIR/writer_reader"; rm -rf "$_wr"; mkdir -p "$_wr/src" "$_wr/bin" +printf 'int x;\n' > "$_wr/src/a.c" +printf 'all:\n\ttrue\n' > "$_wr/Makefile" +printf '#!/bin/sh\nexit 0\n' > "$_wr/bin/make"; chmod 755 "$_wr/bin/make" +{ + printf '#!/bin/sh\n' + printf 'case "$1" in\n' + printf ' --version) echo "PostgreSQL 18.4" ;;\n' + printf ' --pkglibdir) echo "%s/lib" ;;\n' "$_wr" + printf 'esac\n' +} > "$_wr/pg_config"; chmod 755 "$_wr/pg_config" + +# Drive the REAL function, exactly as pgc_setup calls it. +( PATH="$_wr/bin:$PATH"; pgc_build_and_install "$_wr" "$_wr/pg_config" 18 ) >/dev/null 2>&1 +_wr_rc=$? + +check "premise: the build path ran to completion, so a stamp was due" "$_wr_rc" "0" + +# What landed, and what the reader will look for. Globbed rather than computed, +# so this reads the writer's ACTUAL choice instead of re-deriving it. +_wr_written="" +for _wr_f in "$_wr"/.pgc_source_stamp.*; do + [ -e "$_wr_f" ] && _wr_written="$_wr_f" +done +_wr_expected="$(pgc_source_stamp_path "$_wr" "$_wr/pg_config")" + +check "premise: the writer wrote a stamp at all" \ + "$([ -n "$_wr_written" ] && echo yes || echo "none in $_wr")" "yes" + +check "the writer writes the file the reader looks for" \ + "$_wr_written" "$_wr_expected" + +# And end to end: the reader gets the fingerprint back, so the verdict is +# `fresh` rather than `unknown`. +check "and the reader reads back the fingerprint the writer recorded" \ + "$(pgc_read_source_stamp "$_wr_expected")" "$(pgc_source_fingerprint "$_wr")" + +check "so the verdict is fresh, not unknown" \ + "$(pgc_freshness_verdict "$(pgc_read_source_stamp "$_wr_expected")" \ + "$(pgc_source_fingerprint "$_wr")")" "fresh" + +unset _wr _wr_rc _wr_written _wr_expected _wr_f