From 20c2552ade446861972f27c9e56af331c167cdd5 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 14 May 2026 15:45:20 -0500 Subject: [PATCH 1/4] Fix results ordering, control file whitespace, ENABLE_* override, debug levels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix `results:` ordering: `verify-results` ran before `make test`, so it checked stale `regression.diffs`; reordered to `results: test verify-results` - Fix `parse_control_file` trailing whitespace: `${value%% }` only removed one space; now uses regex to strip all trailing spaces and tabs - Add `override` to `PGXNTOOL_ENABLE_TEST_BUILD`/`PGXNTOOL_ENABLE_TEST_INSTALL` so command-line values are normalized by `pgxntool_validate_yesno` rather than silently ignored - Fix `pgtle.sh` debug levels: single-digit values (1–4) replaced with standard multiples-of-10 (30, 40, 50) per documented convention - Add Makefile variable assignment and debug level rules to `CLAUDE.md` Related changes in pgxntool-test: - New test in `04-pgtle.bats`: verify `pgtle.sh` handles multiple spaces and tab before trailing comment in control files - Updated `make-results.bats`: new test verifying `verify-results` blocks `make results` after a failing test run - Updated `test-verify-results.bats`: reflect that `make results` now runs `make test` first Co-Authored-By: Claude --- CLAUDE.md | 24 ++++++++++++++++++++++++ HISTORY.asc | 11 +++++++++++ base.mk | 13 ++++++++++--- pgtle.sh | 42 +++++++++++++++++++++++++----------------- 4 files changed, 70 insertions(+), 20 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index cfa7b8d..13e39ce 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -239,6 +239,30 @@ Generated files depend on: - No C code support (pg_tle requires trusted languages only) - PostgreSQL 14.5+ required (pg_tle not available on earlier versions) +## Makefile Variable Assignment Rules + +**RULE: Do not use `:=` (simply expanded) unless you have a specific need for immediate evaluation.** + +Use `=` (recursively expanded) for standard variable assignments. Reserve `:=` for cases where the right-hand side must be evaluated exactly once at assignment time — for example, when assigning the result of a `$(call ...)` function that references the variable being set (which would cause infinite recursion with `=`). + +When a variable must also override command-line values, combine `override` with `:=` — but only where `override` is genuinely needed. + +## Debug Level Rules (lib.sh `debug` function) + +The `debug` function in `lib.sh` uses a numeric level scale. All debug calls in pgxntool scripts must use levels from one of these two groups: + +- **Single-digit (1–9)**: Reserved for the rarest, highest-value messages. Use sparingly. +- **Multiples of 10 (10, 20, 30, 40, 50, ...)**: The standard scale: + - **10**: Critical errors, important warnings + - **20**: Significant state changes, warnings + - **30**: General debugging — function entry/exit, key state summaries + - **40**: Verbose details — array resets, internal state checks + - **50+**: Maximum verbosity — loop iterations (per-item in a loop) + +**Forbidden**: Multi-digit levels that are not multiples of 10 (e.g., 11, 22, 25). These break the fine-tuning purpose of the scale. The commit skill sanity-checks for this. + +Note: The BATS test helper `debug` function (in `test/lib/helpers.bash`) uses a separate 1–5 scale. The two systems are independent. + ## Critical Gotchas 1. **Empty Variables**: If `DOCS` or `MODULES` is empty, base.mk sets to empty to prevent PGXS errors diff --git a/HISTORY.asc b/HISTORY.asc index 11e351c..459bdaf 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -1,3 +1,14 @@ +STABLE +------ +== Fix `verify-results` checking stale results in `make results` +`make results` ran `verify-results` before `make test`, so it checked stale +`regression.diffs` from a prior run. Reordered so `verify-results` always +checks the fresh results. + +== Fix `PGXNTOOL_ENABLE_TEST_BUILD`/`PGXNTOOL_ENABLE_TEST_INSTALL` ignoring command-line values +Without `override`, the `pgxntool_validate_yesno` normalization was silently +skipped when these variables were set on the command line. + 2.0.2 ----- == Fix parse_control_file corrupting values with trailing comments diff --git a/base.mk b/base.mk index edbd6cd..f1df39b 100644 --- a/base.mk +++ b/base.mk @@ -93,7 +93,9 @@ pgxntool_validate_yesno = $(strip \ TEST_BUILD_SQL_FILES = $(wildcard $(TESTDIR)/build/*.sql) TEST_BUILD_FILES = $(TEST_BUILD_SQL_FILES) ifdef PGXNTOOL_ENABLE_TEST_BUILD - PGXNTOOL_ENABLE_TEST_BUILD := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_TEST_BUILD),PGXNTOOL_ENABLE_TEST_BUILD) + # override needed so command-line values (make VAR=YES) are normalized, not silently ignored. + # := needed for immediate evaluation of the function call (avoids infinite recursion with =). + override PGXNTOOL_ENABLE_TEST_BUILD := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_TEST_BUILD),PGXNTOOL_ENABLE_TEST_BUILD) else # Auto-detect: enable if test/build/ directory has SQL files ifneq ($(strip $(TEST_BUILD_FILES)),) @@ -135,7 +137,9 @@ endif # Either approach would eliminate the ~10-line block repeated for each feature. TEST_INSTALL_SQL_FILES = $(wildcard $(TESTDIR)/install/*.sql) ifdef PGXNTOOL_ENABLE_TEST_INSTALL - PGXNTOOL_ENABLE_TEST_INSTALL := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_TEST_INSTALL),PGXNTOOL_ENABLE_TEST_INSTALL) + # override needed so command-line values (make VAR=YES) are normalized, not silently ignored. + # := needed for immediate evaluation of the function call (avoids infinite recursion with =). + override PGXNTOOL_ENABLE_TEST_INSTALL := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_TEST_INSTALL),PGXNTOOL_ENABLE_TEST_INSTALL) else # Auto-detect: enable if test/install/ directory has SQL files ifneq ($(strip $(TEST_INSTALL_SQL_FILES)),) @@ -292,9 +296,12 @@ endif # make results: runs `make test` and copies all result files to expected. # DO NOT RUN THIS UNLESS YOU'RE CERTAIN ALL YOUR TESTS ARE PASSING! +# verify-results runs AFTER test (not before) so it checks the fresh regression.diffs +# from this run, not stale results from a prior run. The old order (verify-results test) +# checked pre-run state, which would miss failures introduced by the re-run itself. .PHONY: results ifeq ($(PGXNTOOL_ENABLE_VERIFY_RESULTS),yes) -results: verify-results test +results: test verify-results else results: test endif diff --git a/pgtle.sh b/pgtle.sh index 3b948e8..124ca84 100755 --- a/pgtle.sh +++ b/pgtle.sh @@ -156,7 +156,7 @@ MODULE_PATHNAME="" VERSION_FILES=() UPGRADE_FILES=() -debug 1 "Global arrays initialized: VERSION_FILES=${#VERSION_FILES[@]}, UPGRADE_FILES=${#UPGRADE_FILES[@]}" +debug 30 "Global arrays initialized: VERSION_FILES=${#VERSION_FILES[@]}, UPGRADE_FILES=${#UPGRADE_FILES[@]}" PGTLE_VERSION="" # Empty = generate all GET_DIR_VERSION="" # For --get-dir option @@ -463,7 +463,15 @@ parse_control_file() { # Order matters: stripping quotes before removing comments leaves a rogue # trailing quote for values like 'version' # note. See issue #25. value="${value%%#*}" # Remove trailing comments - value="${value%% }" # Trim trailing whitespace + # Trim all trailing whitespace (spaces and tabs). The prior %% pattern with + # a literal space only removed one character; multiple spaces or a tab before + # the comment (e.g., 'value' # note or 'value'$'\t'# note) would leave + # stray whitespace that breaks the quote-strip below. + if [[ "$value" =~ ^(.*[^[:space:]])[[:space:]]*$ ]]; then + value="${BASH_REMATCH[1]}" + else + value="" + fi # Strip quotes if present (both single and double) value="${value#\'}" @@ -513,21 +521,21 @@ parse_control_file() { discover_sql_files() { echo "Discovering SQL files for extension: $EXTENSION" >&2 - debug 2 "discover_sql_files: Starting discovery for extension: $EXTENSION" + debug 30 "discover_sql_files: Starting discovery for extension: $EXTENSION" # Ensure default_version file exists and has content if base file exists # This handles the case where make all hasn't generated it yet, or it exists but is empty local default_version_file="sql/${EXTENSION}--${DEFAULT_VERSION}.sql" local base_file="sql/${EXTENSION}.sql" if [ -f "$base_file" ] && ([ ! -f "$default_version_file" ] || [ ! -s "$default_version_file" ]); then - debug 3 "discover_sql_files: Creating default_version file from base file" + debug 40 "discover_sql_files: Creating default_version file from base file" cp "$base_file" "$default_version_file" fi # Find versioned files: sql/{ext}--{version}.sql # Use find to get proper null-delimited output, then filter out upgrade scripts VERSION_FILES=() # Reset array - debug 3 "discover_sql_files: Reset VERSION_FILES array" + debug 40 "discover_sql_files: Reset VERSION_FILES array" while IFS= read -r -d '' file; do local basename=$(basename "$file" .sql) local dash_count=$(echo "$basename" | grep -o -- "--" | wc -l | tr -d '[:space:]') @@ -545,7 +553,7 @@ discover_sql_files() { # Find upgrade scripts: sql/{ext}--{ver1}--{ver2}.sql # These have TWO occurrences of "--" in the filename UPGRADE_FILES=() # Reset array - debug 3 "discover_sql_files: Reset UPGRADE_FILES array" + debug 40 "discover_sql_files: Reset UPGRADE_FILES array" while IFS= read -r -d '' file; do # Empty upgrade files are allowed (no-op upgrades) local basename=$(basename "$file" .sql) @@ -566,15 +574,15 @@ discover_sql_files() { echo " - $f" >&2 done - debug 3 "discover_sql_files: Checking UPGRADE_FILES array, count=${#UPGRADE_FILES[@]}" + debug 40 "discover_sql_files: Checking UPGRADE_FILES array, count=${#UPGRADE_FILES[@]}" if array_not_empty "${#UPGRADE_FILES[@]}"; then echo " Found ${#UPGRADE_FILES[@]} upgrade script(s):" >&2 - debug 2 "discover_sql_files: Iterating over ${#UPGRADE_FILES[@]} upgrade files" + debug 30 "discover_sql_files: Iterating over ${#UPGRADE_FILES[@]} upgrade files" for f in "${UPGRADE_FILES[@]}"; do echo " - $f" >&2 done else - debug 2 "discover_sql_files: No upgrade files found" + debug 30 "discover_sql_files: No upgrade files found" fi } @@ -752,7 +760,7 @@ generate_install_update_path() { generate_pgtle_sql() { local pgtle_version="$1" - debug 2 "generate_pgtle_sql: Starting for version $pgtle_version, extension $EXTENSION" + debug 30 "generate_pgtle_sql: Starting for version $pgtle_version, extension $EXTENSION" # Get capability using function (compatible with bash < 4.0) local capability=$(get_pgtle_capability "$pgtle_version") @@ -761,9 +769,9 @@ generate_pgtle_sql() { # Ensure arrays are initialized (defensive programming) # Arrays should already be initialized at top level, but ensure they exist - debug 3 "generate_pgtle_sql: Checking array initialization" - debug 2 "generate_pgtle_sql: VERSION_FILES is ${VERSION_FILES+set}, count=${#VERSION_FILES[@]}" - debug 2 "generate_pgtle_sql: UPGRADE_FILES is ${UPGRADE_FILES+set}, count=${#UPGRADE_FILES[@]}" + debug 40 "generate_pgtle_sql: Checking array initialization" + debug 30 "generate_pgtle_sql: VERSION_FILES is ${VERSION_FILES+set}, count=${#VERSION_FILES[@]}" + debug 30 "generate_pgtle_sql: UPGRADE_FILES is ${UPGRADE_FILES+set}, count=${#UPGRADE_FILES[@]}" if [ -z "${VERSION_FILES+set}" ]; then echo "WARNING: VERSION_FILES not set, initializing" >&2 @@ -822,17 +830,17 @@ EOF # Install all upgrade paths local upgrade_count=${#UPGRADE_FILES[@]} - debug 3 "generate_pgtle_sql: upgrade_count=$upgrade_count" + debug 40 "generate_pgtle_sql: upgrade_count=$upgrade_count" if [ "$upgrade_count" -gt 0 ]; then - debug 2 "generate_pgtle_sql: Processing $upgrade_count upgrade path(s)" + debug 30 "generate_pgtle_sql: Processing $upgrade_count upgrade path(s)" local i=0 while [ "$i" -lt "$upgrade_count" ]; do - debug 4 "generate_pgtle_sql: Processing upgrade file $i: ${UPGRADE_FILES[$i]}" + debug 50 "generate_pgtle_sql: Processing upgrade file $i: ${UPGRADE_FILES[$i]}" generate_install_update_path "${UPGRADE_FILES[$i]}" i=$((i + 1)) done else - debug 2 "generate_pgtle_sql: No upgrade paths to process" + debug 30 "generate_pgtle_sql: No upgrade paths to process" fi cat < Date: Fri, 15 May 2026 14:45:37 -0500 Subject: [PATCH 2/4] Clarify debug() level scheme: use ranges, not multiples of 10 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update the `debug()` comment in `lib.sh` to document the intended range-based scheme: 1-9 for high-level script context, 10-19 for per-loop-iteration detail, 20-29 for nesting inside that — with room within each decade to fine-tune without renumbering. The old comment described "multiples of 10" which was inaccurate and misleading. Related changes in pgxntool-test: - `helpers.bash`: test `debug()` now uses `$TESTDEBUG` instead of `$DEBUG` - `SKILL.md`: added advisory debug level distribution check to commit workflow Co-Authored-By: Claude --- lib.sh | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/lib.sh b/lib.sh index ea38665..0979acf 100644 --- a/lib.sh +++ b/lib.sh @@ -60,13 +60,18 @@ array_not_empty() { # Debug function # Usage: debug LEVEL "message" # Outputs message to stderr if DEBUG >= LEVEL -# Debug levels use multiples of 10 (10, 20, 30, 40, etc.) to allow for easy expansion -# - 10: Critical errors, important warnings -# - 20: Warnings, significant state changes -# - 30: General debugging, function entry/exit, array operations -# - 40: Verbose details, loop iterations -# - 50+: Maximum verbosity -# Enable with: DEBUG=30 scriptname.sh +# +# Debug levels use ranges, not strict multiples, to leave room for fine-tuning +# without renumbering. The order of magnitude gives a rough sense of verbosity: +# - 1-9: High-level script context: arguments (1), top-level entry/exit (2-3), +# lower-level helper calls (4-5) +# - 10-19: Per-iteration detail inside loops: exit+status (10), entry (11), +# intermediate results (15) +# - 20-29: Nesting inside loop-level detail +# - Higher ranges follow the same pattern +# +# A non-trivial script with loops should have debug calls in multiple ranges. +# Enable with: DEBUG=15 scriptname.sh debug() { local level=$1 shift From 86b57440ff4d93292772ec112ceed465fb480f81 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 15 May 2026 15:15:21 -0500 Subject: [PATCH 3/4] Move developer guidelines out of CLAUDE.md into .claude/development.md CLAUDE.md is for users of pgxntool. Developer-focused guidelines (Makefile variable assignment rules, debug level scheme) belong in .claude/ where they are visible to contributors but not surfaced to extension developers reading the user docs. Also updates the debug level rules to the range-based scheme documented in lib.sh (1-9, 10-19, 20-29...) rather than the old multiples-of-10 description. Co-Authored-By: Claude --- .claude/development.md | 27 +++++++++++++++++++++++++++ CLAUDE.md | 24 ------------------------ 2 files changed, 27 insertions(+), 24 deletions(-) create mode 100644 .claude/development.md diff --git a/.claude/development.md b/.claude/development.md new file mode 100644 index 0000000..356ad84 --- /dev/null +++ b/.claude/development.md @@ -0,0 +1,27 @@ +# pgxntool Development Guidelines + +Guidelines for contributors working on pgxntool itself. This file is loaded by +Claude Code when working in the pgxntool repository. + +## Makefile Variable Assignment Rules + +**RULE: Do not use `:=` (simply expanded) unless you have a specific need for immediate evaluation.** + +Use `=` (recursively expanded) for standard variable assignments. Reserve `:=` for cases where the right-hand side must be evaluated exactly once at assignment time — for example, when assigning the result of a `$(call ...)` function that references the variable being set (which would cause infinite recursion with `=`). + +When a variable must also override command-line values, combine `override` with `:=` — but only where `override` is genuinely needed. + +## Debug Level Rules (lib.sh `debug` function) + +The `debug` function in `lib.sh` uses a range-based numeric level scheme. The order of magnitude gives a rough sense of verbosity; room within each decade allows fine-tuning without renumbering. + +- **1–9**: High-level script context — script arguments (1), top-level entry/exit (2–3), lower-level helper calls (4–5). Use sparingly. +- **10–19**: Per-loop-iteration detail — exit with status (10), entry (11), intermediate results (15). +- **20–29**: Nesting inside loop-level detail. +- Higher ranges follow the same pattern. + +A non-trivial script with loops should have debug calls spanning multiple ranges. + +The commit skill checks for unusual distributions (e.g., a script with loops that only uses single-digit levels). + +Note: The BATS test helper `debug` function (in `test/lib/helpers.bash`) uses a separate 1–5 scale controlled by `$TESTDEBUG`. The two systems are independent. diff --git a/CLAUDE.md b/CLAUDE.md index 13e39ce..cfa7b8d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -239,30 +239,6 @@ Generated files depend on: - No C code support (pg_tle requires trusted languages only) - PostgreSQL 14.5+ required (pg_tle not available on earlier versions) -## Makefile Variable Assignment Rules - -**RULE: Do not use `:=` (simply expanded) unless you have a specific need for immediate evaluation.** - -Use `=` (recursively expanded) for standard variable assignments. Reserve `:=` for cases where the right-hand side must be evaluated exactly once at assignment time — for example, when assigning the result of a `$(call ...)` function that references the variable being set (which would cause infinite recursion with `=`). - -When a variable must also override command-line values, combine `override` with `:=` — but only where `override` is genuinely needed. - -## Debug Level Rules (lib.sh `debug` function) - -The `debug` function in `lib.sh` uses a numeric level scale. All debug calls in pgxntool scripts must use levels from one of these two groups: - -- **Single-digit (1–9)**: Reserved for the rarest, highest-value messages. Use sparingly. -- **Multiples of 10 (10, 20, 30, 40, 50, ...)**: The standard scale: - - **10**: Critical errors, important warnings - - **20**: Significant state changes, warnings - - **30**: General debugging — function entry/exit, key state summaries - - **40**: Verbose details — array resets, internal state checks - - **50+**: Maximum verbosity — loop iterations (per-item in a loop) - -**Forbidden**: Multi-digit levels that are not multiples of 10 (e.g., 11, 22, 25). These break the fine-tuning purpose of the scale. The commit skill sanity-checks for this. - -Note: The BATS test helper `debug` function (in `test/lib/helpers.bash`) uses a separate 1–5 scale. The two systems are independent. - ## Critical Gotchas 1. **Empty Variables**: If `DOCS` or `MODULES` is empty, base.mk sets to empty to prevent PGXS errors From 054925cb7a6a3915a593ac20964559987380e8c2 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 15 May 2026 15:18:39 -0500 Subject: [PATCH 4/4] Clarify audience: CLAUDE.md for users, .claude/ for developers CLAUDE.md is loaded by anyone who has pgxntool embedded in their project. .claude/ is only relevant to people making changes to pgxntool itself. - CLAUDE.md: rewrite "Scope" section to make clear it is for extension developers consuming pgxntool, not for pgxntool contributors - .claude/development.md: add prominent "developers only" header and a "Work from pgxntool-test, Not Here" section explaining why changes must be made from a pgxntool-test checkout (full test infrastructure lives there) - Fix test helper path reference: tests/lib/helpers.bash in pgxntool-test Co-Authored-By: Claude Sonnet 4.6 --- .claude/development.md | 27 ++++++++++++++++++++++++--- CLAUDE.md | 15 +++++++++++---- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/.claude/development.md b/.claude/development.md index 356ad84..579a95b 100644 --- a/.claude/development.md +++ b/.claude/development.md @@ -1,7 +1,28 @@ # pgxntool Development Guidelines -Guidelines for contributors working on pgxntool itself. This file is loaded by -Claude Code when working in the pgxntool repository. +**THIS FILE IS FOR PGXNTOOL DEVELOPERS ONLY.** + +If you are an extension developer using pgxntool in your project, this file does not +apply to you. See the top-level `CLAUDE.md` instead. + +## Critical: Work from pgxntool-test, Not Here + +**NEVER make changes to pgxntool directly from this repository.** + +pgxntool development must be done from a checkout of **pgxntool-test**, which contains +the full test infrastructure. Working here directly means you cannot run tests, and +any changes you commit cannot be validated before merging. + +**Correct workflow:** +1. Clone or use an existing checkout of `pgxntool-test` +2. Work in a worktree: both `pgxntool/` and `pgxntool-test/` will be siblings +3. Make changes to `pgxntool/` from within that pgxntool-test context +4. Run the test suite via `make test` in pgxntool-test before committing + +**See:** https://github.com/Postgres-Extensions/pgxntool-test for the full development +workflow. + +--- ## Makefile Variable Assignment Rules @@ -24,4 +45,4 @@ A non-trivial script with loops should have debug calls spanning multiple ranges The commit skill checks for unusual distributions (e.g., a script with loops that only uses single-digit levels). -Note: The BATS test helper `debug` function (in `test/lib/helpers.bash`) uses a separate 1–5 scale controlled by `$TESTDEBUG`. The two systems are independent. +Note: The BATS test helper `debug` function (in `tests/lib/helpers.bash` in pgxntool-test) uses a separate 1–5 scale controlled by `$TESTDEBUG`. The two systems are independent. diff --git a/CLAUDE.md b/CLAUDE.md index cfa7b8d..96d2340 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,10 +4,17 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ## Scope of This File -The guidance in this CLAUDE.md applies to pgxntool's own source files and provides -recommended best practices for projects using pgxntool. However, any agent working in -an extension project should always defer to that project's own CLAUDE.md and -instructions over anything stated here. +**CLAUDE.md is for people USING pgxntool** — extension developers who have embedded +pgxntool into their project via `git subtree`. It documents the build system, available +commands, and how pgxntool works. + +**If you are making changes to pgxntool itself**, stop — you are in the wrong place. +See `.claude/` in this directory for developer guidelines. More importantly, pgxntool +development must be done from the **pgxntool-test** repository, not from here. See the +`Development Workflow` section below. + +Any agent working in an extension project should always defer to that project's own +CLAUDE.md and instructions over anything stated here. ## Git Commit Guidelines