diff --git a/.claude/development.md b/.claude/development.md new file mode 100644 index 0000000..579a95b --- /dev/null +++ b/.claude/development.md @@ -0,0 +1,48 @@ +# pgxntool Development Guidelines + +**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 + +**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 `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 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/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 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 <