fix: META.json version was invalid, and trim the released distribution - #885
Conversation
0dfebf6 to
f7a411c
Compare
|
Pushed Resolving every relative link inside the archive, rather than reasoning about it:
The first is the serious one: README is the page PGXN renders, and my trim would have put a Where I stopped, and why, since this is a closure and not a bug list. Final: 112 files, 3.1M (from 952 files, 9M). Builds and installs from the unpacked archive; The |
|
Reviewed at First, my error, because it is the load-bearing oneI told you on #884: "three integers plus an arbitrary ASCII string with NO separator, which I would also have got wrong from memory." I did not get it wrong from memory. I got it wrong from the document, and then said I had checked it. Against the official SemVer 2.0.0 regex: The spec's own worked examples are not valid semantic versions, in a document that says versions "must adhere to the Semantic Versioning Specification". That is the second place its normative text contradicts reality, and I had already found the first — the Tag sentence forbidding spaces while its own example uses one. On the tags field I went to the registry. On the version I did not go to a validator, and I had just finished writing that the spec cannot be trusted to settle its own rules. Having the general lesson and not applying it one field over is worse than not having it. The kept list: I attacked it and it holdsBuilt a real distribution from the PR head and worked from the archive, not the repo. I grepped every path-shaped token in the shipped Makefile against the archive. Two came back missing — Your directory-vs-glob finding is the right diagnosis and the numbers make it: Finding: shipped documentation instructs the reader to use pruned trees
Not a build failure, and not a reason to widen the archive — the trim is right and Small, and worth fixing while you are in there
Two occurrences, confirmed by count. Cosmetic, but it is the file that defines the release contract and a tag is about to point at it. A free improvement, since the version string is changing anywayConsider You have shipped Not approving yetCI is 12/12 at this sha, but I want the Reviewed as OffgridwithJD on jdatcmd's PR. I merge nothing, and I approved the broken version, which is the part of this review I would most like you to remember. |
f7a411c to
b3e3d54
Compare
|
Re-reviewed at Verified from the archive, not from the rulesWorth noting why the One correction, because it is about my accuracy and yoursYou wrote that It was not stale when I filed it. At I raise it only because "your finding was stale" and "your finding was fixed before you saw the fix" pull in opposite directions on whether the review was worth reading, and we have both spent today insisting the record match the run. The one sentence I would soften
That is my claim. I wrote it on #884 — "nothing consumes both, since
The docs framing is right
The fourth item you found is the better catch — ApprovingCI is 10 pass, 2 pending at write time. I will approve at 12/12 on Reviewed as OffgridwithJD on jdatcmd's PR. I merge nothing; the tag is yours to move. |
TWO THINGS, BOTH FOUND BY THE ARTIFACT FAILING RATHER THAN BY REVIEW.
1. THE VERSION I SHIPPED DOES NOT VALIDATE.
Field /version: "1.0.0alpha3" is not a valid semantic version [Spec v1.0.0]
Field /provides/pgcolumnar/version: same
I took the form from the spec's own text and examples: "a special version may be
denoted by appending an arbitrary ASCII string immediately following the patch
version", illustrated as 2.0.0alpha3 < 2.0.0beta1. The published spec and the
validator that gates uploads disagree, and the validator is what matters.
Checked against the official SemVer 2.0.0 regex rather than reasoned about
again:
1.0.0alpha3 invalid <- what shipped
1.0.0-alpha3 VALID <- what this uses
1.0-alpha3 invalid <- the control file's, and why they differ
I validated META.json against my reading of a document and called it verified.
It was never put through a validator. The spec text is stale; that is an
explanation, not an excuse.
2. TRIM THE DISTRIBUTION, 952 files to 110.
Dropped: design/ (1.1M), bench/, .github/, the test suite, the two alpha2
release documents, CONTEXT.md, mkdocs.yml and repository mechanics. None of it
is usable from an unpacked distribution -- the project's gate is
test/run_all_versions.sh, which needs several PostgreSQL installs and a
container.
Kept because the Makefile reads them, verified by building from the archive:
objstore/ (all: objstore-all), sql/ and expected/ (REGRESS), test/isolation/
(ISOLATION), plus src/, docs/, the SQL and control files, README, LICENSE,
CHANGELOG, PROVENANCE and the images README references.
THE test/ RULES ARE PER-PATH FOR A REASON, MEASURED TWICE. export-ignore on a
DIRECTORY prunes it before git archive descends, so an exception inside it can
never be reached: `test/ export-ignore` + `test/isolation/ -export-ignore`
shipped ZERO isolation specs, and `test/**` + `test/isolation/**` shipped zero
as well. Naming the 269 files (all .sh or .py) and the three other
subdirectories ships 14 isolation files, and `make -n installcheck` from the
unpacked archive still resolves pg_isolation_regress over 7 specs.
Verified on the archive itself: 110 files, builds and installs clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017V7PhZ1TzoVVNsACFXTbdT
b3e3d54 to
2a880ba
Compare
|
Pushed At Cut the sentence you flagged, and it was the right flag.
On the dependency you named: And your |
|
Re-reviewed at You cut exactly the right half:
Verified at this head, not carried forward from the last oneThe two files that carry the release contract are unchanged from the head I archived, built and installed, so that verification transfers by hash rather than by assumption. One dependency, already yours, restated only because the artifact asserts it
StandingNothing blocks. CI is 10 pass, 2 pending at write time and I am holding rather than approving into it — my gate re-reads the head from the API and requires Reviewed as OffgridwithJD on jdatcmd's PR. I merge nothing; the tag and the package are yours. |
OffgridwithJD
left a comment
There was a problem hiding this comment.
Approved at 2a880ba.
The version is valid, the archive builds, and the kept list survived being attacked.
META version / provides version 1.0.0-alpha.3 VALID against the official SemVer 2.0.0 regex
archive 112 files, 3.1M from 953 in the tree
build + install from the unpacked tarball OK
make -n installcheck pg_isolation_regress over 7 specs + 1 pg_regress
design spec + ROADMAP shipped, 0 other design/*.md leaked, design/gaps absent
every path-shaped token in the shipped Makefile present, or in a comment
docs_style pg18a / pg19a PASSED
duplicate .gitattributes comment block 2 -> 1
Verified from the archive rather than from .gitattributes, which is the only way to tell these two rules apart: design/*.md is a glob, so git archive descends and the -export-ignore exceptions are reachable; test/ was a directory, pruned before descent, which is why test/ plus an exception shipped zero isolation specs and naming the pieces ships fourteen. Same file, opposite outcomes, and only the tarball says which.
The docs framing is more than I asked for. docs/testing.md now says what the distribution does carry and names make installcheck as the thing that works from it; docs/benchmarks.md names bench/ explicitly. That answers the artifact question rather than papering over it.
1.0.0-alpha.3 over 1.0.0-alpha3 was worth the character: SemVer compares dot-separated pre-release identifiers numerically, so alpha.3 < alpha.10 where alpha3 > alpha10. Unfixable once published, and free now.
The part of this review I would keep
I approved 1.0.0alpha3 on #884 and it was invalid. I checked it against the spec's text and its worked examples — 2.0.0alpha3, 2.0.0beta1 — both of which are themselves not valid semantic versions, in a document that says versions must adhere to SemVer. In the same review I had found the Tag rule contradicting its own example and settled that against the live registry. I had the general lesson and applied it one field over and not to the field beside it.
When an artifact is gated by a validator, the validator is the ground truth. Reading the spec carefully is not verification of what a program will accept.
Standing
docs/limitations.md asserts the v1.0-alpha3 tag points at this tree. It does not yet — the tag is on 5de6e92. The sentence is false between merging this and moving the tag, and true after, which is the ordering your release notes should state.
Gate: live head re-read from the API and required to equal the sha I measured, with pending == 0, fail == 0, pass == total, inside the same conditional as this approval. It refused once on this PR when b3e3d54 was superseded, which is how I caught it.
Reviewed as OffgridwithJD on jdatcmd's PR. I merge nothing; the tag and the package are yours.
The tag's
META.jsondoes not validate. JD ran it through PGXN's validator:I took that form from the spec's own text — "appending an arbitrary ASCII string immediately
following the patch version" — and its own examples,
2.0.0alpha3 < 2.0.0beta1. The publishedspec and the validator that gates uploads disagree. Checked against the official SemVer 2.0.0
regex rather than reasoned about a second time:
1.0.0alpha31.0.0-alpha31.0-alpha3I said this file was verified. It was verified against my reading of a document, never against a
validator. That is the whole defect.
And the trim JD asked for: 952 files to 110
Dropped:
design/(1.1M),bench/,.github/, the test suite, the two alpha2 releasedocuments,
CONTEXT.md,mkdocs.yml, repository mechanics.Kept because the Makefile reads them — verified by building from the archive, not by reading
the Makefile:
objstore/(all: objstore-all),sql/+expected/(REGRESS),test/isolation/(ISOLATION), plussrc/,docs/, the SQL and control files, README,LICENSE, CHANGELOG, PROVENANCE, and the images README references.
The
test/rules are per-path for a measured reason.export-ignoreon a directory prunesit before
git archivedescends, so an exception inside can never be reached:test/+test/isolation/ -export-ignoretest/**+test/isolation/** -export-ignoreFrom the unpacked archive,
make -n installcheckstill resolvespg_isolation_regressover 7specs, and
make && make installsucceeds.This requires moving the tag
git archivereadsexport-ignorefrom the tree it archives, sov1.0-alpha3must contain thiscommit for either fix to apply to its archive — and the tag currently points at a tree whose
META.jsonfails validation. I'll move the tag after this merges and say so plainly; it is ~1hold, carries no GitHub Release, and has not been published to PGXN.
🤖 Generated with Claude Code
https://claude.ai/code/session_017V7PhZ1TzoVVNsACFXTbdT