Repository navigation
Fix base.mk recipe loops that ignore a failed iteration - #142
Draft
jnasbyupgrade wants to merge 4 commits into
Draft
jnasbyupgrade wants to merge 4 commits into
jnasbyupgrade wants to merge 4 commits into
Conversation
The `pgtle` recipe joined its per-extension `pgtle.sh` calls with `;`, so the target's exit status was that of the last call only. Chain them with `&&` (ending in `true`) so a failure for any extension fails the target. Fixes Postgres-Extensions#140 Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`results`, `results-build`, `print-pgtle` and the test/install schedule rule each run one command per file or extension inside a shell loop. A loop's status is only its last command's, so a failure partway through was ignored. `results-build` also discarded the last copy's status, behind its trailing skipped-file check. Each command now ends in `|| exit 1`, matching the `make pgtle` fix. The HISTORY.asc entry for Postgres-Extensions#140 now covers every affected target.
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool-test
that referenced
this pull request
Oct 14, 2026
Add two tests to test/standard/make-test.bats that make a `cp` into the expected-output directory fail by putting a directory where cp writes, which fails even as root. The `make results` test makes a copy other than the last one fail, and runs `make -o verify-results results` so only the copy loop runs. The `results-build` test runs the real target. Without the fix, both targets exit 0 after the failed copy. Covers Postgres-Extensions/pgxntool#140; needs the recipe fix in Postgres-Extensions/pgxntool#142.
make pgtle ignoring failures for all but the last extensionThe lead sentence said a failure of any command but the last was ignored, but the results-build entry covers the last command too.
`pgtle` now calls `pgtle.sh` once per extension inside a shell `for` loop, with `|| exit 1` on each call. That is the form the `results`, `results-build` and test/install schedule loops already use. It still stops at the first extension that fails. The calls, their arguments and order, and make's exit status are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five
base.mktargets run one command per extension or per file, and a failed command was ignored unless it was the last one, somakeexited 0 after a real failure. They now fail as soon as any one command fails.pgtlepgtle.sh --extension <ext>resultscp test/results/<name>.out test/expected/<name>.outresults-buildcp test/build/results/<name>.out test/build/expected/<name>.outprint-pgtlecat pg_tle/<version>/<ext>.sqltest/install/schedule(withPGXNTOOL_ENABLE_TEST_INSTALL=yes)echo "test: ../install/<name>" >> test/install/schedulepgtlejoined its$(foreach ...)calls with;. It now runs them in a shellforloop over the extensions, with|| exit 1on each call.make -n pgtlein a two-extension project:PGXNTOOL_PGTLE_VERSIONhandling is unchanged (--pgtle-version 1.5.0+is still appended to each call when set).The other four targets put
|| exit 1on the command inside the loop.Tests
Postgres-Extensions/pgxntool-test#108 adds regression tests for
pgtle,resultsandresults-build. Each forces a failure by putting a directory where a file would be written, which fails even when the tests run as root. Forresults, the failing copy is not the last one.pgtlewas also checked by hand in a scratch two-extension project, replacing one extension'spg_tle/1.5.0+/<ext>.sqloutput with a directory:PGXNTOOL_PGTLE_VERSION=1.5.0+print-pgtleand the install schedule have no regression test, because neither failure can be forced portably:print-pgtlealready rejects a missing or non-regular file beforecatruns. A test would needcatto fail on a regular file, which means a read error. Checked by hand on Linux, in a scratch two-extension project, by symlinking the first extension'spg_tle/1.5.0+/<ext>.sqlto/proc/self/mem. That path is a regular file whose read fails withInput/output error.make -o pgtle print-pgtle PGXNTOOL_PGTLE_TARGET_VERSION=1.5.2exited 0 before this change and exits 2 after it./procis Linux-only, so this isn't a committed test.>. Anything a test can do to that path also makes that first write fail, andmakealready stops there. Only a write error partway through the loop, such as a full disk, reaches the ignored case.Fixes #140