diff --git a/CHANGELOG.md b/CHANGELOG.md index e22a8099..8b662328 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -503,6 +503,46 @@ true until the next version shipped. there is no aarch64 box here to test it on. What this change does is make the next nightly report the symbol names instead of a bare exit status, so the fix after it is measured rather than reasoned about. +- The cross-major preflight leaves no object tree behind, on a box with no + `pg_config` on `PATH` (#1219). + + `test/build_all_versions.sh` ends with a clean, and its comment says why: + "Leave no object tree behind from whichever major happened to be last: the + next build against a different major would link objects compiled for this + one." It was written as + + ```sh + make -C "$SRCDIR" clean >/dev/null 2>&1 || true + ``` + + with no `PG_CONFIG` and its failure swallowed. PGXS resolves `pg_config` from + `PATH`, so on a box without a packaged one that `make` fails, the `|| true` + eats it, and the tree keeps the last major's objects while the script prints + PASSED. Measured, same tree, pg18a then pg19a: + + ``` + normal PATH built 2 of 2 PASSED objects left: 0 + PATH with no pg_config built 2 of 2 PASSED objects left: 36 + ``` + + **Passing a `pg_config` is not the fix.** `make clean` needs PGXS loaded to do + anything, objects live in `src/` *and* `objstore/`, and a clean whose failure + is swallowed cannot be told from one that worked. The sweep now removes by + `find` -- needing no `pg_config` -- and then verifies, through two functions the + selftest drives directly: + + ``` + pgc_bav_tree_has_objects DIR -> yes | no + pgc_bav_clean_tree SRCDIR [PG_CONFIG] -> clean | dirty + ``` + + End to end, the real script under `env -i PATH=` now leaves + **0 objects** where it left 36. + + The consequence was bounded -- #1221's DWARF provenance catches foreign objects + on the next suite and cleans them -- so this cost a rebuild rather than a wrong + install. What it did not cost is nothing, and the comment promised something it + did not always do. - `pgc_reconcile_records` no longer reports an impossible mismatch on a log it cannot measure (#1242). Handed a log that states `checks run: N` and carries diff --git a/test/build_all_versions.sh b/test/build_all_versions.sh index cb79783a..b960aa6c 100755 --- a/test/build_all_versions.sh +++ b/test/build_all_versions.sh @@ -102,6 +102,48 @@ fi echo "== pgColumnar build check across majors ==" failed=0 +# Remove every build artifact from a tree, and SAY WHETHER IT WORKED (#1219). +# +# The final sweep used to be `make -C "$SRCDIR" clean >/dev/null 2>&1 || true` +# with no PG_CONFIG. PGXS resolves `pg_config` from PATH, so on a box with no +# packaged one that make fails, the `|| true` eats the failure, and the tree +# keeps the last major's objects while the script still prints PASSED. Measured, +# same tree, pg18a then pg19a: +# +# normal PATH built 2 of 2 PASSED objects left: 0 +# PATH with no pg_config built 2 of 2 PASSED objects left: 36 +# +# PASSING A pg_config IS NOT THE FIX. `make clean` needs PGXS loaded to do +# anything at all, objects live in src/ AND objstore/, and a clean whose failure +# is swallowed cannot be told from one that worked. So: try make when a +# pg_config is at hand, because PGXS knows about artifacts this sweep does not +# name; then remove by find, which needs no pg_config; then VERIFY, because a +# sweep that reports nothing is the defect being fixed. +# THE DETECTOR IS ITS OWN FUNCTION so it can be judged directly. Folded into +# the cleaner it was unreachable: in every fixture the sweep works, so `clean` is +# the right answer and a cleaner that ALWAYS says `clean` reddens nothing. +# Measured -- that mutation passed all five arms. Splitting it out is what gives +# the detection logic a killer; the belt-and-braces re-check inside the cleaner +# still only fires when the sweep fails, which cannot be staged as root, and +# that residue is recorded rather than papered over. +pgc_bav_tree_has_objects() { # pgc_bav_tree_has_objects DIR -> yes|no + local _d="${1:-}" + [ -n "$_d" ] && [ -d "$_d" ] || { echo no; return; } + if [ -n "$(find "$_d" \( -name '*.o' -o -name '*.bc' -o -name '*.so' \) -type f 2>/dev/null | head -1)" ]; then + echo yes + else + echo no + fi +} + +pgc_bav_clean_tree() { # pgc_bav_clean_tree SRCDIR [PG_CONFIG] -> clean|dirty + local _d="${1:-}" _pgc="${2:-}" + [ -n "$_d" ] && [ -d "$_d" ] || { echo dirty; return; } + [ -n "$_pgc" ] && make -C "$_d" clean PG_CONFIG="$_pgc" >/dev/null 2>&1 + find "$_d" \( -name '*.o' -o -name '*.bc' -o -name '*.so' \) -type f -delete 2>/dev/null + [ "$(pgc_bav_tree_has_objects "$_d")" = yes ] && echo dirty || echo clean +} + built=0 for pgc in "${CONFIGS[@]}"; do if [ ! -x "$pgc" ]; then @@ -129,8 +171,14 @@ for pgc in "${CONFIGS[@]}"; do done # Leave no object tree behind from whichever major happened to be last: the next -# build against a different major would link objects compiled for this one. -make -C "$SRCDIR" clean >/dev/null 2>&1 || true +# build against a different major would link objects compiled for this one. The +# verdict is read rather than discarded -- the previous form swallowed its own +# failure and left the tree dirty on any box without a pg_config on PATH (#1219). +if [ "$(pgc_bav_clean_tree "$SRCDIR" "${pgc:-}")" != clean ]; then + echo "build_all_versions: objects remain in $SRCDIR after the final sweep;" >&2 + echo " the next build against another major would link them" >&2 + failed=1 +fi # What was actually compiled, next to what was asked for. Without this line the # verdict below collapses "built five" and "built none" into the same word: a diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 3e491229..1b984d9b 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -514,11 +514,28 @@ harness_selftest 280-the-shared-cluster-config-must-not premise: the cluster-con harness_selftest 280-the-shared-cluster-config-must-not premise: the detector fires on the line that caused #799 15;16;17;18;19 never - harness_selftest 280-the-shared-cluster-config-must-not the per-suite escape hatch PGC_EXTRA_CONF is still applied to the config 15;16;17;18;19 never - harness_selftest 280-the-shared-cluster-config-must-not the shared cluster config sets no pgcolumnar.* GUC 15;16;17;18;19 never - -harness_selftest 290-a-preflight-that-built-nothing a preflight that built nothing does not report PASSED 15;16;17;18;19 never - -harness_selftest 290-a-preflight-that-built-nothing a preflight that built nothing exits non-zero 15;16;17;18;19 never - +harness_selftest 290-a-preflight-that-built-nothing a preflight that built nothing does not report PASSED 15;16;17;18;19 2026-09-24 the zero-build refusal removed, so a preflight that built nothing reports PASSED +harness_selftest 290-a-preflight-that-built-nothing a preflight that built nothing exits non-zero 15;16;17;18;19 2026-09-24 the zero-build refusal removed, so a preflight that built nothing reports PASSED harness_selftest 290-a-preflight-that-built-nothing a preflight that built nothing says how many it built 15;16;17;18;19 never - +harness_selftest 290-a-preflight-that-built-nothing a tree full of objects is reported clean after the sweep 15;16;17;18;19 2026-09-24 the cleaner always answers dirty, whatever the detector says;the find sweep removed, so only make clean is relied on;the sweep looks only in src/, missing objstore/ +harness_selftest 290-a-preflight-that-built-nothing a tree holding objects is detected as holding them 15;16;17;18;19 2026-09-24 the detector blinded, so it never reports an object +harness_selftest 290-a-preflight-that-built-nothing a tree with nothing to remove is clean, not an error 15;16;17;18;19 2026-09-24 the cleaner always answers dirty, whatever the detector says;the find sweep removed, so only make clean is relied on;the sweep looks only in src/, missing objstore/ +harness_selftest 290-a-preflight-that-built-nothing and a directory that is not there holds none 15;16;17;18;19 never - +harness_selftest 290-a-preflight-that-built-nothing and a directory that is not there is dirty rather than silently clean 15;16;17;18;19 2026-09-24 the missing-or-empty directory guard removed +harness_selftest 290-a-preflight-that-built-nothing and clean when it reports none 15;16;17;18;19 2026-09-24 the cleaner always answers dirty, whatever the detector says +harness_selftest 290-a-preflight-that-built-nothing and it left the source alone 15;16;17;18;19 2026-09-24 the sweep loses its name filter and deletes every file it finds +harness_selftest 290-a-preflight-that-built-nothing and no argument at all is dirty rather than sweeping the cwd 15;16;17;18;19 2026-09-24 the missing-or-empty directory guard removed +harness_selftest 290-a-preflight-that-built-nothing and the detector now says the tree holds none 15;16;17;18;19 2026-09-24 the find sweep removed, so only make clean is relied on;the sweep looks only in src/, missing objstore/ +harness_selftest 290-a-preflight-that-built-nothing and the objects are gone, including the ones outside src/ 15;16;17;18;19 2026-09-24 the find sweep removed, so only make clean is relied on;the sweep looks only in src/, missing objstore/ +harness_selftest 290-a-preflight-that-built-nothing and the old swallow-everything form is gone 15;16;17;18;19 2026-09-24 the old swallow-everything make clean put back alongside the new call harness_selftest 290-a-preflight-that-built-nothing premise: and it built none of them 15;16;17;18;19 never - +harness_selftest 290-a-preflight-that-built-nothing premise: and the leftover detector it reads is exposed too 15;16;17;18;19 never - +harness_selftest 290-a-preflight-that-built-nothing premise: the fixture holds objects in both source directories 15;16;17;18;19 never - harness_selftest 290-a-preflight-that-built-nothing premise: the probe run skipped every major 15;16;17;18;19 never - +harness_selftest 290-a-preflight-that-built-nothing premise: the real detector is back, and sees a real object 15;16;17;18;19 2026-09-24 the detector blinded, so it never reports an object +harness_selftest 290-a-preflight-that-built-nothing premise: the tree cleaner is exposed to be judged 15;16;17;18;19 never - +harness_selftest 290-a-preflight-that-built-nothing the cleaner says dirty when the detector reports objects left behind 15;16;17;18;19 2026-09-24 the cleaner never verifies and always answers clean +harness_selftest 290-a-preflight-that-built-nothing the script asks the cleaner rather than merely defining it 15;16;17;18;19 2026-09-24 the call site reverted, so the cleaner is defined but never asked harness_selftest 300-a-test-script-must-be-runnable and every executable script declares one 15;16;17;18;19 never - harness_selftest 300-a-test-script-must-be-runnable and every script a document names exists 15;16;17;18;19 never - harness_selftest 300-a-test-script-must-be-runnable and every script a document names is executable 15;16;17;18;19 never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index c5d24d38..6fc2eb6c 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -835,4 +835,66 @@ suites_not_covered 249 # # The three rows arriving `never` are premises: two `type -t` exposure checks # and one asserting that a rebuild with no pg_config fails. -checks_never_observed_red 1532 +# +# 1532 -> 1534 for #1219, composed onto the main that carries #1213 AND #1248. +# RE-DERIVED BY COUNTING: +# +# awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l -> 1534 +# 1690 rows total, 156 not `never`, 1534 + 156 == 1690 +# +# 17 new rows: 13 arrive dated, 4 arrive `never` +# 2 pre-existing rows flip never -> dated (two of the #809 arms, via K) +# 1532 - 2 + 4 == 1534 +# +# THIS BRANCH HAS NOW CARRIED THREE NUMBERS -- 1508, 1532 and 1534 -- and only the +# last describes a tree that exists. They are not corrections of each other: +# each was counted correctly on the tree it was written against. 1508 was +# against a main with neither #1213 nor #1248; 1532 against one with #1213 +# alone; 1534 against one with both. The delta this branch contributes is +# constant at -2 + 4; the base moved twice. +# +# AND 1532 APPEARED TWICE FOR UNRELATED REASONS. It is what #1248 composed with +# #1213 came to, and it is what THIS branch came to when rebased onto #1213 +# alone -- different rows, different deltas, same total. Had #1248 not landed +# first, this file would now read 1532 and be right; it reads 1534 and is right. +# Two trees agreeing on a number is not evidence either is correct. Recount on +# the tree you have. +# +# THE DECOMPOSITION ABOVE WAS WRONG IN AN EARLIER VERSION of this file, and the +# way it was wrong is worth keeping. It said "9 arrive dated, 8 arrive never" +# and "1506 - 2 + 8 == 1508". The split is 13/4 and 1506 - 2 + 8 is 1512: TWO +# COMPENSATING ERRORS REACHING THE RIGHT ANSWER. Nothing failed, because the +# shipped number was right -- but a reader checking the working got 1512 and +# could not tell whether to distrust the count or the comment. The cause: the +# diff shows 19 `+` lines, which is 17 new rows PLUS the 2 pre-existing rows +# that changed, and counting `+` lines as new rows produced the 9/8. Caught by +# @jdatcmd. +# +# ELEVEN MUTATIONS. Three are the point: +# +# C the cleaner never verifies and always answers `clean` +# -> reddened NOTHING, twice: inline, and again after the detector was +# split out. Its verdict is only load-bearing when the SWEEP FAILS, and +# that cannot be staged as root -- chmod does not stop an unlink, and +# anything `find -type f -delete` removes, the `-type f` re-check cannot +# see either. FIXED BY STUBBING WHAT THE CODE CONSULTS rather than by a +# cleverer fixture: the cleaner asks the detector, so the detector is +# stubbed to `yes` and the cleaner must answer `dirty`. The real one is +# restored and the restore ASSERTED against a freshly created object, +# because an unrestored stub makes every later arm measure a function +# this part wrote. +# +# G the detector's own directory guard removed -> reddens NOTHING, and that +# is correct rather than a gap. Measured with and without it over +# '/nonexistent/nope', '' and '/tmp': identical answers, because `find` on +# a missing path already yields nothing and falls through to `no`. A +# REDUNDANT CONDITION in #1236's taxonomy, not an uncovered one. +# +# A and B redden the SAME four arms, so this set catches "no sweep at all" and +# "sweep only src/" without distinguishing them. +# +# TWO NON-PREMISE ARMS REMAIN `never` HERE, both named: "and a directory that is +# not there holds none" (G, redundant) and "a preflight that built nothing says +# how many it built", which predates this change -- K reddens its two siblings +# but leaves the `built 0 of 3` line intact. +checks_never_observed_red 1534 diff --git a/test/selftest/290-a-preflight-that-built-nothing.sh b/test/selftest/290-a-preflight-that-built-nothing.sh index 3b953205..4d745113 100644 --- a/test/selftest/290-a-preflight-that-built-nothing.sh +++ b/test/selftest/290-a-preflight-that-built-nothing.sh @@ -28,6 +28,112 @@ _bav_src="$PGC_TESTDIR/build_all_versions.sh" +# ---- and it must leave no object tree behind, on a box with no pg_config ----- +# +# #1219. The script's LAST act is a clean, and its comment says why: "Leave no +# object tree behind from whichever major happened to be last: the next build +# against a different major would link objects compiled for this one." It was +# written as +# +# make -C "$SRCDIR" clean >/dev/null 2>&1 || true +# +# with no PG_CONFIG and its failure swallowed. PGXS resolves `pg_config` from +# PATH, so on a box with no packaged one this make fails, the `|| true` eats it, +# and the tree keeps the LAST major's objects while the script still prints +# PASSED. Measured, same tree, pg18a then pg19a: +# +# normal PATH built 2 of 2 PASSED objects left: 0 +# PATH with no pg_config built 2 of 2 PASSED objects left: 36 +# +# The consequence is bounded -- #1221's DWARF provenance catches the foreign +# objects on the next suite and cleans them -- so this costs a rebuild rather +# than a wrong install. What it does not cost is nothing, and the comment +# promises something it does not always do. +# +# THE SWEEP DOES NOT DEPEND ON pg_config AT ALL, and that is the fix rather than +# passing one: objects live in src/ AND objstore/, a `make clean` needs PGXS +# loaded to do anything, and a clean whose failure is swallowed cannot be +# distinguished from one that worked. So it removes by find and then VERIFIES, +# and the verdict is a value this part can judge. +eval "$(sed -n '/^pgc_bav_tree_has_objects()/,/^}/p' "$PGC_TESTDIR/build_all_versions.sh")" +eval "$(sed -n '/^pgc_bav_clean_tree()/,/^}/p' "$PGC_TESTDIR/build_all_versions.sh")" +check_text "premise: the tree cleaner is exposed to be judged" \ + "$(type -t pgc_bav_clean_tree)" "function" +check_text "premise: and the leftover detector it reads is exposed too" \ + "$(type -t pgc_bav_tree_has_objects)" "function" + +_bav_ct="$(mktemp -d)" +mkdir -p "$_bav_ct/src" "$_bav_ct/objstore" +: > "$_bav_ct/src/columnar.o" +: > "$_bav_ct/src/columnar.bc" +: > "$_bav_ct/objstore/columnar_objstore_module.o" +: > "$_bav_ct/pgcolumnar.so" +: > "$_bav_ct/src/keep.c" + +# THE FIXTURE MUST ACTUALLY HOLD OBJECTS, or every arm below passes by measuring +# an empty directory. +check_num "premise: the fixture holds objects in both source directories" \ + "$(find "$_bav_ct" \( -name '*.o' -o -name '*.bc' -o -name '*.so' \) | wc -l)" "4" + +# THE DETECTOR, JUDGED BEFORE ANYTHING IS SWEPT. Folded inside the cleaner it +# was unreachable -- a cleaner that always answered `clean` reddened none of +# these arms, measured. Driven directly it has a killer. +check_text "a tree holding objects is detected as holding them" \ + "$(pgc_bav_tree_has_objects "$_bav_ct")" "yes" +check_text "and a directory that is not there holds none" \ + "$(pgc_bav_tree_has_objects "$_bav_ct/nope")" "no" + +check_text "a tree full of objects is reported clean after the sweep" \ + "$(pgc_bav_clean_tree "$_bav_ct")" "clean" +check_num "and the objects are gone, including the ones outside src/" \ + "$(find "$_bav_ct" \( -name '*.o' -o -name '*.bc' -o -name '*.so' \) | wc -l)" "0" +check_num "and it left the source alone" \ + "$([ -f "$_bav_ct/src/keep.c" ] && echo 1 || echo 0)" "1" +check_text "and the detector now says the tree holds none" \ + "$(pgc_bav_tree_has_objects "$_bav_ct")" "no" + +# AND THE CLEANER MUST REPORT WHAT THE DETECTOR TELLS IT, which needs the +# detector STUBBED rather than a fixture. A sweep that fails is what makes the +# cleaner's verdict load-bearing, and it cannot be staged here: the suite runs as +# root, so chmod does not stop an unlink, and anything `find -type f -delete` can +# remove the `-type f` re-check cannot see either. Measured before this arm +# existed -- a cleaner hard-wired to `echo clean` passed every other arm in this +# part. Stubbing the one function it consults is what gives that mutation a +# killer. +pgc_bav_tree_has_objects() { echo yes; } +check_text "the cleaner says dirty when the detector reports objects left behind" \ + "$(pgc_bav_clean_tree "$_bav_ct")" "dirty" +pgc_bav_tree_has_objects() { echo no; } +check_text "and clean when it reports none" \ + "$(pgc_bav_clean_tree "$_bav_ct")" "clean" + +# THE STUB IS PUT BACK, and that is asserted rather than assumed: an unrestored +# stub makes every later arm measure a function this part wrote. +eval "$(sed -n '/^pgc_bav_tree_has_objects()/,/^}/p' "$PGC_TESTDIR/build_all_versions.sh")" +: > "$_bav_ct/src/restored.o" +check_text "premise: the real detector is back, and sees a real object" \ + "$(pgc_bav_tree_has_objects "$_bav_ct")" "yes" +rm -f "$_bav_ct/src/restored.o" + +# THE #1219 CASE ITSELF: no pg_config is given and none need be. +check_text "a tree with nothing to remove is clean, not an error" \ + "$(pgc_bav_clean_tree "$_bav_ct")" "clean" +check_text "and a directory that is not there is dirty rather than silently clean" \ + "$(pgc_bav_clean_tree "$_bav_ct/nope")" "dirty" +check_text "and no argument at all is dirty rather than sweeping the cwd" \ + "$(pgc_bav_clean_tree)" "dirty" +rm -rf "$_bav_ct" + +# AND THE SCRIPT MUST ASK IT, not merely define it. A correct cleaner that +# nothing calls leaves the defect exactly where it was, and the arms above would +# all still pass. Comments are stripped first: this file's own prose quotes the +# call, and part 190 already paid for that lesson once. +_bav_code="$(sed 's/#.*//' "$PGC_TESTDIR/build_all_versions.sh")" +check_num "the script asks the cleaner rather than merely defining it" \ + "$(printf '%s\n' "$_bav_code" | grep -c 'pgc_bav_clean_tree "')" "1" +check_num "and the old swallow-everything form is gone" \ + "$(printf '%s\n' "$_bav_code" | grep -cE 'make -C "\$SRCDIR" clean >/dev/null 2>&1 \|\| true')" "0" + # Run a COPY, in a scratch tree, because the real script ends with # `make -C "$SRCDIR" clean` and SRCDIR is derived from the script's own # location. Run in place, this selftest would wipe the object tree of the very