test: print the .so under test, and name what PGC_SKIP_BUILD actually skips (#508 follow-up) - #513
Conversation
… skips
Three separate defects in one session were a suite reporting checks against a
binary nobody had just built:
- a compile error the harness did not check, which reported 19 green checks
against a source file containing invalid C (commandprompt#508);
- a PGC_SKIP_BUILD=1 run that skipped the INSTALL, not just the build, and
exercised a guard-removed leftover from an earlier removal proof -- three
suites failing for a reason that had nothing to do with the change;
- objects from one major linked into another's .so, which surfaces as a
cluster that will not start behind a message naming nothing.
Every one produced a plausible PASS/FAIL list. Every one is one line of md5sum
away from being obvious. So pgc_setup now prints that line on every run, whether
or not anything looks wrong -- the point is precisely that nothing does.
It also makes a red-on-change proof self-evidencing. Two arms reporting the same
hash have proved nothing whatever their check counts say, and that is currently
something each person has to remember to verify by hand.
PGC_SKIP_BUILD is named in the same change because the variable is not what it
says: it reads as "skip the build" and means "skip the build AND the install, and
test whatever is already installed". That is correct for the matrix, which
installs once per major before setting it, and a trap for a person who has just
edited source and run make by hand -- which is exactly how the second defect
above happened.
harness_selftest gains two checks. The first is that the line exists. The second
is the one with teeth: it compares what is INSTALLED against what was just BUILT,
using the build tree as an independent source rather than recomputing the
installed hash the same way twice.
Proved by reproducing the original incident rather than by deleting the check:
build a genuinely different binary, do not install it, run with
PGC_SKIP_BUILD=1.
installed 8b58d7fdcb0d, built 7564d4f138ed -> FAIL, as it should
The first attempt at that proof appended a comment to a source file and rebuilt.
The binary was byte-identical, both hashes matched, and the check passed --
which the fingerprint line itself is what revealed. A perturbation that does not
perturb is not a proof, and this one says so out loud.
Refs commandprompt#508
jdatcmd
left a comment
There was a problem hiding this comment.
Approved. Read it rather than approving the thing I asked for on the strength of having asked for it — that is the failure mode where a reviewer rubber-stamps their own suggestion.
The design is right and better than what I specified. Comparing installed against built, using the build tree as an independent source, can actually disagree; recomputing the installed hash two ways would only have proved md5sum is deterministic. And printing the line on every run rather than only on failure is the part that makes it useful — the defect it catches is invisible in a PASS/FAIL list, so a diagnostic that only appears when something already looks wrong would not have caught any of the three instances.
Naming what PGC_SKIP_BUILD actually skips is the half I would have under-weighted. "Not building AND NOT INSTALLING; whatever is already installed is what these checks measure" is the sentence that would have stopped me — I read the variable and reasoned about the build.
One non-blocking edge
harness_selftest under PGC_SKIP_BUILD=1 in a tree that was never built: _so_built is empty, so the check reports no (installed <hash>, built <none>) and reds.
Under the matrix that cannot happen — the runner builds into the builddir and only then sets the variable, so $PGC_SRCDIR/pgcolumnar.so is right there. It is reachable only by a person running the self-test standalone with PGC_SKIP_BUILD=1 from a clean checkout, which is a legitimate "test what is already installed" flow.
The honest outcome there is cannot tell, and this repository has a shape for that which is neither a pass nor a fail. A red is defensible — the harness genuinely cannot verify what it claims — but it fails as though the install were broken when nothing is. Your call whether it is worth distinguishing; I would not hold the PR for it.
The residual gap worth stating somewhere, since the check's name implies more than it can deliver: it detects install skew, not staleness. Built and installed can agree and both be old. That is still an enormous improvement over what we had, and it is exactly what caught me twice.
Follow-up to #508, which fixed the harness reporting checks after a build that
failed. This makes the same class visible rather than fixing one instance of it.
pgc_setupnow prints one line on every run:Why a line nobody asked for
Three separate defects in a single session were a suite reporting checks against
a binary nobody had just built:
against a source file containing invalid C (fix: a suite must not report checks against the previously installed .so #508);
PGC_SKIP_BUILD=1run that skipped the install, not just the build, andexercised a guard-removed leftover from an earlier removal proof — three suites
failing for a reason unrelated to the change;
.so, which surfaces as a clusterthat will not start behind a message naming nothing.
Every one produced a plausible PASS/FAIL list. Every one is one line of
md5sumaway from being obvious. The point is that it prints when nothing looks wrong,
because that is the state all three of those were in.
It also makes a red-on-change proof self-evidencing. Two arms reporting the same
hash have proved nothing whatever their check counts say, and today that is
something each of us has to remember to verify by hand.
PGC_SKIP_BUILD is named in the same change
The variable is not what it says. It reads as "skip the build" and means "skip
the build and the install, and test whatever is already installed" — correct
for the matrix, which installs once per major before setting it, and a trap for a
person who has just edited source and run
makeby hand. That is exactly how thesecond defect above happened, so the harness now says it out loud.
Which removals were tried, and which one proved nothing
harness_selftestgains two checks: that the line exists, and — the one withteeth — that what is installed matches what was just built, using the
build tree as an independent source rather than recomputing the installed hash
the same way twice. Recomputing it two ways proves only that
md5sumisdeterministic.
Proved by reproducing the original incident rather than by deleting the check:
And the removal that proved nothing, which belongs here rather than buried in a
commit message. My first attempt appended a C comment to a source file and
rebuilt, expecting a different binary. The hashes were identical — comments do not
survive to the object — the check passed, and it would have shipped labelled
"proved by removal". The line being justified is what revealed it.
The general form, which is worth more than this patch: a removal proof must
remove something the compiler, the planner or the runtime can observe. A
comment; a fixture where two cases coincide; a guard nothing reaches. In each the
"before" arm equals the "after" by construction, and the check passes in both
while carrying the authority of a real proof.
Gate
Full matrix on 18 and 19.
harness_selftest=PASSon both majors and not amongeither job's skips.
PG19 reports
temporal=FAILin the container this ran in, which is its missingbtree_gistand which unmodifiedmainfails identically there — measured, andwritten up on #505. Happy to re-run on the bench host before merge if you would
rather see it without that caveat.