diff --git a/.claude/development.md b/.claude/development.md new file mode 100644 index 0000000..886484a --- /dev/null +++ b/.claude/development.md @@ -0,0 +1,47 @@ +# 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) + +`debug LEVEL "msg"` prints when `DEBUG >= LEVEL`. LEVEL encodes how noisy/esoteric a message is — how far you'd crank `DEBUG` before you'd want to see it — **not** code nesting depth. A top-level line can warrant a high level if it's esoteric, and loop-body detail is usually high precisely because it's noisy. Judge by signal-to-noise. + +The tiers are anchors, not strict multiples of 10 — any value in range is fine, leaving room to fine-tune between existing calls without renumbering: + +- **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 (per-iteration innards) + +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 e5d701a..75c9d16 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,10 +18,17 @@ may pick up the wrong run. `--commit SHA` targets the exact push and avoids it. ## 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 98ea2a3..66384e2 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -7,6 +7,15 @@ re-run `git init`, and the sync scripts aborted with "Not in a git repository." They now detect the repo with `git rev-parse --git-dir`, which works in both a normal clone and a worktree. +== 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.3 ----- == Fix pgxntool-sync remote, and make it runnable without make diff --git a/base.mk b/base.mk index fc9923e..a7ab93c 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,14 @@ 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! +# +# Dependency chain (verify-results: test) guarantees test completes before verify-results +# checks regression.diffs, even under make -j. Listing both as independent prerequisites +# of results would allow them to run concurrently, letting verify-results see stale state. .PHONY: results ifeq ($(PGXNTOOL_ENABLE_VERIFY_RESULTS),yes) -results: verify-results test +verify-results: test +results: verify-results else results: test endif diff --git a/lib.sh b/lib.sh index 853dc84..3cbeb0e 100644 --- a/lib.sh +++ b/lib.sh @@ -60,12 +60,21 @@ 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 +# +# LEVEL encodes how noisy/esoteric a message is -- roughly, how far you'd crank +# DEBUG before you'd actually want to see it. Higher = noisier, more rarely +# useful. This is signal-to-noise, NOT code nesting depth: a top-level line can +# warrant a high level if it's esoteric, and loop-body detail is usually high +# precisely because it's noisy. +# +# The tiers below are anchors, not strict multiples -- pick any value in range +# to fine-tune between existing calls without renumbering: # - 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 +# - 50+: Maximum verbosity (per-iteration innards) +# # Enable with: DEBUG=30 scriptname.sh debug() { local level=$1 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 <