From b001184cdaa2e251d672b004530ecee23e20630f Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 26 Jul 2026 09:54:42 -0500 Subject: [PATCH 1/5] Fix parallel-build SQL corruption, remote hardcoding, and add safety checks (#7, #14, #19, #28, #50, #53) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 (#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` (#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` (#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 (#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 (#50) - `make clean`'s `EXTRA_CLEAN` targeted a nonexistent top-level `results/` instead of the real `$(TESTOUT)/results/` (#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 --- CLAUDE.md | 2 +- base.mk | 59 +++++++++++++++++++++++++++++++++++++++++++++++---- control.mk.sh | 7 ++++-- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c443bef..0a83451 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -101,7 +101,7 @@ include pgxntool/base.mk - `PGXNVERSION` - version number - `EXTENSIONS` - list of extensions provided - `EXTENSION_*_VERSION` - per-extension versions - - `EXTENSION__CURRENT_VERSION__FILES` - auto-generated versioned SQL files + - `EXTENSION__CURRENT_VERSION__FILES` - the current/most-recent auto-generated versioned SQL file for each extension (not all version files) - `base.mk` includes `meta.mk` via `-include` ### The Magic of base.mk diff --git a/base.mk b/base.mk index b45273b..69f5668 100644 --- a/base.mk +++ b/base.mk @@ -1,3 +1,12 @@ +# Include guard: base.mk can end up included twice in a single `make` run +# (e.g. an extension's own .mk module includes it, and the extension's +# Makefile also includes it directly via the line setup.sh writes). Without +# this, every target in the file gets redefined, producing +# overriding-recipe/ignoring-old-recipe warnings. A second inclusion is a +# harmless no-op. +ifndef PGXNTOOL_BASE_MK_INCLUDED +PGXNTOOL_BASE_MK_INCLUDED := 1 + PGXNTOOL_DIR := pgxntool # Ensure 'all' is the default target (not META.json which happens to be first) @@ -193,6 +202,13 @@ MODULES =# Set to NUL so PGXS doesn't puke endif EXTRA_CLEAN = $(wildcard ../$(PGXN)-*.zip) pg_tle/ +# PGXS's own pg_regress_clean_files unconditionally rm -rf's a top-level +# results/, but our tests write to $(TESTOUT)/results/ (see REGRESS_OPTS +# --outputdir above), so that's the directory that actually needs cleaning. +# filter-out (not -=, which GNU Make 4.4+ rejects as a parse error) guards +# against a stray top-level results/ entry while adding the real one. +EXTRA_CLEAN := $(filter-out results/,$(EXTRA_CLEAN)) +EXTRA_CLEAN += $(TESTOUT)/results/ # Get Postgres version, as well as major (9.4, etc) version. # NOTE! In at least some versions, PGXS defines VERSION, so we intentionally don't use that variable @@ -256,6 +272,35 @@ installcheck: $(TEST_RESULT_FILES) $(TEST_SQL_FILES) | $(TESTDIR)/sql/ $(TESTDIR # # These targets are meant to make running tests easier. +# ------------------------------------------------------------------------------ +# check-stale-expected: catch orphaned test/expected/ files +# ------------------------------------------------------------------------------ +# Purpose: test/expected/*.out must mirror test/sql/*.sql 1:1 (likewise +# test/build/expected/*.out vs test/build/*.sql, when test-build is in use). +# It's easy to leave a stale .out behind after renaming or removing a .sql +# file; this makes `make test` fail loudly instead of letting it linger +# unnoticed. +# +# test/install/ is NOT checked here: per the test/install comments above, its +# expected output lives alongside the .sql files (test/install/foo.out), not +# in a separate expected/ subdirectory, so there's no 1:1 directory mirror to +# compare. +.PHONY: check-stale-expected +check-stale-expected: + @failed=0; \ + for pair in "$(TESTDIR)/sql $(TESTDIR)/expected" "$(TESTDIR)/build $(TESTDIR)/build/expected"; do \ + set -- $$pair; sqldir=$$1; expdir=$$2; \ + for f in "$$expdir"/*.out; do \ + [ -f "$$f" ] || continue; \ + base=$$(basename "$$f" .out); \ + if [ ! -f "$$sqldir/$$base.sql" ]; then \ + echo "ERROR: $$f has no corresponding $$sqldir/$$base.sql" >&2; \ + failed=1; \ + fi; \ + done; \ + done; \ + exit $$failed + # make test: run any test dependencies, then do a `make install installcheck`. # If regressions are found, it will output them. # @@ -264,7 +309,7 @@ installcheck: $(TEST_RESULT_FILES) $(TEST_SQL_FILES) | $(TESTDIR)/sql/ $(TESTDIR # clean it's an indication of a missing dependency anyway. .PHONY: test # Build test dependencies list based on enabled features -TEST_DEPS = testdeps +TEST_DEPS = check-stale-expected testdeps ifeq ($(PGXNTOOL_ENABLE_TEST_BUILD),yes) TEST_DEPS += test-build endif @@ -439,10 +484,14 @@ docclean: # # TAGGING SUPPORT # +# Remote used for tag/rmtag/forcetag/dist. Override on the command line or in +# your Makefile if you push tags somewhere other than origin. +PGXN_REMOTE ?= origin + rmtag: - git fetch origin # Update our remotes + git fetch $(PGXN_REMOTE) # Update our remotes @test -z "$$(git tag --list $(PGXNVERSION))" || git tag -d $(PGXNVERSION) - @test -z "$$(git ls-remote --tags origin $(PGXNVERSION) | grep -v '{}')" || git push --delete origin $(PGXNVERSION) + @test -z "$$(git ls-remote --tags $(PGXN_REMOTE) $(PGXNVERSION) | grep -v '{}')" || git push --delete $(PGXN_REMOTE) $(PGXNVERSION) tag: @test -z "$$(git status --porcelain)" || (echo 'Untracked changes!'; echo; git status; exit 1) @@ -457,7 +506,7 @@ tag: else \ git tag $(PGXNVERSION); \ fi - git push origin $(PGXNVERSION) + git push $(PGXN_REMOTE) $(PGXNVERSION) .PHONY: forcetag forcetag: rmtag tag @@ -560,3 +609,5 @@ $(DESTDIR)$(datadir)/extension/pgtap.control: pgxn install pgtap --sudo endif # fndef PGXNTOOL_NO_PGXS_INCLUDE + +endif # ifndef PGXNTOOL_BASE_MK_INCLUDED diff --git a/control.mk.sh b/control.mk.sh index c7e981c..aa491cf 100755 --- a/control.mk.sh +++ b/control.mk.sh @@ -82,8 +82,11 @@ for control_file in "$@"; do echo "EXTENSION_${ext}_VERSION_FILE = sql/${ext}--\$(EXTENSION_${ext}_VERSION).sql" echo "EXTENSION__CURRENT_VERSION__FILES += \$(EXTENSION_${ext}_VERSION_FILE)" echo "\$(EXTENSION_${ext}_VERSION_FILE): sql/${ext}.sql ${control_file}" - echo " @echo '/* DO NOT EDIT - AUTO-GENERATED FILE */' > \$(EXTENSION_${ext}_VERSION_FILE)" - echo " @cat sql/${ext}.sql >> \$(EXTENSION_${ext}_VERSION_FILE)" + # Single redirect via a subshell, not `>` truncate followed by `>>` append: + # if this rule ever runs twice in the same `make` invocation (e.g. reached + # through two different dependency chains under parallel make), the `>>` + # form doubles the file's content instead of overwriting it. + echo " @(echo '/* DO NOT EDIT - AUTO-GENERATED FILE */'; cat sql/${ext}.sql) > \$(EXTENSION_${ext}_VERSION_FILE)" echo done From aadd9fc349d48c08f746b762f8d3ac8702ea2215 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 26 Jul 2026 14:12:49 -0500 Subject: [PATCH 2/5] Address review: fix check-stale-expected alternate-output false positive, document all override variables, add HISTORY entries - check-stale-expected (#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 #7, #14, #19, #50, #53 (see paired pgxntool-test PR) Co-authored-by: Claude Sonnet 5 --- HISTORY.asc | 28 +++++++++++++++++ README.asc | 49 +++++++++++++++++++++++++++++ README.html | 90 ++++++++++++++++++++++++++++++++++++++++++++++++----- base.mk | 19 +++++++++-- 4 files changed, 175 insertions(+), 11 deletions(-) diff --git a/HISTORY.asc b/HISTORY.asc index 8440012..d4e09af 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -18,6 +18,34 @@ with a digit, so extensions using a persistent `stable` pseudo-version filename". Widened to accept any non-empty, `--`-free version string, matching how pg_tle itself treats extension versions. +== Fix versioned SQL file corruption under overlapping make invocations +The rule generating `sql/--.sql` used a `>` truncate followed +by a separate `>>` append across two recipe lines. If the rule fired more +than once from overlapping `make` processes (e.g. two builds sharing a +checkout), the second firing's append doubled the file's content instead of +overwriting it — confirmed in practice as a 264-line file found at 527 +lines. Now uses a single atomic redirect, so the rule is safe no matter how +many times it fires. + +== Add `check-stale-expected` to catch orphaned test/expected files +`make test` now fails if `test/expected/*.out` (or `test/build/expected/*.out`) +has no corresponding `test/sql/*.sql` (or `test/build/*.sql`), catching files +left behind after a test is renamed or removed. If your project has +genuinely stale expected-output files sitting around, `make test` will newly +fail until they're removed. pg_regress's alternate expected-output files +(`test_0.out` .. `test_9.out`) are recognized and not flagged as stale. + +== Add `PGXN_REMOTE` to override the remote used by tag/rmtag/forcetag/dist +These targets hardcoded the `origin` remote, silently re-tagging the wrong +repository for maintainers whose `origin` is a personal fork. Defaults to +`origin`, so existing behavior is unchanged unless you set it. + +== Add include guard to base.mk +`base.mk` could end up included twice in one `make` run (for example, an +extension's own `.mk` module includes it in addition to the Makefile's own +include), which redefined every target and printed overriding-recipe +warnings. A second inclusion is now a harmless no-op. + 2.1.0 ----- == Fix setup.sh / pgxntool-sync.sh / update-setup-files.sh inside a git worktree diff --git a/README.asc b/README.asc index 5fdc15c..daa36d7 100644 --- a/README.asc +++ b/README.asc @@ -603,6 +603,55 @@ Each generated SQL file is wrapped in a transaction (`BEGIN;` ... `COMMIT;`) to *Solution*: pg_tle only supports trusted languages. You cannot use C extensions with pg_tle. The script will warn you but still generate files (which won't work). NOTE: there are several untrusted languages (such as plpython), and the only tests for C. + +== Configuration Variables + +This is the comprehensive list of make variables you can override to customize pgxntool's behavior, either in your Makefile (before `include pgxntool/base.mk`) or on the `make` command line. Several of these are also covered in more detail in the sections above; this section exists so there's one place that lists all of them. + +PGXN_REMOTE:: +Default: `origin`. +The git remote used by `tag`, `rmtag`, `forcetag`, and `dist` (which depends on `tag`). If your `origin` is a personal fork rather than the canonical repository -- for example, if you develop on a fork and only push release tags to the canonical repo -- set this so tagging and pushing target the right remote: ++ +---- +make dist PGXN_REMOTE=upstream +---- + +ASCIIDOC:: +Default: auto-detected, the first of `asciidoctor` or `asciidoc` found on `PATH`. +Path to the Asciidoc processor used to build `.html` files from `$(ASCIIDOC_EXTS)` source files. Override if the processor you want isn't first on `PATH`, or isn't on `PATH` at all. See <<_document_handling>>. + +PG_CONFIG:: +Default: `pg_config`. +Path to the `pg_config` binary used to detect the PostgreSQL version and locate PGXS. Override when the right `pg_config` isn't the one on `PATH`, such as when testing against a specific PostgreSQL install. + +TESTDIR:: +Default: `test`. +Root directory for test input files: `$(TESTDIR)/sql/`, `$(TESTDIR)/expected/`, `$(TESTDIR)/build/`, `$(TESTDIR)/install/`. Overriding this on its own is unusual; it mainly exists so `TESTOUT` has a sensible default. + +TESTOUT:: +Default: `$(TESTDIR)`. +Directory `pg_regress` writes actual test output to -- `$(TESTOUT)/results/`, `$(TESTOUT)/regression.diffs`, etc. -- via `--outputdir` in `REGRESS_OPTS`. Kept separate from `TESTDIR` so generated output can be pointed somewhere other than the directory holding your checked-in `sql`/`expected` files, if you want that separation. + +PGXNTOOL_VERIFY_RESULTS_MODE:: +Default: `pgtap`. +Controls how the <<_verify_results_safeguard,verify-results safeguard>> decides whether tests are failing. `pgtap` scans `test/results/*.out` for pgTap `not ok` lines and plan mismatches (falling back to checking for `regression.diffs` too); `diffs` checks only for `regression.diffs`, matching classic pg_regress behavior. Set to `diffs` if your test suite doesn't use pgTap. + +PGXNTOOL_ENABLE_TEST_BUILD:: +Default: auto-detected -- `yes` if `test/build/*.sql` files exist, `no` otherwise. +Enables or disables the <<_test_build,test-build>> pre-flight SQL check. Set explicitly to `yes` if you want an error when `test/build/` unexpectedly has no SQL files (catches accidental deletion of its contents), or to `no` to disable the check even when files are present. + +PGXNTOOL_ENABLE_TEST_INSTALL:: +Default: auto-detected -- `yes` if `test/install/*.sql` files exist, `no` otherwise. +Enables or disables the <<_testinstall,test/install>> schedule-based setup feature. Same explicit-override semantics as `PGXNTOOL_ENABLE_TEST_BUILD`. + +PGXNTOOL_ENABLE_VERIFY_RESULTS:: +Default: `yes`. +Enables or disables the <<_verify_results_safeguard,verify-results safeguard>> that blocks `make results` when tests are failing. Setting it to empty on the command line (`make PGXNTOOL_ENABLE_VERIFY_RESULTS= results`) also disables it. + +PGXNTOOL_NO_PGXS_INCLUDE:: +Default: unset (PGXS is included normally). +Skips including PGXS (`$(PGXS)`) entirely. This is only for advanced scenarios where you need to manage the PGXS include yourself; most projects should never set this. + == Copyright Copyright (c) 2026 Jim Nasby diff --git a/README.html b/README.html index 431358f..23f3311 100644 --- a/README.html +++ b/README.html @@ -4,7 +4,7 @@ - + PGXNtool @@ -141,7 +141,7 @@ #content::before{content:none} #header>h1:first-child{color:rgba(0,0,0,.85);margin-top:2.25rem;margin-bottom:0} #header>h1:first-child+#toc{margin-top:8px;border-top:1px solid #dddddf} -#header>h1:only-child{border-bottom:1px solid #dddddf;padding-bottom:8px} +#header>h1:only-child,body.toc2 #header>h1:nth-last-child(2){border-bottom:1px solid #dddddf;padding-bottom:8px} #header .details{border-bottom:1px solid #dddddf;line-height:1.45;padding-top:.25em;padding-bottom:.25em;padding-left:.25em;color:rgba(0,0,0,.6);display:flex;flex-flow:row wrap} #header .details span:first-child{margin-left:-.125em} #header .details span.email a{color:rgba(0,0,0,.85)} @@ -163,7 +163,6 @@ #toctitle{color:#7a2518;font-size:1.2em} @media screen and (min-width:768px){#toctitle{font-size:1.375em} body.toc2{padding-left:15em;padding-right:0} -body.toc2 #header>h1:nth-last-child(2){border-bottom:1px solid #dddddf;padding-bottom:8px} #toc.toc2{margin-top:0!important;background:#f8f8f7;position:fixed;width:15em;left:0;top:0;border-right:1px solid #e7e7e9;border-top-width:0!important;border-bottom-width:0!important;z-index:1000;padding:1.25em 1em;height:100%;overflow:auto} #toc.toc2 #toctitle{margin-top:0;margin-bottom:.8rem;font-size:1.2em} #toc.toc2>ul{font-size:.9em;margin-bottom:0} @@ -329,7 +328,7 @@ a.image object{pointer-events:none} sup.footnote,sup.footnoteref{font-size:.875em;position:static;vertical-align:super} sup.footnote a,sup.footnoteref a{text-decoration:none} -sup.footnote a:active,sup.footnoteref a:active,#footnotes .footnote a:first-of-type:active{text-decoration:underline} +sup.footnote a:active,sup.footnoteref a:active{text-decoration:underline} #footnotes{padding-top:.75em;padding-bottom:.75em;margin-bottom:.625em} #footnotes hr{width:20%;min-width:6.25em;margin:-.25em 0 .75em;border-width:1px 0 0} #footnotes .footnote{padding:0 .375em 0 .225em;line-height:1.3334;font-size:.875em;margin-left:1.2em;margin-bottom:.2em} @@ -492,6 +491,8 @@

PGXNtool

  • 7.7. Troubleshooting
    • +
  • 8. Configuration Variables
    • +
  • 9. Copyright
    • @@ -1497,23 +1498,96 @@

    there are several untrusted languages (such as plpython), and the only tests for C. -== Copyright -Copyright (c) 2026 Jim Nasby <Jim.Nasby@gmail.com> + + + + +
    +

    8. Configuration Variables

    +
    -

    PGXNtool is released under a BSD license. Note that it includes JSON.sh, which is released under a MIT license.

    +

    This is the comprehensive list of make variables you can override to customize pgxntool’s behavior, either in your Makefile (before include pgxntool/base.mk) or on the make command line. Several of these are also covered in more detail in the sections above; this section exists so there’s one place that lists all of them.

    +
    +
    +
    +
    PGXN_REMOTE
    +
    +

    Default: origin. +The git remote used by tag, rmtag, forcetag, and dist (which depends on tag). If your origin is a personal fork rather than the canonical repository — for example, if you develop on a fork and only push release tags to the canonical repo — set this so tagging and pushing target the right remote:

    +
    +
    +
    make dist PGXN_REMOTE=upstream
    +
    +
    ASCIIDOC
    +
    +

    Default: auto-detected, the first of asciidoctor or asciidoc found on PATH. +Path to the Asciidoc processor used to build .html files from $(ASCIIDOC_EXTS) source files. Override if the processor you want isn’t first on PATH, or isn’t on PATH at all. See Document Handling.

    +
    +
    PG_CONFIG
    +
    +

    Default: pg_config. +Path to the pg_config binary used to detect the PostgreSQL version and locate PGXS. Override when the right pg_config isn’t the one on PATH, such as when testing against a specific PostgreSQL install.

    +
    +
    TESTDIR
    +
    +

    Default: test. +Root directory for test input files: $(TESTDIR)/sql/, $(TESTDIR)/expected/, $(TESTDIR)/build/, $(TESTDIR)/install/. Overriding this on its own is unusual; it mainly exists so TESTOUT has a sensible default.

    +
    +
    TESTOUT
    +
    +

    Default: $(TESTDIR). +Directory pg_regress writes actual test output to — $(TESTOUT)/results/, $(TESTOUT)/regression.diffs, etc. — via --outputdir in REGRESS_OPTS. Kept separate from TESTDIR so generated output can be pointed somewhere other than the directory holding your checked-in sql/expected files, if you want that separation.

    +
    +
    PGXNTOOL_VERIFY_RESULTS_MODE
    +
    +

    Default: pgtap. +Controls how the verify-results safeguard decides whether tests are failing. pgtap scans test/results/*.out for pgTap not ok lines and plan mismatches (falling back to checking for regression.diffs too); diffs checks only for regression.diffs, matching classic pg_regress behavior. Set to diffs if your test suite doesn’t use pgTap.

    +
    +
    PGXNTOOL_ENABLE_TEST_BUILD
    +
    +

    Default: auto-detected — yes if test/build/*.sql files exist, no otherwise. +Enables or disables the test-build pre-flight SQL check. Set explicitly to yes if you want an error when test/build/ unexpectedly has no SQL files (catches accidental deletion of its contents), or to no to disable the check even when files are present.

    +
    +
    PGXNTOOL_ENABLE_TEST_INSTALL
    +
    +

    Default: auto-detected — yes if test/install/*.sql files exist, no otherwise. +Enables or disables the test/install schedule-based setup feature. Same explicit-override semantics as PGXNTOOL_ENABLE_TEST_BUILD.

    +
    +
    PGXNTOOL_ENABLE_VERIFY_RESULTS
    +
    +

    Default: yes. +Enables or disables the verify-results safeguard that blocks make results when tests are failing. Setting it to empty on the command line (make PGXNTOOL_ENABLE_VERIFY_RESULTS= results) also disables it.

    +
    +
    PGXNTOOL_NO_PGXS_INCLUDE
    +
    +

    Default: unset (PGXS is included normally). +Skips including PGXS ($(PGXS)) entirely. This is only for advanced scenarios where you need to manage the PGXS include yourself; most projects should never set this.

    +
    +
    +
    +
    +
    +
    +

    9. Copyright

    +
    +
    +

    Copyright (c) 2026 Jim Nasby <Jim.Nasby@gmail.com>

    +
    +
    +

    PGXNtool is released under a BSD license. Note that it includes JSON.sh, which is released under a MIT license.

    diff --git a/base.mk b/base.mk index 69f5668..ee66238 100644 --- a/base.mk +++ b/base.mk @@ -207,8 +207,7 @@ EXTRA_CLEAN = $(wildcard ../$(PGXN)-*.zip) pg_tle/ # --outputdir above), so that's the directory that actually needs cleaning. # filter-out (not -=, which GNU Make 4.4+ rejects as a parse error) guards # against a stray top-level results/ entry while adding the real one. -EXTRA_CLEAN := $(filter-out results/,$(EXTRA_CLEAN)) -EXTRA_CLEAN += $(TESTOUT)/results/ +EXTRA_CLEAN := $(filter-out results/,$(EXTRA_CLEAN)) $(TESTOUT)/results/ # Get Postgres version, as well as major (9.4, etc) version. # NOTE! In at least some versions, PGXS defines VERSION, so we intentionally don't use that variable @@ -285,6 +284,13 @@ installcheck: $(TEST_RESULT_FILES) $(TEST_SQL_FILES) | $(TESTDIR)/sql/ $(TESTDIR # expected output lives alongside the .sql files (test/install/foo.out), not # in a separate expected/ subdirectory, so there's no 1:1 directory mirror to # compare. +# +# pg_regress supports up to 10 alternate expected-output files per test +# (test.out, test_0.out .. test_9.out - see get_alternative_expectfile() in +# pg_regress.c), tried in turn when the primary doesn't match. A naive +# basename comparison flags test_1.out etc. as orphaned since there's no +# test_1.sql. Before reporting a file as stale, also check whether stripping +# a trailing _N (single digit) yields a base with a matching .sql. .PHONY: check-stale-expected check-stale-expected: @failed=0; \ @@ -293,7 +299,14 @@ check-stale-expected: for f in "$$expdir"/*.out; do \ [ -f "$$f" ] || continue; \ base=$$(basename "$$f" .out); \ - if [ ! -f "$$sqldir/$$base.sql" ]; then \ + ok=0; \ + [ -f "$$sqldir/$$base.sql" ] && ok=1; \ + if [ "$$ok" = 0 ]; then \ + case "$$base" in \ + *_[0-9]) altbase=$${base%_*}; [ -f "$$sqldir/$$altbase.sql" ] && ok=1 ;; \ + esac; \ + fi; \ + if [ "$$ok" = 0 ]; then \ echo "ERROR: $$f has no corresponding $$sqldir/$$base.sql" >&2; \ failed=1; \ fi; \ From 06644df677f94f5a8e493b0318b20281bce1f559 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 26 Jul 2026 14:34:15 -0500 Subject: [PATCH 3/5] Address review round 2: extract check-stale-expected to a script, run 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 --- HISTORY.asc | 18 ++--- README.asc | 64 +++++++++--------- README.html | 141 ++++++++++++++++++++++++---------------- base.mk | 48 ++++---------- check-stale-expected.sh | 66 +++++++++++++++++++ 5 files changed, 204 insertions(+), 133 deletions(-) create mode 100755 check-stale-expected.sh diff --git a/HISTORY.asc b/HISTORY.asc index d4e09af..8b002e6 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -20,20 +20,16 @@ matching how pg_tle itself treats extension versions. == Fix versioned SQL file corruption under overlapping make invocations The rule generating `sql/--.sql` used a `>` truncate followed -by a separate `>>` append across two recipe lines. If the rule fired more -than once from overlapping `make` processes (e.g. two builds sharing a -checkout), the second firing's append doubled the file's content instead of -overwriting it — confirmed in practice as a 264-line file found at 527 -lines. Now uses a single atomic redirect, so the rule is safe no matter how -many times it fires. +by a separate `>>` append, so a rule that fired more than once (e.g. +overlapping `make` processes) doubled the file's content. Now uses a single +atomic redirect. == Add `check-stale-expected` to catch orphaned test/expected files `make test` now fails if `test/expected/*.out` (or `test/build/expected/*.out`) -has no corresponding `test/sql/*.sql` (or `test/build/*.sql`), catching files -left behind after a test is renamed or removed. If your project has -genuinely stale expected-output files sitting around, `make test` will newly -fail until they're removed. pg_regress's alternate expected-output files -(`test_0.out` .. `test_9.out`) are recognized and not flagged as stale. +has no corresponding `test/sql/*.sql` (or `test/build/*.sql`). If your project +has genuinely stale expected-output files sitting around, `make test` will +newly fail until they're removed. pg_regress's alternate expected-output +files (`test_0.out` .. `test_9.out`) are recognized and not flagged as stale. == Add `PGXN_REMOTE` to override the remote used by tag/rmtag/forcetag/dist These targets hardcoded the `origin` remote, silently re-tagging the wrong diff --git a/README.asc b/README.asc index daa36d7..414daa6 100644 --- a/README.asc +++ b/README.asc @@ -608,49 +608,51 @@ NOTE: there are several untrusted languages (such as plpython), and the only tes This is the comprehensive list of make variables you can override to customize pgxntool's behavior, either in your Makefile (before `include pgxntool/base.mk`) or on the `make` command line. Several of these are also covered in more detail in the sections above; this section exists so there's one place that lists all of them. -PGXN_REMOTE:: -Default: `origin`. -The git remote used by `tag`, `rmtag`, `forcetag`, and `dist` (which depends on `tag`). If your `origin` is a personal fork rather than the canonical repository -- for example, if you develop on a fork and only push release tags to the canonical repo -- set this so tagging and pushing target the right remote: -+ +NOTE: Variables marked with `*` must be set to exactly `yes` or `no` (case-insensitive) when given explicitly; any other value is a hard `make` error. + +=== PGXN_REMOTE + +Default: `origin`. The git remote used by `tag`, `rmtag`, `forcetag`, and `dist` (which depends on `tag`). If your `origin` is a personal fork rather than the canonical repository -- for example, if you develop on a fork and only push release tags to the canonical repo -- set this so tagging and pushing target the right remote: + ---- make dist PGXN_REMOTE=upstream ---- -ASCIIDOC:: -Default: auto-detected, the first of `asciidoctor` or `asciidoc` found on `PATH`. -Path to the Asciidoc processor used to build `.html` files from `$(ASCIIDOC_EXTS)` source files. Override if the processor you want isn't first on `PATH`, or isn't on `PATH` at all. See <<_document_handling>>. +=== ASCIIDOC + +Default: auto-detected, the first of `asciidoctor` or `asciidoc` found on `PATH`. Path to the Asciidoc processor used to build `.html` files from `$(ASCIIDOC_EXTS)` source files. Override if the processor you want isn't first on `PATH`, or isn't on `PATH` at all. See <<_document_handling>>. + +=== PG_CONFIG + +Default: `pg_config`. Path to the `pg_config` binary used to detect the PostgreSQL version and locate PGXS. Override when the right `pg_config` isn't the one on `PATH`, such as when testing against a specific PostgreSQL install. + +=== TESTDIR + +Default: `test`. Root directory for test input files: `$(TESTDIR)/sql/`, `$(TESTDIR)/expected/`, `$(TESTDIR)/build/`, `$(TESTDIR)/install/`. Overriding this on its own is unusual; it mainly exists so `TESTOUT` has a sensible default. + +=== TESTOUT + +Default: `$(TESTDIR)`. Directory `pg_regress` writes actual test output to -- `$(TESTOUT)/results/`, `$(TESTOUT)/regression.diffs`, etc. -- via `--outputdir` in `REGRESS_OPTS`. Kept separate from `TESTDIR` so generated output can be pointed somewhere other than the directory holding your checked-in `sql`/`expected` files, if you want that separation. + +=== PGXNTOOL_VERIFY_RESULTS_MODE + +Default: `pgtap`. Controls how the <<_verify_results_safeguard,verify-results safeguard>> decides whether tests are failing. `pgtap` scans `test/results/*.out` for pgTap `not ok` lines and plan mismatches (falling back to checking for `regression.diffs` too); `diffs` checks only for `regression.diffs`, matching classic pg_regress behavior. Set to `diffs` if your test suite doesn't use pgTap. -PG_CONFIG:: -Default: `pg_config`. -Path to the `pg_config` binary used to detect the PostgreSQL version and locate PGXS. Override when the right `pg_config` isn't the one on `PATH`, such as when testing against a specific PostgreSQL install. +=== PGXNTOOL_ENABLE_TEST_BUILD * -TESTDIR:: -Default: `test`. -Root directory for test input files: `$(TESTDIR)/sql/`, `$(TESTDIR)/expected/`, `$(TESTDIR)/build/`, `$(TESTDIR)/install/`. Overriding this on its own is unusual; it mainly exists so `TESTOUT` has a sensible default. +Default: auto-detected -- `yes` if `test/build/*.sql` files exist, `no` otherwise. Enables or disables the <<_test_build,test-build>> pre-flight SQL check. Set explicitly to `yes` if you want an error when `test/build/` unexpectedly has no SQL files (catches accidental deletion of its contents), or to `no` to disable the check even when files are present. -TESTOUT:: -Default: `$(TESTDIR)`. -Directory `pg_regress` writes actual test output to -- `$(TESTOUT)/results/`, `$(TESTOUT)/regression.diffs`, etc. -- via `--outputdir` in `REGRESS_OPTS`. Kept separate from `TESTDIR` so generated output can be pointed somewhere other than the directory holding your checked-in `sql`/`expected` files, if you want that separation. +=== PGXNTOOL_ENABLE_TEST_INSTALL * -PGXNTOOL_VERIFY_RESULTS_MODE:: -Default: `pgtap`. -Controls how the <<_verify_results_safeguard,verify-results safeguard>> decides whether tests are failing. `pgtap` scans `test/results/*.out` for pgTap `not ok` lines and plan mismatches (falling back to checking for `regression.diffs` too); `diffs` checks only for `regression.diffs`, matching classic pg_regress behavior. Set to `diffs` if your test suite doesn't use pgTap. +Default: auto-detected -- `yes` if `test/install/*.sql` files exist, `no` otherwise. Enables or disables the <<_testinstall,test/install>> schedule-based setup feature. Same explicit-override semantics as `PGXNTOOL_ENABLE_TEST_BUILD`. -PGXNTOOL_ENABLE_TEST_BUILD:: -Default: auto-detected -- `yes` if `test/build/*.sql` files exist, `no` otherwise. -Enables or disables the <<_test_build,test-build>> pre-flight SQL check. Set explicitly to `yes` if you want an error when `test/build/` unexpectedly has no SQL files (catches accidental deletion of its contents), or to `no` to disable the check even when files are present. +=== PGXNTOOL_ENABLE_VERIFY_RESULTS * -PGXNTOOL_ENABLE_TEST_INSTALL:: -Default: auto-detected -- `yes` if `test/install/*.sql` files exist, `no` otherwise. -Enables or disables the <<_testinstall,test/install>> schedule-based setup feature. Same explicit-override semantics as `PGXNTOOL_ENABLE_TEST_BUILD`. +Default: `yes`. Enables or disables the <<_verify_results_safeguard,verify-results safeguard>> that blocks `make results` when tests are failing. Setting it to empty on the command line (`make PGXNTOOL_ENABLE_VERIFY_RESULTS= results`) also disables it. -PGXNTOOL_ENABLE_VERIFY_RESULTS:: -Default: `yes`. -Enables or disables the <<_verify_results_safeguard,verify-results safeguard>> that blocks `make results` when tests are failing. Setting it to empty on the command line (`make PGXNTOOL_ENABLE_VERIFY_RESULTS= results`) also disables it. +=== PGXNTOOL_NO_PGXS_INCLUDE -PGXNTOOL_NO_PGXS_INCLUDE:: -Default: unset (PGXS is included normally). -Skips including PGXS (`$(PGXS)`) entirely. This is only for advanced scenarios where you need to manage the PGXS include yourself; most projects should never set this. +Default: unset (PGXS is included normally). Skips including PGXS (`$(PGXS)`) entirely. This is only for advanced scenarios where you need to manage the PGXS include yourself; most projects should never set this. == Copyright Copyright (c) 2026 Jim Nasby diff --git a/README.html b/README.html index 23f3311..fcba57d 100644 --- a/README.html +++ b/README.html @@ -491,7 +491,20 @@

    PGXNtool

  • 7.7. Troubleshooting
    • -
  • 8. Configuration Variables
    • +
  • 8. Configuration Variables + +
  • 9. Copyright
    • @@ -1512,64 +1525,82 @@

    This is the comprehensive list of make variables you can override to customize pgxntool’s behavior, either in your Makefile (before include pgxntool/base.mk) or on the make command line. Several of these are also covered in more detail in the sections above; this section exists so there’s one place that lists all of them.

    -
    -
    -
    PGXN_REMOTE
    -
    -

    Default: origin. -The git remote used by tag, rmtag, forcetag, and dist (which depends on tag). If your origin is a personal fork rather than the canonical repository — for example, if you develop on a fork and only push release tags to the canonical repo — set this so tagging and pushing target the right remote:

    +
    + + + + + +
    +
    Note
    +    		 +Variables marked with * must be set to exactly yes or no (case-insensitive) when given explicitly; any other value is a hard make error. +
    +
    +
    +

    8.1. PGXN_REMOTE

    +
    +

    Default: origin. The git remote used by tag, rmtag, forcetag, and dist (which depends on tag). If your origin is a personal fork rather than the canonical repository — for example, if you develop on a fork and only push release tags to the canonical repo — set this so tagging and pushing target the right remote:

    +
    make dist PGXN_REMOTE=upstream
    -
    -
    ASCIIDOC
    -
    -

    Default: auto-detected, the first of asciidoctor or asciidoc found on PATH. -Path to the Asciidoc processor used to build .html files from $(ASCIIDOC_EXTS) source files. Override if the processor you want isn’t first on PATH, or isn’t on PATH at all. See Document Handling.

    -
    -
    PG_CONFIG
    -
    -

    Default: pg_config. -Path to the pg_config binary used to detect the PostgreSQL version and locate PGXS. Override when the right pg_config isn’t the one on PATH, such as when testing against a specific PostgreSQL install.

    -
    -
    TESTDIR
    -
    -

    Default: test. -Root directory for test input files: $(TESTDIR)/sql/, $(TESTDIR)/expected/, $(TESTDIR)/build/, $(TESTDIR)/install/. Overriding this on its own is unusual; it mainly exists so TESTOUT has a sensible default.

    -
    -
    TESTOUT
    -
    -

    Default: $(TESTDIR). -Directory pg_regress writes actual test output to — $(TESTOUT)/results/, $(TESTOUT)/regression.diffs, etc. — via --outputdir in REGRESS_OPTS. Kept separate from TESTDIR so generated output can be pointed somewhere other than the directory holding your checked-in sql/expected files, if you want that separation.

    -
    -
    PGXNTOOL_VERIFY_RESULTS_MODE
    -
    -

    Default: pgtap. -Controls how the verify-results safeguard decides whether tests are failing. pgtap scans test/results/*.out for pgTap not ok lines and plan mismatches (falling back to checking for regression.diffs too); diffs checks only for regression.diffs, matching classic pg_regress behavior. Set to diffs if your test suite doesn’t use pgTap.

    -
    -
    PGXNTOOL_ENABLE_TEST_BUILD
    -
    -

    Default: auto-detected — yes if test/build/*.sql files exist, no otherwise. -Enables or disables the test-build pre-flight SQL check. Set explicitly to yes if you want an error when test/build/ unexpectedly has no SQL files (catches accidental deletion of its contents), or to no to disable the check even when files are present.

    -
    -
    PGXNTOOL_ENABLE_TEST_INSTALL
    -
    -

    Default: auto-detected — yes if test/install/*.sql files exist, no otherwise. -Enables or disables the test/install schedule-based setup feature. Same explicit-override semantics as PGXNTOOL_ENABLE_TEST_BUILD.

    -
    -
    PGXNTOOL_ENABLE_VERIFY_RESULTS
    -
    -

    Default: yes. -Enables or disables the verify-results safeguard that blocks make results when tests are failing. Setting it to empty on the command line (make PGXNTOOL_ENABLE_VERIFY_RESULTS= results) also disables it.

    -
    -
    PGXNTOOL_NO_PGXS_INCLUDE
    -
    -

    Default: unset (PGXS is included normally). -Skips including PGXS ($(PGXS)) entirely. This is only for advanced scenarios where you need to manage the PGXS include yourself; most projects should never set this.

    -
    -
    +
    +
    +

    8.2. ASCIIDOC

    +
    +

    Default: auto-detected, the first of asciidoctor or asciidoc found on PATH. Path to the Asciidoc processor used to build .html files from $(ASCIIDOC_EXTS) source files. Override if the processor you want isn’t first on PATH, or isn’t on PATH at all. See Document Handling.

    +
    +
    +
    +

    8.3. PG_CONFIG

    +
    +

    Default: pg_config. Path to the pg_config binary used to detect the PostgreSQL version and locate PGXS. Override when the right pg_config isn’t the one on PATH, such as when testing against a specific PostgreSQL install.

    +
    +
    +
    +

    8.4. TESTDIR

    +
    +

    Default: test. Root directory for test input files: $(TESTDIR)/sql/, $(TESTDIR)/expected/, $(TESTDIR)/build/, $(TESTDIR)/install/. Overriding this on its own is unusual; it mainly exists so TESTOUT has a sensible default.

    +
    +
    +
    +

    8.5. TESTOUT

    +
    +

    Default: $(TESTDIR). Directory pg_regress writes actual test output to — $(TESTOUT)/results/, $(TESTOUT)/regression.diffs, etc. — via --outputdir in REGRESS_OPTS. Kept separate from TESTDIR so generated output can be pointed somewhere other than the directory holding your checked-in sql/expected files, if you want that separation.

    +
    +
    +
    +

    8.6. PGXNTOOL_VERIFY_RESULTS_MODE

    +
    +

    Default: pgtap. Controls how the verify-results safeguard decides whether tests are failing. pgtap scans test/results/*.out for pgTap not ok lines and plan mismatches (falling back to checking for regression.diffs too); diffs checks only for regression.diffs, matching classic pg_regress behavior. Set to diffs if your test suite doesn’t use pgTap.

    +
    +
    +
    +

    8.7. PGXNTOOL_ENABLE_TEST_BUILD *

    +
    +

    Default: auto-detected — yes if test/build/*.sql files exist, no otherwise. Enables or disables the test-build pre-flight SQL check. Set explicitly to yes if you want an error when test/build/ unexpectedly has no SQL files (catches accidental deletion of its contents), or to no to disable the check even when files are present.

    +
    +
    +
    +

    8.8. PGXNTOOL_ENABLE_TEST_INSTALL *

    +
    +

    Default: auto-detected — yes if test/install/*.sql files exist, no otherwise. Enables or disables the test/install schedule-based setup feature. Same explicit-override semantics as PGXNTOOL_ENABLE_TEST_BUILD.

    +
    +
    +
    +

    8.9. PGXNTOOL_ENABLE_VERIFY_RESULTS *

    +
    +

    Default: yes. Enables or disables the verify-results safeguard that blocks make results when tests are failing. Setting it to empty on the command line (make PGXNTOOL_ENABLE_VERIFY_RESULTS= results) also disables it.

    +
    +
    +
    +

    8.10. PGXNTOOL_NO_PGXS_INCLUDE

    +
    +

    Default: unset (PGXS is included normally). Skips including PGXS ($(PGXS)) entirely. This is only for advanced scenarios where you need to manage the PGXS include yourself; most projects should never set this.

    +
    @@ -1587,7 +1618,7 @@

    diff --git a/base.mk b/base.mk index ee66238..bd387ce 100644 --- a/base.mk +++ b/base.mk @@ -278,41 +278,17 @@ installcheck: $(TEST_RESULT_FILES) $(TEST_SQL_FILES) | $(TESTDIR)/sql/ $(TESTDIR # test/build/expected/*.out vs test/build/*.sql, when test-build is in use). # It's easy to leave a stale .out behind after renaming or removing a .sql # file; this makes `make test` fail loudly instead of letting it linger -# unnoticed. -# -# test/install/ is NOT checked here: per the test/install comments above, its -# expected output lives alongside the .sql files (test/install/foo.out), not -# in a separate expected/ subdirectory, so there's no 1:1 directory mirror to -# compare. -# -# pg_regress supports up to 10 alternate expected-output files per test -# (test.out, test_0.out .. test_9.out - see get_alternative_expectfile() in -# pg_regress.c), tried in turn when the primary doesn't match. A naive -# basename comparison flags test_1.out etc. as orphaned since there's no -# test_1.sql. Before reporting a file as stale, also check whether stripping -# a trailing _N (single digit) yields a base with a matching .sql. +# unnoticed. Logic lives in check-stale-expected.sh (enough of it to warrant +# a real script rather than an inline recipe). +# +# This depends on `installcheck` directly (not just position in TEST_DEPS) +# because it MUST run after pg_regress, not before: TEST_DEPS lists multiple +# independent prerequisites of `test`, and Make does not guarantee the order +# unrelated prerequisites of the same target are built in. An explicit +# dependency edge is the only ordering guarantee Make actually gives. .PHONY: check-stale-expected -check-stale-expected: - @failed=0; \ - for pair in "$(TESTDIR)/sql $(TESTDIR)/expected" "$(TESTDIR)/build $(TESTDIR)/build/expected"; do \ - set -- $$pair; sqldir=$$1; expdir=$$2; \ - for f in "$$expdir"/*.out; do \ - [ -f "$$f" ] || continue; \ - base=$$(basename "$$f" .out); \ - ok=0; \ - [ -f "$$sqldir/$$base.sql" ] && ok=1; \ - if [ "$$ok" = 0 ]; then \ - case "$$base" in \ - *_[0-9]) altbase=$${base%_*}; [ -f "$$sqldir/$$altbase.sql" ] && ok=1 ;; \ - esac; \ - fi; \ - if [ "$$ok" = 0 ]; then \ - echo "ERROR: $$f has no corresponding $$sqldir/$$base.sql" >&2; \ - failed=1; \ - fi; \ - done; \ - done; \ - exit $$failed +check-stale-expected: installcheck + @$(PGXNTOOL_DIR)/check-stale-expected.sh $(TESTDIR) # make test: run any test dependencies, then do a `make install installcheck`. # If regressions are found, it will output them. @@ -322,11 +298,11 @@ check-stale-expected: # clean it's an indication of a missing dependency anyway. .PHONY: test # Build test dependencies list based on enabled features -TEST_DEPS = check-stale-expected testdeps +TEST_DEPS = testdeps ifeq ($(PGXNTOOL_ENABLE_TEST_BUILD),yes) TEST_DEPS += test-build endif -TEST_DEPS += install installcheck +TEST_DEPS += install installcheck check-stale-expected test: $(TEST_DEPS) @if [ -r $(TESTOUT)/regression.diffs ]; then cat $(TESTOUT)/regression.diffs; fi diff --git a/check-stale-expected.sh b/check-stale-expected.sh new file mode 100755 index 0000000..63f532e --- /dev/null +++ b/check-stale-expected.sh @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +# +# check-stale-expected.sh - Catch orphaned test/expected/ files +# +# test/expected/*.out must mirror test/sql/*.sql 1:1 (likewise +# test/build/expected/*.out vs test/build/*.sql, when test-build is in use). +# It's easy to leave a stale .out behind after renaming or removing a .sql +# file; this makes `make test` fail loudly instead of letting it linger +# unnoticed. +# +# test/install/ is NOT checked here: its expected output lives alongside the +# .sql files (test/install/foo.out), not in a separate expected/ +# subdirectory, so there's no 1:1 directory mirror to compare. +# +# pg_regress supports up to 10 alternate expected-output files per test +# (test.out, test_0.out .. test_9.out - see get_alternative_expectfile() in +# pg_regress.c), tried in turn when the primary doesn't match. For each +# expected file we compute the real test base name up front (falling back to +# stripping a trailing _N only when the direct name has no matching .sql), +# so there's a single existence check per file rather than a separately +# tracked found/not-found flag. +# +# Usage: check-stale-expected.sh + +set -o errexit -o errtrace -o pipefail + +BASEDIR=$(dirname "$0") +source "$BASEDIR/lib.sh" + +if [ $# -ne 1 ]; then + die 1 "Usage: check-stale-expected.sh " +fi + +testdir="$1" +failed=0 + +# Usage: check_pair +check_pair() { + local sqldir="$1" expdir="$2" + local f base + + [ -d "$expdir" ] || return 0 + + for f in "$expdir"/*.out; do + [ -f "$f" ] || continue + + base=$(basename "$f" .out) + case "$base" in + *_[0-9]) + [ -f "$sqldir/$base.sql" ] || base=${base%_*} + ;; + esac + + if [ ! -f "$sqldir/$base.sql" ]; then + error "$f has no corresponding $sqldir/$base.sql" + failed=1 + fi + done +} + +check_pair "$testdir/sql" "$testdir/expected" +check_pair "$testdir/build" "$testdir/build/expected" + +exit "$failed" + +# vi: expandtab ts=2 sw=2 From 9d0669d4c0f37a861639e52c84bd0429ad49ad7a Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 26 Jul 2026 15:15:05 -0500 Subject: [PATCH 4/5] Address review round 3: move check-stale-expected.sh to test/bin/, wire 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 #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 --- README.asc | 20 +++++- README.html | 37 +++++++++-- base.mk | 55 ++++++++++++++-- check-stale-expected.sh | 66 -------------------- test/bin/check-stale-expected.sh | 104 +++++++++++++++++++++++++++++++ 5 files changed, 206 insertions(+), 76 deletions(-) delete mode 100755 check-stale-expected.sh create mode 100755 test/bin/check-stale-expected.sh diff --git a/README.asc b/README.asc index 414daa6..c592f2e 100644 --- a/README.asc +++ b/README.asc @@ -636,7 +636,15 @@ Default: `$(TESTDIR)`. Directory `pg_regress` writes actual test output to -- `$ === PGXNTOOL_VERIFY_RESULTS_MODE -Default: `pgtap`. Controls how the <<_verify_results_safeguard,verify-results safeguard>> decides whether tests are failing. `pgtap` scans `test/results/*.out` for pgTap `not ok` lines and plan mismatches (falling back to checking for `regression.diffs` too); `diffs` checks only for `regression.diffs`, matching classic pg_regress behavior. Set to `diffs` if your test suite doesn't use pgTap. +Default: `pgtap`. Controls how the <<_verify_results_safeguard,verify-results safeguard>> decides whether tests are failing. + +==== pgtap + +Scans `test/results/*.out` for pgTap `not ok` lines and plan mismatches, falling back to checking for `regression.diffs` too. Use this mode when your test suite uses pgTap. + +==== diffs + +Checks only for `regression.diffs`, matching classic pg_regress behavior. Use this mode when your tests use plain SQL expected-output comparison only. === PGXNTOOL_ENABLE_TEST_BUILD * @@ -650,6 +658,16 @@ Default: auto-detected -- `yes` if `test/install/*.sql` files exist, `no` otherw Default: `yes`. Enables or disables the <<_verify_results_safeguard,verify-results safeguard>> that blocks `make results` when tests are failing. Setting it to empty on the command line (`make PGXNTOOL_ENABLE_VERIFY_RESULTS= results`) also disables it. +=== PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED * + +Default: `yes`. Enables or disables the check-stale-expected safeguard, which fails `make test` if `test/expected/` (or `test/build/expected/`) contains a `.out` file with no corresponding `.sql` file -- catching a stale file left behind after a test was renamed or removed. Set to `no` to make the check a complete no-op (it's dropped from `TEST_DEPS` entirely). + +This is also the supported pattern for disabling one specific optional pgxntool feature: a documented `PGXNTOOL_ENABLE_*` variable, gating both the target's registration into `TEST_DEPS`/etc. and (where applicable) the target's own definition. Cleanly replacing or overriding an arbitrary pgxntool-generated recipe with your own entirely is a separate, larger, and not-yet-decided design question -- see https://github.com/Postgres-Extensions/pgxntool/issues/30[issue #30] (two proposed approaches) -- and is *not* what this mechanism solves. + +=== PGXNTOOL_CHECK_EXPECTED_FILE_TYPES * + +Default: `yes`. Sub-check of check-stale-expected, independent of `PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED`: fails (with a distinct error message and exit code from the orphaned-`.out` check) if `test/expected/` (or `test/build/expected/`) contains any file that isn't `*.out`. Set to `no` to disable just this sub-check while leaving the orphaned-`.out` check active. + === PGXNTOOL_NO_PGXS_INCLUDE Default: unset (PGXS is included normally). Skips including PGXS (`$(PGXS)`) entirely. This is only for advanced scenarios where you need to manage the PGXS include yourself; most projects should never set this. diff --git a/README.html b/README.html index fcba57d..8926728 100644 --- a/README.html +++ b/README.html @@ -502,7 +502,9 @@

    PGXNtool

  • 8.7. PGXNTOOL_ENABLE_TEST_BUILD *
  • 8.8. PGXNTOOL_ENABLE_TEST_INSTALL *
  • 8.9. PGXNTOOL_ENABLE_VERIFY_RESULTS *
    • -
  • 8.10. PGXNTOOL_NO_PGXS_INCLUDE
    • +
  • 8.10. PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED *
    • +
  • 8.11. PGXNTOOL_CHECK_EXPECTED_FILE_TYPES *
    • +
  • 8.12. PGXNTOOL_NO_PGXS_INCLUDE
  • 9. Copyright
    • @@ -1575,7 +1577,19 @@

    8.6. PGXNTOOL_VERIFY_RESULTS_MODE

    -

    Default: pgtap. Controls how the verify-results safeguard decides whether tests are failing. pgtap scans test/results/*.out for pgTap not ok lines and plan mismatches (falling back to checking for regression.diffs too); diffs checks only for regression.diffs, matching classic pg_regress behavior. Set to diffs if your test suite doesn’t use pgTap.

    +

    Default: pgtap. Controls how the verify-results safeguard decides whether tests are failing.

    +
    +
    +

    8.6.1. pgtap

    +
    +

    Scans test/results/*.out for pgTap not ok lines and plan mismatches, falling back to checking for regression.diffs too. Use this mode when your test suite uses pgTap.

    +
    +
    +
    +

    8.6.2. diffs

    +
    +

    Checks only for regression.diffs, matching classic pg_regress behavior. Use this mode when your tests use plain SQL expected-output comparison only.

    +
    @@ -1597,7 +1611,22 @@

    -

    8.10. PGXNTOOL_NO_PGXS_INCLUDE

    +

    8.10. PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED *

    +
    +

    Default: yes. Enables or disables the check-stale-expected safeguard, which fails make test if test/expected/ (or test/build/expected/) contains a .out file with no corresponding .sql file — catching a stale file left behind after a test was renamed or removed. Set to no to make the check a complete no-op (it’s dropped from TEST_DEPS entirely).

    +
    +
    +

    This is also the supported pattern for disabling one specific optional pgxntool feature: a documented PGXNTOOL_ENABLE_* variable, gating both the target’s registration into TEST_DEPS/etc. and (where applicable) the target’s own definition. Cleanly replacing or overriding an arbitrary pgxntool-generated recipe with your own entirely is a separate, larger, and not-yet-decided design question — see issue #30 (two proposed approaches) — and is not what this mechanism solves.

    +
    +
    +
    +

    8.11. PGXNTOOL_CHECK_EXPECTED_FILE_TYPES *

    +
    +

    Default: yes. Sub-check of check-stale-expected, independent of PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED: fails (with a distinct error message and exit code from the orphaned-.out check) if test/expected/ (or test/build/expected/) contains any file that isn’t *.out. Set to no to disable just this sub-check while leaving the orphaned-.out check active.

    +
    +
    +
    +

    8.12. PGXNTOOL_NO_PGXS_INCLUDE

    Default: unset (PGXS is included normally). Skips including PGXS ($(PGXS)) entirely. This is only for advanced scenarios where you need to manage the PGXS include yourself; most projects should never set this.

    @@ -1618,7 +1647,7 @@

    diff --git a/base.mk b/base.mk index bd387ce..ef3b2dd 100644 --- a/base.mk +++ b/base.mk @@ -191,6 +191,41 @@ endif # Default mode: pgtap (scans results/*.out for TAP failures) PGXNTOOL_VERIFY_RESULTS_MODE ?= pgtap +# ------------------------------------------------------------------------------ +# check-stale-expected: catch orphaned/unexpected test/expected/ files +# ------------------------------------------------------------------------------ +# Variable: PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED +# - Can be set manually in Makefile or command line +# - Allowed values: "yes" or "no" (case-insensitive) +# - Default: "yes" (enabled by default for all pgxntool projects) +# - Set to "no" to make this check a complete no-op: the target is +# dropped from TEST_DEPS entirely (see its own definition below) +# +# Variable: PGXNTOOL_CHECK_EXPECTED_FILE_TYPES +# - Sub-check, independent of the variable above: fails if test/expected/ +# (or test/build/expected/) contains any file that isn't *.out +# - Allowed values: "yes" or "no" (case-insensitive) +# - Default: "yes" +# - Set to "no" to disable just this sub-check while leaving the rest of +# check-stale-expected (the orphaned-.out check) active +# - Passed through to check-stale-expected.sh; see that script for the +# distinct error message/exit code this sub-check uses +# +# Implementation: See check-stale-expected target definition (search for +# "check-stale-expected:" in this file) +# +ifdef PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED + override PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED),PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED) +else + PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED = yes +endif + +ifdef PGXNTOOL_CHECK_EXPECTED_FILE_TYPES + override PGXNTOOL_CHECK_EXPECTED_FILE_TYPES := $(call pgxntool_validate_yesno,$(PGXNTOOL_CHECK_EXPECTED_FILE_TYPES),PGXNTOOL_CHECK_EXPECTED_FILE_TYPES) +else + PGXNTOOL_CHECK_EXPECTED_FILE_TYPES = yes +endif + # Generate unique database name for tests to prevent conflicts across projects # Uses project name + first 5 chars of md5 hash of current directory # This prevents multiple test runs in different directories from clobbering each other @@ -271,8 +306,14 @@ installcheck: $(TEST_RESULT_FILES) $(TEST_SQL_FILES) | $(TESTDIR)/sql/ $(TESTDIR # # These targets are meant to make running tests easier. +# Build test dependencies list based on enabled features. This base +# assignment (a plain `=`, not `+=`) must come before any `TEST_DEPS +=` +# line below -- Make processes the file top-to-bottom, and a later plain +# `=` would silently wipe out any `+=` that came before it. +TEST_DEPS = testdeps + # ------------------------------------------------------------------------------ -# check-stale-expected: catch orphaned test/expected/ files +# check-stale-expected: catch orphaned/unexpected test/expected/ files # ------------------------------------------------------------------------------ # Purpose: test/expected/*.out must mirror test/sql/*.sql 1:1 (likewise # test/build/expected/*.out vs test/build/*.sql, when test-build is in use). @@ -286,9 +327,15 @@ installcheck: $(TEST_RESULT_FILES) $(TEST_SQL_FILES) | $(TESTDIR)/sql/ $(TESTDIR # independent prerequisites of `test`, and Make does not guarantee the order # unrelated prerequisites of the same target are built in. An explicit # dependency edge is the only ordering guarantee Make actually gives. +# +# See PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED / PGXNTOOL_CHECK_EXPECTED_FILE_TYPES +# above for how to disable this entirely or just its non-.out file sub-check. +ifeq ($(PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED),yes) .PHONY: check-stale-expected check-stale-expected: installcheck - @$(PGXNTOOL_DIR)/check-stale-expected.sh $(TESTDIR) + @PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=$(PGXNTOOL_CHECK_EXPECTED_FILE_TYPES) $(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh $(TESTDIR) +TEST_DEPS += check-stale-expected +endif # make test: run any test dependencies, then do a `make install installcheck`. # If regressions are found, it will output them. @@ -297,12 +344,10 @@ check-stale-expected: installcheck # watch-make if you're generating intermediate files. If tests end up needing # clean it's an indication of a missing dependency anyway. .PHONY: test -# Build test dependencies list based on enabled features -TEST_DEPS = testdeps ifeq ($(PGXNTOOL_ENABLE_TEST_BUILD),yes) TEST_DEPS += test-build endif -TEST_DEPS += install installcheck check-stale-expected +TEST_DEPS += install installcheck test: $(TEST_DEPS) @if [ -r $(TESTOUT)/regression.diffs ]; then cat $(TESTOUT)/regression.diffs; fi diff --git a/check-stale-expected.sh b/check-stale-expected.sh deleted file mode 100755 index 63f532e..0000000 --- a/check-stale-expected.sh +++ /dev/null @@ -1,66 +0,0 @@ -#!/usr/bin/env bash -# -# check-stale-expected.sh - Catch orphaned test/expected/ files -# -# test/expected/*.out must mirror test/sql/*.sql 1:1 (likewise -# test/build/expected/*.out vs test/build/*.sql, when test-build is in use). -# It's easy to leave a stale .out behind after renaming or removing a .sql -# file; this makes `make test` fail loudly instead of letting it linger -# unnoticed. -# -# test/install/ is NOT checked here: its expected output lives alongside the -# .sql files (test/install/foo.out), not in a separate expected/ -# subdirectory, so there's no 1:1 directory mirror to compare. -# -# pg_regress supports up to 10 alternate expected-output files per test -# (test.out, test_0.out .. test_9.out - see get_alternative_expectfile() in -# pg_regress.c), tried in turn when the primary doesn't match. For each -# expected file we compute the real test base name up front (falling back to -# stripping a trailing _N only when the direct name has no matching .sql), -# so there's a single existence check per file rather than a separately -# tracked found/not-found flag. -# -# Usage: check-stale-expected.sh - -set -o errexit -o errtrace -o pipefail - -BASEDIR=$(dirname "$0") -source "$BASEDIR/lib.sh" - -if [ $# -ne 1 ]; then - die 1 "Usage: check-stale-expected.sh " -fi - -testdir="$1" -failed=0 - -# Usage: check_pair -check_pair() { - local sqldir="$1" expdir="$2" - local f base - - [ -d "$expdir" ] || return 0 - - for f in "$expdir"/*.out; do - [ -f "$f" ] || continue - - base=$(basename "$f" .out) - case "$base" in - *_[0-9]) - [ -f "$sqldir/$base.sql" ] || base=${base%_*} - ;; - esac - - if [ ! -f "$sqldir/$base.sql" ]; then - error "$f has no corresponding $sqldir/$base.sql" - failed=1 - fi - done -} - -check_pair "$testdir/sql" "$testdir/expected" -check_pair "$testdir/build" "$testdir/build/expected" - -exit "$failed" - -# vi: expandtab ts=2 sw=2 diff --git a/test/bin/check-stale-expected.sh b/test/bin/check-stale-expected.sh new file mode 100755 index 0000000..b35972d --- /dev/null +++ b/test/bin/check-stale-expected.sh @@ -0,0 +1,104 @@ +#!/usr/bin/env bash +# +# check-stale-expected.sh - Catch orphaned/unexpected test/expected/ files +# +# test/expected/*.out must mirror test/sql/*.sql 1:1 (likewise +# test/build/expected/*.out vs test/build/*.sql, when test-build is in use). +# It's easy to leave a stale .out behind after renaming or removing a .sql +# file; this makes `make test` fail loudly instead of letting it linger +# unnoticed. +# +# test/install/ is NOT checked here: its expected output lives alongside the +# .sql files (test/install/foo.out), not in a separate expected/ +# subdirectory, so there's no 1:1 directory mirror to compare. +# +# pg_regress supports up to 10 alternate expected-output files per test +# (test.out, test_0.out .. test_9.out - see get_alternative_expectfile() in +# pg_regress.c), tried in turn when the primary doesn't match. See the case +# block below for how that's recognized and the tradeoff involved. +# +# expected/ is also checked for files that aren't *.out at all -- there's no +# legitimate reason for anything else to live there (stray editor swap +# files, .orig files from a botched merge, etc.). This is a distinct +# failure class from an orphaned .out file: different message, different +# exit code (see below), and independently disable-able via +# PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no. +# +# Exit code is a bitmask so the two failure classes can be told apart: +# 1 - one or more orphaned .out files (no corresponding .sql) +# 2 - one or more unexpected non-.out files in expected/ +# 3 - both +# +# Usage: check-stale-expected.sh +# Env: PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=yes|no (default yes) -- controls +# the non-*.out file check described above. + +set -o errexit -o errtrace -o pipefail + +BASEDIR=$(dirname "$0") +source "$BASEDIR/../../lib.sh" + +if [ $# -ne 1 ]; then + die 1 "Usage: check-stale-expected.sh " +fi + +testdir="$1" +failed=0 + +check_file_types=$(printf '%s' "${PGXNTOOL_CHECK_EXPECTED_FILE_TYPES:-yes}" | tr '[:upper:]' '[:lower:]') + +# Usage: check_pair +check_pair() { + local sqldir="$1" expdir="$2" + local f base + + [ -d "$expdir" ] || return 0 + + for f in "$expdir"/*; do + # Without nullglob, this glob is left as the literal unexpanded pattern + # string when $expdir has no entries at all; skip that non-existent + # "file" rather than treating it as real input. + [ -f "$f" ] || continue + + case "$f" in + *.out) + base=$(basename "$f" .out) + case "$base" in + # pg_regress supports up to 10 alternate expected-output files + # per test (test.out, test_0.out .. test_9.out -- see + # get_alternative_expectfile() in pg_regress.c), so a trailing + # _N here doesn't necessarily mean an orphaned file. Recognizing + # this is pure string matching -- it doesn't require checking + # whether a file exists -- so the suffix is stripped unconditionally, + # leaving exactly one existence check per file (below) rather than + # one to recognize the pattern and a second to validate it. + # Tradeoff: a real test literally named e.g. foo_1.sql (with no + # foo.sql at all) would be misidentified as an alternate file for + # a nonexistent "foo" and incorrectly flagged as stale. Accepted + # as a vanishingly rare edge case. + *_[0-9]) + base=${base%_*} + ;; + esac + + if [ ! -f "$sqldir/$base.sql" ]; then + error "$f has no corresponding $sqldir/$base.sql" + (( failed |= 1 )) + fi + ;; + *) + if [ "$check_file_types" = yes ]; then + error "unexpected non-.out file in $expdir: $f" + (( failed |= 2 )) + fi + ;; + esac + done +} + +check_pair "$testdir/sql" "$testdir/expected" +check_pair "$testdir/build" "$testdir/build/expected" + +exit "$failed" + +# vi: expandtab ts=2 sw=2 From 0fe9dfc5a4f5973cd74a0c3870b2bbae39c1a281 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 27 Jul 2026 16:16:21 -0500 Subject: [PATCH 5/5] Pass PGXNTOOL_CHECK_EXPECTED_FILE_TYPES to check-stale-expected.sh as an arg check-stale-expected.sh now takes the file-type check toggle as a second positional argument ( [check-file-types], default yes) instead of reading PGXNTOOL_CHECK_EXPECTED_FILE_TYPES from its environment. base.mk's check-stale-expected recipe passes the make variable through as that arg. This makes the script easier to unit-test directly: a test loop can vary an argument per-invocation, where an env var has to be set/unset around each call and can leak between cases. Add PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT, a make variable naming exactly which script path the check-stale-expected recipe invokes (default $(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh). This lets a test swap in a throwaway stub script to prove the target does or doesn't invoke it, without needing to fake out all of PGXNTOOL_DIR -- which many unrelated targets (META.json, control.mk, verify-results, pgtle.sh, etc.) also read. Related changes in pgxntool-test: - Add PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no coverage: a stub script, substituted via PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT, proves check-stale-expected.sh is never invoked when disabled (not just that a failure from it is ignored). - Split check-stale-expected's pure script-logic tests (orphan detection, _N.out alternate-file handling, the build/expected pair) out of make-test.bats into a new check-stale-expected-script.bats that invokes the script directly against a scratch directory, no foundation/make/ Postgres required. - Update the remaining check-stale-expected tests in make-test.bats to use the script's new arg-passing form directly, and the make-variable form through `make`. - Document the "narrow the interface, then stub that seam" pattern in CLAUDE.md for future reuse. Co-Authored-By: Claude --- base.mk | 13 ++++++++++++- test/bin/check-stale-expected.sh | 15 +++++++++------ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/base.mk b/base.mk index ef3b2dd..1a419d5 100644 --- a/base.mk +++ b/base.mk @@ -211,6 +211,15 @@ PGXNTOOL_VERIFY_RESULTS_MODE ?= pgtap # - Passed through to check-stale-expected.sh; see that script for the # distinct error message/exit code this sub-check uses # +# Variable: PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT +# - Path to the script the check-stale-expected target invokes +# - Default: $(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh +# - Exists so a test can swap in a stub script (e.g. one that just records +# that it was called) without needing to fake out any other part of +# PGXNTOOL_DIR, which many unrelated targets also read. See +# pgxntool-test's CLAUDE.md, "Proving a Script Was/Wasn't Invoked" +# (under "Testing"), for the pattern this supports. +# # Implementation: See check-stale-expected target definition (search for # "check-stale-expected:" in this file) # @@ -226,6 +235,8 @@ else PGXNTOOL_CHECK_EXPECTED_FILE_TYPES = yes endif +PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT ?= $(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh + # Generate unique database name for tests to prevent conflicts across projects # Uses project name + first 5 chars of md5 hash of current directory # This prevents multiple test runs in different directories from clobbering each other @@ -333,7 +344,7 @@ TEST_DEPS = testdeps ifeq ($(PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED),yes) .PHONY: check-stale-expected check-stale-expected: installcheck - @PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=$(PGXNTOOL_CHECK_EXPECTED_FILE_TYPES) $(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh $(TESTDIR) + @$(PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT) $(TESTDIR) $(PGXNTOOL_CHECK_EXPECTED_FILE_TYPES) TEST_DEPS += check-stale-expected endif diff --git a/test/bin/check-stale-expected.sh b/test/bin/check-stale-expected.sh index b35972d..6a4d221 100755 --- a/test/bin/check-stale-expected.sh +++ b/test/bin/check-stale-expected.sh @@ -29,23 +29,26 @@ # 2 - one or more unexpected non-.out files in expected/ # 3 - both # -# Usage: check-stale-expected.sh -# Env: PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=yes|no (default yes) -- controls -# the non-*.out file check described above. +# Usage: check-stale-expected.sh [check-file-types] +# check-file-types: yes|no (default yes) -- controls the non-*.out +# file check described above. Taken as a positional argument (rather +# than an environment variable) so it's easy to vary directly in a +# test loop instead of having to set/unset an env var around each +# invocation. set -o errexit -o errtrace -o pipefail BASEDIR=$(dirname "$0") source "$BASEDIR/../../lib.sh" -if [ $# -ne 1 ]; then - die 1 "Usage: check-stale-expected.sh " +if [ $# -lt 1 ] || [ $# -gt 2 ]; then + die 1 "Usage: check-stale-expected.sh [check-file-types]" fi testdir="$1" failed=0 -check_file_types=$(printf '%s' "${PGXNTOOL_CHECK_EXPECTED_FILE_TYPES:-yes}" | tr '[:upper:]' '[:lower:]') +check_file_types=$(printf '%s' "${2:-yes}" | tr '[:upper:]' '[:lower:]') # Usage: check_pair check_pair() {