STF-1287: Hand the extension release off to CI - #266
Conversation
The extension repository now builds and publishes its own release assets from a workflow that runs on a pushed tag, so the release script no longer compiles and packages them on the releaser's workstation. It bumps the submodule and pushes an annotated tag, and the workflow does the rest. The tag is annotated because the workflow creates the release with --notes-from-tag. The tag push is no longer inside the branch that commits the submodule bump. It was only reached when that commit was needed, so a re-run, or a submodule bump made by hand, produced no extension release at all and said nothing about it. Pushing a tag that already exists now fails with an explanation rather than a git error. Also fix the substitution for the ext-maxminddb conflict constraint in composer.json. Its lookahead required a comma after the version, but the constraint reads "<1.11.1 || >=2.0.0", so it had matched nothing since the constraint gained its upper bound in December 2023. perl -pi does not fail on a non-match, so the constraint quietly stopped tracking releases.
42f0a4a to
8c39521
Compare
horgh
left a comment
There was a problem hiding this comment.
It looks reasonable to me. Claude had a few comments.
| # -f replaces a tag left behind by an earlier failed run rather than aborting; | ||
| # the check above has already established that it was never pushed. | ||
| echo "Tagging $tag in extension repository..." | ||
| git tag -f -a "$tag" -m "$notes" |
There was a problem hiding this comment.
The annotated tag cannot deliver the release notes — the whole rationale for git tag -a fails today.
gh reads a tag annotation from a local clone, not the API. Verified directly:
$ gh release create v0.0.0-notreal --notes-from-tag # outside a git repo
cannot generate release notes from tag v0.0.0-notreal as it does not exist locally
In maxmind/MaxMind-DB-Reader-php-ext#2, the draft-release job (release.yml:188-235) is the only job with no actions/checkout — every other one has ref: ${{ needs.setup.outputs.checkout-ref }}. So --notes-from-tag always fails there and the workflow's own fallback fires:
echo "::warning::--notes-from-tag failed; falling back to generated notes."
gh release create "$TAG" --draft --title "$title" --generate-notesNet effect: every extension release gets GitHub's generated commit list instead of the CHANGELOG notes, announced only as a ::warning:: in a log nobody reads. The fix belongs in the ext PR (add a checkout with ref: $TAG, or pass --notes-file), but this PR shouldn't ship a comment asserting behaviour that never holds.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Confirmed, and fixed — but in the extension PR, as you say it should be.
draft-release was the only job in that workflow with no actions/checkout, so
--notes-from-tag failed on every release and the fallback fired every time.
It now checks the tag out.
The comment here is reworded too: it no longer asserts behaviour that never held.
Thank you for testing it outside a git repository rather than reading the flag's
documentation — the error message is the whole proof, and it is not something the
docs make obvious.
🤖 Claude, replying on behalf of Greg.
| # Clean up tarball | ||
| rm -f "$pie_tarball" | ||
| echo "Pushing tag $tag..." | ||
| git push origin "refs/tags/$tag" |
There was a problem hiding this comment.
Merge order is unmet, unguarded, and unrecoverable.
.github/workflows/ on the ext repo's main currently contains only links.yml — release.yml exists solely on maxmind/MaxMind-DB-Reader-php-ext#2, still open. If this merges first and someone cuts a release, this push triggers nothing, yet lines 254-256 print ✓ Its release workflow will build and publish the release assets and the script exits 0 at RELEASE COMPLETE.
The guard at 232-242 then permanently blocks a retry, and the recovery it prints (gh workflow run release.yml) also fails, because workflow_dispatch resolves the workflow from the default branch. The recovery path is as absent as the build path.
The PR body documents the ordering, but nothing enforces it. Lines 5-39 are entirely pre-flight checks; this belongs there:
if ! gh workflow view release.yml --repo maxmind/MaxMind-DB-Reader-php-ext &>/dev/null; then
echo "Error: release.yml is not on the extension repo's default branch;"
echo "pushing a tag would build nothing. Nothing has been published yet."
exit 1
fi🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed, and put where you suggested — with the pre-flight checks, before anything
is published:
if ! gh workflow view release.yml --repo maxmind/MaxMind-DB-Reader-php-ext &>/dev/null; thenYour second paragraph is what makes it worth a hard failure rather than a
warning: the tag guard then blocks every retry, and the gh workflow run it
recommends also resolves the workflow from the default branch, so the recovery
path is as absent as the build path. That is unrecoverable state produced by a
green run.
🤖 Claude, replying on behalf of Greg.
| popd >/dev/null | ||
| exit 1 | ||
| fi | ||
| # The tag must be annotated: the workflow creates the release with |
There was a problem hiding this comment.
"The tag must be annotated" overstates a soft requirement.
release.yml:229-233 in the ext PR falls back rather than failing:
if ! gh release create "$TAG" --draft --title "$title" --notes-from-tag; then
echo "::warning::--notes-from-tag failed; falling back to generated notes."
gh release create "$TAG" --draft --title "$title" --generate-notes
fiA lightweight tag does not break the release; it downgrades the notes behind a warning. This is the same overstatement pattern 79bdc97 was correcting. Suggested: "Annotate the tag: the workflow passes --notes-from-tag when it creates the draft, so the annotation becomes the release notes. A lightweight tag still releases, but falls back to generated notes."
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed. That comment is gone in the rewrite of this block — the annotation is now
described as what becomes the release notes, without claiming the tag must be
annotated, and the code decides up front with git cat-file -t rather than
catching a failure.
You are right that it is the same overstatement pattern 79bdc97 was correcting.
🤖 Claude, replying on behalf of Greg.
| echo " published the release with all of its assets:" | ||
| echo " https://github.com/maxmind/MaxMind-DB-Reader-php-ext/actions/workflows/release.yml" | ||
| echo " It builds the pre-packaged source tarball and the precompiled binaries," | ||
| echo " checks them, and only then un-drafts the release. Until it succeeds the" |
There was a problem hiding this comment.
"Until it succeeds the release stays a draft" is false.
In release.yml, publish runs gh release edit "$TAG" --draft=false (line 878) and smoke declares needs: [setup, publish] (line 886). The pie install smoke test therefore runs after the release is public, so a red workflow can mean an already-published, immutable release — the opposite of what this tells the releaser.
Worth splitting the two claims: the release stays a draft until publish's last step; the smoke test is a post-publication check whose failure needs a human.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed, and split into the two claims as suggested. The action item now says the
release stays a draft until publish's last step, and separately that the smoke
test runs after that step — so a red workflow can mean an already-published
release, which needs a human rather than a re-run.
This was the most misleading line in the file, because it told the releaser the
opposite of the truth at exactly the moment they would be acting on it.
🤖 Claude, replying on behalf of Greg.
| echo "ERROR: Tag $tag already exists in the extension repository." | ||
| echo "The release workflow only runs on a newly pushed tag, so re-pushing" | ||
| echo "it would build nothing. Check whether the release is already there:" | ||
| echo "https://github.com/maxmind/MaxMind-DB-Reader-php-ext/releases/tag/$tag" |
There was a problem hiding this comment.
This recovery command does not work in the case it is printed for.
The message says the tag already exists, then advises gh workflow run ... -f tag=$tag to rebuild assets. But draft-release refuses outright once the release is no longer a draft (release.yml:213-216):
::error::Release $TAG exists and is already published; refusing to stage assets onto it.
The dispatch only helps while the release is still a draft or absent. Worth saying so, otherwise the operator burns a run to find out.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed — the message now says the dispatch helps while the release is still a
draft or absent, and that it will not help once published, because
draft-release refuses to stage onto a published release.
Worth the extra two lines given the alternative is burning a run to discover
it.
🤖 Claude, replying on behalf of Greg.
| popd >/dev/null | ||
| echo "Extension repository is up to date" | ||
| echo "No commit needed in extension repository (submodule already at $tag)" | ||
| else |
There was a problem hiding this comment.
PR-body / commit-message correction: the already-bumped-submodule case did not exit 0 silently — it exited 1. main pops the directory stack inside this branch and again after fi (3 pushd against 9 popd). Reproduced:
No changes needed in extension repository (already at v9.9.9)
Extension repository is up to date
release.sh: line 95: popd: directory stack empty
exit=1
The net effect (no extension release) and the fix are both right; only the diagnosis is off. Worth correcting so the next reader isn't hunting a silent-exit bug that was never there — "exited 1 on a dir-stack underflow" and "exited 0" have different fixes.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Thank you — I will correct the PR body and the commit message. "Exited 1 on a
dir-stack underflow" and "exited 0 silently" really do have different fixes, and
someone reading the latter would go looking for a missing error check that was
never the problem.
The 3-pushd-against-9-popd count is the detail that makes it verifiable.
🤖 Claude, replying on behalf of Greg.
| # Push changes | ||
| echo "Pushing to origin..." | ||
| git push origin main | ||
| fi |
There was a problem hiding this comment.
Verification suggestion. The tag block is currently testable only by duplicating it — the PR's scratch-repo testing was done on an extracted copy, which validates control flow but not the data flowing through it (the #-stripping and empty-notes issues above are both invisible to that method, and a copy cannot catch divergence from the shipping block).
dev-bin/test-gate-extension.sh on this same branch is the precedent, and its header comment argues the general case — the gate "is only ever observed succeeding". That applies here with almost no modification: this block will be observed succeeding once per release, at the worst possible moment to discover it is wrong.
Two cheap changes would make it durable: extract the block into a function or script taking (remote, tag, notes) so a test can invoke the real code, and add RELEASE_DRY_RUN=1 gating the three mutating operations, which is the single largest reduction in "unexercised until someone cuts a real release".
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Not done, and I think you are right that it is the highest-value thing left in
this file.
Recording the argument so it does not get lost: this block will be observed
succeeding exactly once per release, at the worst possible moment to discover it
is wrong, and the scratch-repo method validates control flow but not the data
flowing through it — which is precisely why the #-stripping and empty-notes
issues were invisible to it.
Some of the pressure is off: the # stripping, the empty notes, the
non-atomic push and the unpushed-commit case are all fixed, so the specific
things that testing would have caught are caught. But the general point holds,
and RELEASE_DRY_RUN=1 around the three mutating operations is the cheap version.
For what it is worth, the extension PR now has the equivalent for its Windows
gate — dev-bin/test-gate-extension.sh, 16 cases — which came directly from your
comment there.
🤖 Claude, replying on behalf of Greg.
| system libmaxminddb as before. GitHub #265. | ||
| * The `conflict` constraint on `ext-maxminddb` in `composer.json` is updated | ||
| when a release is cut again. The substitution that maintains it had | ||
| silently stopped matching, leaving the constraint at `<1.11.1` since |
There was a problem hiding this comment.
The date and the cause are both off (the commit message states this more strongly: "since the constraint gained its upper bound in December 2023").
git log -L 25,25:composer.json shows:
2824e6d(2019-12-18) introduced the upper bound — so it has had one since 2019, not 20231e66f73(2023-12-01) wrote"<1.11.1,>=2.0.0"— comma-separated, which the old(?=,)pattern did matchaf17965(2024-04-08, "Fix conflict directive for ext-maxminddb") changed the separator to||, which is what actually broke it
So the substitution died in April 2024, and four releases shipped a stale floor (1.12.0, 1.12.1, 1.13.0, 1.13.1). This line is defensible as written about the value (it has read <1.11.1 since December 2023), but the commit message's causal claim is not.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed, and thank you for running git log -L rather than taking the commit
message at face value. Confirmed all three commits.
The entry now says the substitution stopped matching in April 2024 when the
separator changed to ||, and that the value has read <1.11.1 through four
releases — which keeps the true part about the value and drops the false causal
claim.
🤖 Claude, replying on behalf of Greg.
| replaces the 1.5.0 import library that PHP publishes for Windows builds. | ||
| The default is unchanged: without the flag, the extension links against a | ||
| system libmaxminddb as before. GitHub #265. | ||
| * The `conflict` constraint on `ext-maxminddb` in `composer.json` is updated |
There was a problem hiding this comment.
Worth noting the user-visible consequence. ext-maxminddb is a Composer platform package, so this constraint resolves against the extension actually loaded in the user's PHP. Reviving the floor means that once 1.14.0 is on Packagist, every user with ext-maxminddb 1.13.1 loaded gets a hard conflict on composer update.
The only thing that clears it is a newer compiled extension, which now arrives from CI asynchronously and reaches PECL only via manual action item 2 — printed after the Composer release is already public. This restores the deliberate 2019 intent (2824e6d), so it isn't wrong, but users have had four releases without it and will experience its return as a regression. A line stating that 1.14.0 requires ext-maxminddb >= 1.14.0 and that C-extension users must upgrade alongside would help.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Added. The entry now states that ext-maxminddb is a Composer platform package,
that this release conflicts with an older compiled extension, and that
composer update will require upgrading the two together.
Your framing decided how to word it: this restores the deliberate 2019 intent, so
it is not wrong, but users have had four releases without it and will experience
its return as a regression. A changelog that only says "the constraint is
maintained again" tells them nothing about what they are about to hit.
🤖 Claude, replying on behalf of Greg.
| * The `conflict` constraint on `ext-maxminddb` in `composer.json` is updated | ||
| when a release is cut again. The substitution that maintains it had | ||
| silently stopped matching, leaving the constraint at `<1.11.1` since | ||
| December 2023. |
There was a problem hiding this comment.
Nit: no GitHub #NNN. reference. The other 1.14.0 entries all carry one (#231, #252, #265) — GitHub #266. would match.
Also, the surrounding entries name their change ("The Windows build configuration now accepts...", "Replaced XtOffsetOf()..."), whereas "is updated when a release is cut again" describes the tooling's future behaviour. The user-visible fact is that the constraint was stale and is now maintained.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Both fixed — GitHub #266. added, and the entry now states the user-visible fact
about the constraint rather than describing the tooling's future behaviour.
🤖 Claude, replying on behalf of Greg.
Under `set -eu -o pipefail` both commands abort the script before the test that follows them runs, so "Please clone manually: ..." and "ERROR: Failed to checkout tag $tag in submodule" could never print. shellcheck flags exactly these two lines and nothing else in the file. The cost was the lost guidance, not the dead branch, so both keep their messages. This branch already removed the same idiom from two other paths; leaving these behind had the file teaching two contradictory lessons about it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The notes parsed out of CHANGELOG.md become four things: this repository's release body, package.xml's <notes>, the extension tag's annotation, and through that the extension release's body. Nothing checked they were non-empty, and nothing downstream would have complained -- `git tag -a -m ""` exits 0 with an empty annotation. So an unusual heading layout that made the sed filter yield nothing would have published all four blank. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`perl -pi` exits 0 whether or not its pattern matched, and exits 0 on a missing file too, printing only to stderr. So each of these seven substitutions could quietly do nothing while the release proceeded, and the `git status --porcelain` test below treats "nothing changed" as normal when for these it is only ever a bug. If every one of them missed, no commit is made and the script goes on to publish a release with no version bumped anywhere. This is not hypothetical. The ext-maxminddb floor in composer.json stopped being updated in April 2024, when af17965 changed the constraint's separator from a comma to " || " and left the anchor matching nothing; four releases shipped it stale. The new anchor is tied to what ends the version rather than to the version's own shape. \d+\.\d+\.\d+ followed by a separator just moves the problem: it does not match "1.14.0-beta1", which the changelog regex explicitly permits, so a prerelease would write a floor the next release then silently failed to update. Consuming up to a space, comma, pipe or quote handles every shape the file has had, a prerelease, and a `composer normalize` that collapses the spaces. The values now reach perl through the environment rather than being interpolated into its source by the shell. $notes is free text, and perl re-reads a double-quoted replacement as code -- "$reader" and "@Args", ordinary words in a PHP project's notes, became variable lookups and vanished. Verified against a copy of the real package.xml. Counted in a separate read-only pass because the substituting run cannot report it: with -i, perl has already renamed the rewritten file into place by the time END could inspect a counter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This script's half of the extension release is a tag push; everything after that is release.yml in the other repository. That workflow is not on the extension repository's default branch yet -- it lives only on maxmind/MaxMind-DB-Reader-php-ext#2 -- and if this merges first, a tag push triggers nothing while the script prints success and exits 0. There is no recovery from that state either. The tag guard further down then refuses every retry because the tag exists, and the `gh workflow run` it suggests cannot help, because workflow_dispatch also resolves the workflow from the default branch. The ordering was documented in the pull request and enforced nowhere. Checked with the other pre-flight checks, before anything is published. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
.ext is only cloned when the directory is absent, so a pre-existing clone made without --recurse-submodules leaves MaxMind-DB-Reader-php an empty directory. `cd` into it succeeds, and git's repository discovery then walks *up*, so `git fetch --tags origin` and `git checkout "$tag"` run against .ext itself and detach its HEAD at its own same-named tag. Both succeed, so `set -e` never fires and the script reports that it updated the submodule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
They were pushed separately, so a failure between them left the commit public and the tag missing -- exactly the half-done state this branch set out to remove. It is not recoverable by re-running either: release.sh dies far earlier at `gh release create`, so the operator never reaches this block again and gets no instructions for it. Pushing main here also settles what the tag names. The cleanliness check above says nothing about ahead-ness, so a local-only commit in a pre-existing .ext clone could be tagged and the tag pushed while the commit itself stayed on no branch GitHub knows about -- the released extension built from an unreachable commit. Sending both together means the tag can only point at something reachable from main, and a main that does not fast-forward now fails the whole push rather than half of it. --cleanup=verbatim on the tag because git's default for -m is --cleanup=strip, which removes every line beginning with '#'. Verified: a "## Breaking changes" heading and a "#123 was fixed" line both vanish, exit 0. The annotation is the extension release's notes, so the two repositories' releases would silently differ. The guard's messages were also wrong in three ways. The workflow does not "only run on a newly pushed tag" -- it also runs on pull requests and workflow_dispatch, which the next line goes on to recommend; what is true is that re-pushing an existing tag raises no event. The suggested dispatch does not work once the release is published, because draft-release refuses to stage onto one. And the paragraph explaining why tagging is necessary was attached to the `git ls-remote` existence check rather than to the tag, so a reader met an argument about workflow triggering followed by an unrelated lookup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The script's last real action was the tag push; after it, success was asserted rather than observed. The code this replaced at least verified its own work -- the tarball had to exist and `gh release create` had to succeed -- and that became a printed promise plus a manual action item. It now waits for a run to appear and says where it is, and if none arrives it says plainly that this repository's release is already public and the situation needs a person. The action item overstated things in the same direction: "until it succeeds the release stays a draft" is false, because the extension workflow's smoke test runs *after* its publish job un-drafts the release. A red workflow can therefore mean an already-published, immutable release, which is the opposite of what the releaser was being told. The two claims are now separate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The entry blamed the stale constraint on something that happened in December 2023. `git log -L 25,25:composer.json` says otherwise: 1e66f73 (2023-12-01) wrote "<1.11.1,>=2.0.0", which the old `(?=,)` anchor matched perfectly well. What broke the substitution was af17965 (2024-04-08, "Fix conflict directive for ext-maxminddb"), which changed the separator to " || ". The value has read <1.11.1 since December 2023; the tooling stopped maintaining it four months later. It also said nothing about what reviving the floor does to users. ext-maxminddb is a Composer platform package, so the constraint resolves against the extension actually loaded: once 1.14.0 is on Packagist, everyone running an older compiled extension gets a hard conflict on `composer update` until they upgrade both together. That restores the deliberate 2019 intent, but users have had four releases without it and will meet its return as a regression, so it is worth saying. Adds the GitHub reference the other 1.14.0 entries carry, and states the change as a fact about the constraint rather than as a promise about the tooling's future behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Requested by Greg Oschwald · Slack thread
STF-1287
For whoever runs a release: today the script clones the extension repo over SSH,
compiles and packages the PIE source tarball on your laptop, and uploads it with
gh release create. After this it bumps the submodule, commits, and pushes anannotated tag — and stops. The extension repo's own workflow builds the source
tarball and the Linux/macOS/Windows binaries, checks each one, uploads them, and
un-drafts the release. Nothing about the main repository's release changes,
including its own
gh release create.The tag is annotated on purpose: the workflow creates the draft release with
--notes-from-tag, so the CHANGELOG-derived notes have to live in the tagannotation.
Merge order: this must not merge before
maxmind/MaxMind-DB-Reader-php-ext#2. Without that workflow in place, the tag
push produces a tag and no release at all.
Two bugs fixed along the way:
branch. A re-run, or a submodule bump someone had already made by hand, took
the other branch and produced no extension release. (It exited 1, not 0, on
a
popdstack underflow -- threepushdagainst ninepopd. The net effectand the fix are unchanged; the diagnosis in an earlier draft of this
description was wrong.) The tag
push now sits outside that branch, since the tag is what starts the release.
Pushing a tag that already exists fails with an explanation and a pointer to
the
workflow_dispatchre-run, rather than a raw git error.perl -pi -e "s/(?<=\"ext-maxminddb\": \"<)\d+.\d+.\d+(?=,)/$version/"nevermatched. The constraint reads
"<1.11.1 || >=2.0.0"— a space follows theversion, not a comma — and
perl -pidoes not fail on a non-match, so theconflict floor has silently sat at 1.11.1. The substitution broke in April
2024, when af17965 changed the separator from a comma to
||; the commaform it replaced was matched correctly, and the bound itself dates from 2019.
Four releases shipped the stale floor. Run against a copy of the real
composer.jsonwithversion=1.14.0, the current pattern leaves the file byte-identical; thefixed pattern yields
"ext-maxminddb": "<1.14.0 || >=2.0.0", with the|| >=2.0.0intact and the JSON still valid. The committed value is leftalone — bumping the floor is a release-time action, and the next release now
performs it.
The extension release path is unexercised until someone cuts a real release.
It cannot be run end to end without pushing a real tag. What was verified:
bash -npasses;shellcheckis now clean on this file, against fivefindings in two classes on
main(SC2181 x4, SC2035 x1) -- an earlier draft ofthis description said "two fewer classes", which overstated it: the first pass
removed one class and three findings, and the two remaining
$?checks havesince been converted too. Note there is still no shell linting in CI, so this
is a local observation nothing preserves; lines 1–93
and 95–137 are byte-identical to
main, so the main-repo path is untouched andonly the one
perlline differs. The new tag block was extracted verbatim fromthe script and run against scratch repositories: a fresh tag is pushed and is
annotated with exactly the release notes, a second run fails with the intended
message, a stale un-pushed local tag is replaced rather than aborting, and — run
side by side against
main— the already-correct-submodule case pushes no tagon
mainand pushes the tag here.Generated by Claude Code