Sync pgxntool to 2.3.0 - #58
Conversation
a6d3287 Release 2.3.0 (#89) 1a7ee20 CLAUDE.md: fix pre-existing documentation-accuracy errors (#88) 35fd187 Fix DATA wildcard skipping historical full-install version scripts (Postgres-Extensions#48) (#80) f9ce2a2 Fix installcheck running before install; make test now fails on regressions (#83) 3ceb930 Rename PGTLE_VERSION to PGXNTOOL_PGTLE_VERSION (#85) 2de8429 claude-code-review.yml: add missing --comment flag (#84) 957bdf6 Restrict @claude mention trigger to a trusted account (#82) b67064b Release 2.2.0 (#77) d949d6d Fix stale README/CLAUDE.md docs; rename internal-only test shim (#76) 6d7822c ci: grant actions:write so bun cache save succeeds (#75) 2738f59 README.asc: fix stale docs, document undocumented API surface (#74) d1d3bee Add make pgxntool-version to print the embedded pgxntool version (#73) 493a0f8 Fix parallel-build SQL corruption, remote hardcoding, and add safety checks (Postgres-Extensions#61) 3b0e3de Fix make pgtle PGTLE_VERSION=X being silently ignored (Postgres-Extensions#66) 21b35ef ci: skip paired-test-PR requirement for doc-only PRs (#71) 55efd03 Rename EXTENSION_VERSION_FILES to EXTENSION__CURRENT_VERSION__FILES (#69) 9d0a856 Fix pgtle.sh rejecting non-numeric version aliases like 'stable' (Postgres-Extensions#57) (Postgres-Extensions#63) 9c2f75f Docs: versioned SQL file tracking tradeoffs and update-testing pattern (Postgres-Extensions#60) c38cf2a ci: never check out fork PR head in claude-code-review pull_request_target job (#70) aab8f2b pgtle_versions.md: fix wrong uninstall_extension() version claim (Postgres-Extensions#59) e2c9174 README.asc: make tag creates a git tag, not a branch (Postgres-Extensions#58) eb84bc6 Stamp 2.1.0 9fbe6f4 Fix results ordering, control file whitespace, ENABLE_* override, debug levels (Postgres-Extensions#31) 88bb4f2 Add Claude Code GitHub Actions workflows (Postgres-Extensions#41) af5bbbb ci: pass repo owners to run-tests.yml for fork-account matching (Postgres-Extensions#40) c7928af Fix repo-root guard to work inside a git worktree (Postgres-Extensions#39) b062fca ci: point reusable test workflow at @master b6cdbfd Add CI workflows and multi-session PR guard (Postgres-Extensions#33) 1ba0987 Stamp 2.0.3 1931cbe Fix pgxntool-sync remote and make it runnable without make (Postgres-Extensions#37) 8176304 Stamp 2.0.2 3e142ab Fix parse_control_file: remove comments before stripping quotes (Postgres-Extensions#27) cacc301 Stamp 2.0.1 bf1db6b Fix bash 3.2 / Linux compatibility issues (Postgres-Extensions#26) 62d0fcb Fix broken ifeq for --load-language=plpgsql on PG < 13 (Postgres-Extensions#24) 121f0b3 Stamp 2.0.0 ad3ca7e Remove .source support; add test/install, test/build, and verify-results (Postgres-Extensions#18) c010cf8 Fix bash 3.2 compatibility (Postgres-Extensions#23) abeb9d3 Remove .source file support from pg_regress integration (Postgres-Extensions#22) 08c1879 Stamp 1.1.2 6e0dad2 Fix double --dbname bug that defeated unique test database names 639756c Stamp 1.1.1 6ba3176 Fix pg_tle exception handler and empty upgrade files (Postgres-Extensions#15) 3b8cb2a Stamp 1.1.0 550a901 Remove commit.md (maintained in pgxntool-test) d73ca93 Add unique test database names to prevent conflicts (Postgres-Extensions#13) 9b344be Add update-setup-files.sh for 3-way merging after pgxntool-sync (#12) ab7f6e2 Stamp 1.0.0 3a571ba Add pg_tle support and modernize test infrastructure (#11) b96ea6d Add support for Claude code; build and doc improvements (#9) e9c24de Fix pg_regress on versions > 12 (#5) git-subtree-dir: pgxntool git-subtree-split: a6d3287e879c45aa0c9a32c6f9513312396c8915
…cratch DESTDIR pgxntool 2.3.0 fixed issue #79 (installcheck running before install) by making installcheck unconditionally depend on install in base.mk -- with no opt-out. That reintroduces the problem TEST_EXISTING_DEPLOY=pgtle mode exists to avoid: calling installcheck directly now also runs install, which would write cat_tools.control to the real extension directory even though the database under test was deployed via pg_tle, tripping bin/assert_fs_clean. Redirect install's filesystem writes to a throwaway DESTDIR instead of trying to skip it: existing mode's pg_regress run never exercises install's output (the extension is already present in the database via pg_tle), so where install's side effect lands doesn't matter, as long as it isn't the real extension directory. Verified locally: real extension directory's cat_tools files are byte-for-byte unchanged (compared by checksum) after a full TEST_EXISTING_DEPLOY=pgtle run-suite invocation, and all 14 tests still pass.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ater Points at the specific lines to remove once pgxntool grows a real install-skip override (pgxntool#55), not just the top-of-file doc comment.
Code review3 issues found. (Note: inline line comments were not available in this environment, so findings are consolidated here with direct links to the relevant lines.) 1. Lines 258 to 265 in 304b4f3 The new installcheck: pgtap
pgtap: $(DESTDIR)$(datadir)/extension/pgtap.control
$(DESTDIR)$(datadir)/extension/pgtap.control:
pgxn install pgtap --sudo
Possible fix: seed 2. Bare issue reference violates CLAUDE.md Lines 39 to 43 in 304b4f3 Line 41 reads Suggested fix: change 3. Scratch Lines 258 to 265 in 304b4f3
Suggested fix: add a trap right after |
… rule for historical refs Addresses code review on Postgres-Extensions#58 (3 findings): 1. base.mk's pgtap target checks $(DESTDIR)$(datadir)/extension/pgtap.control but its recipe (pgxn install pgtap --sudo) ignores DESTDIR entirely, always installing for real. Against our empty scratch DESTDIR that stub can never exist, so Make considered pgtap stale and ran a real pgxn install pgtap --sudo on every pg_tle-mode call -- hitting network/sudo and writing to the actual system extension directory, defeating the whole point of this workaround for that one file. Fixed by seeding a stub pgtap.control under the scratch DESTDIR before calling installcheck. Verified: pgtap install output (previously a full rebuild) no longer appears, and a checksum diff of the real extension directory (pgtap + cat_tools files) is empty before/after a full pg_tle-mode run-suite invocation. 2. The DESTDIR temp dir was only cleaned up by a trailing rm -rf, which a genuine test failure (a routine outcome here, not an edge case) skips entirely under set -euo pipefail. Switched to an EXIT trap and made pgtle_destdir non-local so the trap can still reference it after run_suite returns (a RETURN trap wouldn't fire on an errexit-triggered exit either). Verified: a deliberately failing test still exits non-zero AND still cleans up the scratch dir. 3. Relaxed CLAUDE.md's PR/issue URL-reference rule: a full URL is still required for anything a reader might still need to act on (an open TODO, a workaround to revisit later), but a purely historical reference (past context, already resolved, nothing left to do) can use a bare #NN. The (issue #79) reference this review flagged is exactly that case -- pgxntool already fixed #79, and the surrounding comment is explaining why past code looks the way it does, not asking anyone to look anything up.
…iterally Double-quote the trap command so $pgtle_destdir expands immediately when trap runs, capturing the actual path as a literal string. The trap then no longer references the variable at all by the time it fires at EXIT, so it can't be affected by whatever value (empty, reassigned, or otherwise) that variable happens to hold later -- the previous single-quoted version re-evaluated $pgtle_destdir lazily at exit time, which is fragile even though the specific failure mode here (an empty/unset value) is harmless (rm -rf "" just errors, it doesn't resolve to anything destructive). Verified: trap -p EXIT shows the literal path baked in (confirmed by clearing the variable immediately after registering the trap and observing cleanup still happens correctly), and the full pg_tle-mode simulation (happy path and a deliberately-failing path) still passes/cleans up identically to before.
Pulls the latest pgxntool release (2.3.0, up from 2.2.0) via
git subtree pull. This release fixes both issues filed from our 2.2.0 sync: #78 (PGTLE_VERSIONcollision, renamed upstream toPGXNTOOL_PGTLE_VERSION) and #79 (check-stale-expectedrunning beforeinstall, fixed by makinginstallcheckunconditionally depend oninstallinbase.mk).That #79 fix reintroduces the exact problem
bin/test_existing'sTEST_EXISTING_DEPLOY=pgtlemode exists to avoid: callinginstallcheckdirectly now also runsinstall, writingcat_tools.controlto the real extension directory even though the database under test was deployed via pg_tle -- trippingbin/assert_fs_clean. Since pgxntool no longer offers a way to skipinstall(that's a separate, still-open request, pgxntool#55), this redirectsinstall's filesystem writes to a throwawayDESTDIRinstead: existing mode's suite never exercisesinstall's output (the extension is already present via pg_tle), so where it writes doesn't matter as long as it isn't the real extension directory. Verified locally against a real database: the real extension directory'scat_toolsfiles are byte-for-byte unchanged (by checksum) after a fullTEST_EXISTING_DEPLOY=pgtle run-suiteinvocation, and all 14 tests still pass.No other cat_tools-side changes were needed for this sync --
EXTENSION_VERSION_FILES/PGTLE_VERSIONaren't referenced anywhere in our ownMakefile/sql.mkbeyond what 2.2.0 already required. A few workarounds in our code are now redundant given what 2.3.0 fixes upstream; those are being cleaned up separately rather than bundled into this sync.