Fix parallel-build SQL corruption, remote hardcoding, and add safety checks#61
Open
jnasbyupgrade wants to merge 5 commits into
Open
Fix parallel-build SQL corruption, remote hardcoding, and add safety checks#61jnasbyupgrade wants to merge 5 commits into
jnasbyupgrade wants to merge 5 commits into
Conversation
…checks (Postgres-Extensions#7, Postgres-Extensions#14, Postgres-Extensions#19, Postgres-Extensions#28, Postgres-Extensions#50, Postgres-Extensions#53) - Fix a real data-corruption bug: versioned SQL file generation used `>` truncate followed by `>>` append across two recipe lines, so a rule that fires twice in one `make` run (e.g. reached via two dependency chains under parallel make) silently doubled the file's content. Now written atomically via a single subshell redirect (Postgres-Extensions#19) - `make test` never failed on a stale `test/expected/` file left behind after renaming/removing a test; new `check-stale-expected` target (run as part of `TEST_DEPS`) catches orphaned `.out` files in `test/sql`/`test/expected` and `test/build`/`test/build/expected` (Postgres-Extensions#14) - Tagging targets (`tag`/`rmtag`/`forcetag`/`dist`) hardcoded the `origin` remote, silently re-tagging the wrong repo for maintainers whose `origin` is a personal fork; now configurable via `PGXN_REMOTE ?= origin` (Postgres-Extensions#53) - `EXTENSION_VERSION_FILES` renamed to `EXTENSION_CURRENT_VERSION_FILES` — the old name implied all version files for an extension, but it only ever holds the current/most-recent file per extension (Postgres-Extensions#28) - `base.mk` had no include guard, so double-inclusion (e.g. an extension's own `.mk` module including it alongside the Makefile's own include) redefined every target with overriding-recipe warnings; now wrapped in an `ifndef`/`endif` guard (Postgres-Extensions#50) - `make clean`'s `EXTRA_CLEAN` targeted a nonexistent top-level `results/` instead of the real `$(TESTOUT)/results/` (Postgres-Extensions#7) Verified with a full test suite run (206/206 passing, 0 skips) against the dedicated pg_tle-preloaded cluster. Changes only in pgxntool. No related changes in pgxntool-test. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
|
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:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…ive, document all override variables, add HISTORY entries - check-stale-expected (Postgres-Extensions#14) false-flagged pg_regress's alternate expected-output files (test_0.out .. test_9.out -- a real, widely-used convention: pg_tle's own test suite ships pg_tle_perms_1.out/pg_tle_versions_1.out, as do pgtap/pglogical). Now strips a trailing _N before checking for a matching .sql. - Consolidate EXTRA_CLEAN's filter-out and += into a single assignment (no reason they needed to be separate lines) - Add a `Configuration Variables` section to README.asc documenting every override variable in base.mk (PGXN_REMOTE, ASCIIDOC, PG_CONFIG, TESTDIR, TESTOUT, PGXNTOOL_VERIFY_RESULTS_MODE, and the three PGXNTOOL_ENABLE_* flags, PGXNTOOL_NO_PGXS_INCLUDE) -- previously only scattered partially across the doc, with several undocumented entirely - Add HISTORY.asc entries for the user-visible changes in this PR, most importantly the EXTENSION_VERSION_FILES rename since it can break a Makefile that references the old name directly Changes only in pgxntool. Related changes in pgxntool-test: - New test/standard/*.bats coverage for Postgres-Extensions#7, Postgres-Extensions#14, Postgres-Extensions#19, Postgres-Extensions#50, Postgres-Extensions#53 (see paired pgxntool-test PR) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
… it after pg_regress, rename to EXTENSION__CURRENT_VERSION__FILES - check-stale-expected's logic moved to check-stale-expected.sh (enough code to warrant a real script over an inline recipe). The base/alt filename computation no longer tracks a separate found/not-found flag -- $base is always resolved to the right value before the one existence check. - check-stale-expected now depends on `installcheck` directly, so it runs after pg_regress rather than before. TEST_DEPS list position doesn't guarantee ordering between unrelated prerequisites; an explicit dependency edge does. - EXTENSION_CURRENT_VERSION_FILES renamed to EXTENSION__CURRENT_VERSION__FILES for clearer segmentation. - README.asc: converted the Configuration Variables section from AsciiDoc labeled lists to `===` subsections per variable, and flagged the yes/no-only variables with a `*` marker plus an explanatory note, instead of repeating the constraint in every entry. - Tightened the two new HISTORY.asc entries that were more verbose than the rest of the changelog's style. Changes only in pgxntool. Related changes in pgxntool-test: - Rewrote the check-stale-expected ordering test to verify the new dependency-based ordering via `make -n test` dry-run, and to prove make test only fails on a stale file after pg_regress has actually produced results - Added check-stale-expected.sh to the dist file-count manifest Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This PR was getting too large; the rename is independent of the other fixes here (check-stale-expected, the parallel-build race, PGXN_REMOTE, the include guard) and doesn't need any test changes, so it's cleaner as its own PR straight from EXTENSION_VERSION_FILES to EXTENSION__CURRENT_VERSION__FILES. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
force-pushed
the
fix-buildsystem-batch
branch
from
July 26, 2026 19:54
0a406a5 to
6ccabae
Compare
This was referenced Jul 26, 2026
…re TEST_DEPS at the target, validate expected/ file types, add enable toggles - Moved check-stale-expected.sh to test/bin/check-stale-expected.sh (fixed its lib.sh source path accordingly) - TEST_DEPS += check-stale-expected now lives directly under the target's own definition, not down in the TEST_DEPS assembly section -- the base `TEST_DEPS = testdeps` assignment moved up ahead of it so the += there is well-defined - check-stale-expected.sh: added comments explaining the nullglob guard and the pg_regress alternate-file (_N.out) convention; removed the redundant existence check that decided whether to strip a _N suffix -- recognizing the pattern is pure string matching and doesn't need a stat call, so there's now exactly one existence check per file (documented tradeoff: a real test literally named foo_1 with no foo.sql would be misflagged -- accepted as vanishingly rare) - expected/ is now also checked for non-*.out files (stray editor files, .orig files, etc.), a distinct failure class from an orphaned .out file with its own message and exit-code bit, independently disable-able via PGXNTOOL_CHECK_EXPECTED_FILE_TYPES - Added PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED to make the whole check a no-op if wanted, following the same pattern as the other PGXNTOOL_ENABLE_* variables; documented both new variables in README.asc, plus a note pointing at Postgres-Extensions#30 for the larger override-any-recipe question this doesn't attempt to solve - README.asc: split PGXNTOOL_VERIFY_RESULTS_MODE's two modes into their own subsections instead of one paragraph Changes only in pgxntool. Related changes in pgxntool-test: - Update dist-expected-files.txt for the new script path - Extend check-stale-expected.bats for the new file-type check and both enable/disable toggles; fix the ordering test to filter out test-build's own (unrelated) pg_regress invocation, which the TEST_DEPS reordering above exposed Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool
that referenced
this pull request
Jul 26, 2026
…ostgres-Extensions#28) The old name implied it listed every version file for an extension; it actually only ever holds the current/most-recent one per extension. Split out of Postgres-Extensions#61 to keep that PR's scope down. If your Makefile references EXTENSION_VERSION_FILES directly (e.g. to add extra dependencies to DATA), update it to EXTENSION__CURRENT_VERSION__FILES. Changes only in pgxntool. Related changes in pgxntool-test: - Update the one code comment referencing the old name in test/sequential/02-dist.bats Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool
that referenced
this pull request
Jul 26, 2026
…ostgres-Extensions#28) The old name implied it listed every version file for an extension; it actually only ever holds the current/most-recent one per extension. Split out of Postgres-Extensions#61 to keep that PR's scope down. If your Makefile references EXTENSION_VERSION_FILES directly (e.g. to add extra dependencies to DATA), update it to EXTENSION__CURRENT_VERSION__FILES. Changes only in pgxntool. Related changes in pgxntool-test: - Update the one code comment referencing the old name in test/sequential/02-dist.bats Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool
that referenced
this pull request
Jul 26, 2026
…ostgres-Extensions#28) The old name implied it listed every version file for an extension; it actually only ever holds the current/most-recent one per extension. Split out of Postgres-Extensions#61 to keep that PR's scope down. If your Makefile references EXTENSION_VERSION_FILES directly (e.g. to add extra dependencies to DATA), update it to EXTENSION__CURRENT_VERSION__FILES. Changes only in pgxntool. Related changes in pgxntool-test: - Update the one code comment referencing the old name in test/sequential/02-dist.bats Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
>truncate then>>append across two recipe lines; a second firing (e.g. two overlappingmakeprocesses) doubled the file's content. Now a single atomic redirect.expected/#14:make testnever caught a staletest/expected/*.outleft behind after a test was renamed/removed. Addedcheck-stale-expected, tolerant of pg_regress's_N.outalternate-output files, and made it depend oninstallcheckdirectly so it runs after pg_regress, not before.originremote. Now configurable viaPGXN_REMOTE ?= origin.EXTENSION_VERSION_FILESis a confusing name #28:EXTENSION_VERSION_FILESrenamed toEXTENSION__CURRENT_VERSION__FILES-- old name implied it held all version files; it only ever holds the current one per extension.base.mkhad no include guard; double-inclusion redefined every target with warnings. Wrapped inifndef/endif.make clean'sEXTRA_CLEANtargeted a nonexistent top-levelresults/instead of$(TESTOUT)/results/.Adds a
Configuration Variablessection to README.asc documenting all override variables, and HISTORY.asc entries for the user-visible changes above.Related pgxntool-test PR: Postgres-Extensions/pgxntool-test#31
Fixes #7, #14, #19, #28, #50, #53.