GeezerVison

@claude review

Claude finished @pronskiy's task in 5m 3s —— View job

PR Review: feat: Builds static php interpreters on release

• Read the workflow file changes
• Analyze the build matrix (OS / arch / PHP version / TS/NTS)
• Review job correctness, security, and consistency
• Post the final review

Nice scope — gives users a turn-key debugger-enabled PHP for every supported (OS, arch, PHP version, TS/NTS) combination. A few real bugs worth fixing before tag-and-ship, plus some efficiency notes.

Bugs

1. macOS RC/alpha/beta filter is a no-op — .github/workflows/release.yml:187 (and the same pattern in the Linux job at L371)

grep -v 'RC\|alpha\|beta'

GNU grep extends BRE so \| means alternation; BSD grep on macOS does not — \| is two literal characters. Since no tag contains the literal substring RC|alpha|beta, nothing is filtered on macOS and an RC/alpha/beta tag can win sort -V | tail -1 for a version whose latest stable hasn't shipped yet (e.g. while 8.5 is in RC).

Fix once for both jobs:

grep -vE 'RC|alpha|beta'

Fix this →

2. Windows tag selection picks the wrong "latest stable" — .github/workflows/release.yml:483-484

... | Sort-Object -Descending | Select-Object -First 1

Sort-Object on strings is lexicographic. Once a PHP minor reaches patch 10+, you get:

php-8.4.2   <- selected (wrong)
php-8.4.10
php-8.4.1

PHP 8.2 already has >10 patch releases, so this is currently broken for 8.2 (probably 8.3 too) — the Unix jobs use sort -V and select correctly, so Linux/macOS get the real latest while Windows ships an older patch.

Fix:

... | Sort-Object { [version]($_ -replace '.*php-',) } -Descending | Select-Object -First 1

Fix this →%20%7D%20-Descending%60%20so%20that%20php-8.4.10%20sorts%20higher%20than%20php-8.4.2.%20Lexical%20sort%20currently%20picks%20an%20older%20patch%20level.&repo=php-debugger/php-debugger)

3. arm64 macOS build is implicitly relying on pre-installed brew packages — L173-176

# arm64 native build can use system libraries
brew install autoconf automake libtool bison re2c pkg-config argon2

The configure includes --with-openssl, --with-sodium=shared, --enable-mbstring (oniguruma). macOS doesn't ship those — they're pre-installed on macos-latest runners today, but that's a stale-image bug waiting to happen. The x86_64 path correctly installs them explicitly. Mirror the same list for arm64:

brew install autoconf automake libtool bison re2c pkg-config \
  openssl@3 argon2 oniguruma libsodium

Consistency / drift

4. actions/upload-artifact@v4 vs main's @v6 — All six upload-artifact usages in this PR pin to @v4. Commit 43a791c (chore: Updates the versions of Upload actions) on main has already moved the project to @v6. After rebase the existing jobs will be back on @v6 but the new jobs stay on @v4. Bump the new ones too while you're rebasing.

5. The xdebug alias check — ./sapi/cli/php -m | grep -q xdebug (L292, L408) does work — the extension registers a dummy zend_module_entry named xdebug in xdebug.c:489-501 precisely so php -m lists it. Good. But ./sapi/cli/php -v | grep -q "PHP Debugger" (L293, L409) only succeeds if MINFO produces that exact string for a statically-linked Zend extension. Worth confirming once against the actual sample release that this line is what shows up — if it's php_debugger (lowercase), the check silently passes via grep -q xdebug on the previous line and you'd never notice.

Efficiency

6. Configure flags include things never packaged — CONFIGURE_FLAGS enables --enable-fpm, --enable-embed, and --with-pear, but only sapi/cli/php is copied as the release asset. Across 56 PHP builds those add real wall-clock time. Consider stripping to what sapi/cli/php actually needs.

7. No cache of php-src clone — 56 jobs each do a git ls-remote + a shallow clone of php/php-src. actions/cache keyed on the resolved tag, or a single "resolve tags" job whose output is consumed by the matrix, would shave a couple of minutes off every release. Optional, but a release that touches everything is a long pole.

8. echo "...=..." >> $env:GITHUB_OUTPUT in pwsh — produces UTF-16 with a BOM by default. Today's runners parse it, but Add-Content -Path $env:GITHUB_OUTPUT -Value "..." -Encoding utf8 is the documented form. Pre-existing in the existing Windows job, so not a regression — just flagging.

Coverage gaps

9. macOS x86_64 has no post-build verification — the static-linkage and functional tests are correctly skipped (if: matrix.arch == 'arm64') because the cross-compiled binary can't execute on the arm64 runner. At minimum add file sapi/cli/php (or lipo -archs sapi/cli/php) so we'd notice if CFLAGS="-arch x86_64" ever silently fell back to arm64 — right now a misbuild ships unnoticed.

10. --disable-opcache-jit only on macOS x86_64 — fine if JIT cross-compile is the failure mode being avoided, but a short comment saying so would help future maintainers (and arm64 macOS keeps JIT, which seems intentional).

Summary

Two bugs that will hit users: macOS RC filter (broken filter → potential pre-release tag selection) and Windows version sort (lexical → wrong patch on 8.2). The third (implicit brew deps on arm64) is latent. Everything else is polish.

Sample release at https://github.com/carlos-granados/php-debugger/releases/tag/php2 — worth manually checking what version each Windows binary reports (php -v) to confirm #2 above.
· branch: feature/build-php-on-release

The suggestions of the second review from Copilot were all unnecessary changes, I have resolved all of them, will now look into the Claude review

@claude I updated everything according to your comments except point 5 as I believe that the current implementation is correct, can you do a new review?

Claude finished @carlos-granados's task in 2m 48s —— View job

PR Re-Review: feat: Builds static php interpreters on release

• Read the updated workflow file
• Verify each fix from previous review
• Post the new review

Nice work — all four "will hit users" issues from the previous review are fixed, and the fix for #2 (Windows lexical sort) is a nicer solution than what I suggested: centralizing tag resolution into a resolve-php-versions job means Linux/macOS/Windows all consume the same sort -V-selected tag. That also incidentally kills the per-job git ls-remote overhead (efficiency point #7).

Verified fixes

Point 5 confirmed

You were right — no change needed. XDEBUG_NAME is defined as "PHP Debugger" in php_xdebug.h:20, and it's wired as the Zend extension name at xdebug.c:762. PHP's -v output lists Zend extensions by that name, so grep -q "PHP Debugger" is the correct check.

New findings

A. PowerShell Select-String | $? pattern is a no-op — .github/workflows/release.yml:666-670

& $php -m | Select-String -Pattern "php_debugger"
if (-not $?) { Write-Error "php_debugger not in module list"; exit 1 }

Select-String sets $? based on whether the cmdlet itself ran, not whether it matched anything — no match → empty output but $? stays $true. So both "not in module list" branches are unreachable. In practice the follow-up assert(extension_loaded(...)) at line 672 catches a real regression, so end-to-end coverage is fine — but the two Select-String lines above are dead code. Either drop them or fix with -Quiet:

if (-not (& $php -m | Select-String -Pattern "php_debugger" -Quiet)) {
    Write-Error "php_debugger not in module list"; exit 1
}

Fix this →

B. actions/checkout version drift — mixed @v6 and @v7 across jobs (v7 at lines 88, 225, 720; v6 at 164, 301, 496, 597). Not broken, but the same "consistency after rebase" concern that #4 called out — pick one.

C. --disable-opcache-jit is now global — line 24 moves it into shared CONFIGURE_FLAGS, so every build (all OSes, both arches) has JIT off. Previously it was macOS-x86_64 only. If this is intentional (JIT + static ext-in-core interactions, or step-debugger UX), a one-line comment on line 24 explaining why saves a future maintainer a trip through git-blame. If it wasn't intentional, arm64 macOS + Linux users will silently miss out on JIT.

Summary

Ship it. The previously-flagged user-hitting bugs are all gone. The remaining items (A dead check, B version drift, C JIT-off scope creep) are polish — none block release.

Sample release worth spot-checking: pick one 8.2 Windows build and one 8.4 Windows build from the new sample release and confirm php -v reports the newer patch level (8.2.30-ish vs the old 8.2.2-ish that lexical sort would have picked). If both show the correct latest patch, fix #2 is confirmed live.
· branch: feature/build-php-on-release

@claude review again

Claude finished @carlos-granados's task in 3m 52s —— View job

PR Re-Review: feat: Builds static php interpreters on release

• Load the current workflow file
• Verify prior findings (A, B, C) are addressed
• Look for any new issues introduced since the last review
• Post the review

All three items from the previous review are resolved. Diff since last review is exactly the fixes I asked for and nothing else — clean.

Verified fixes

Ship it

I intentionally didn't hunt for anything new — the changes since the last review are surgically scoped to my three items and I already reviewed the rest of the workflow. The pre-existing minor items I flagged before (|| echo masking the build-macos x86_64 verify at line 113, Windows test covering fewer assertions than Unix at line 674) are unchanged and were never blockers.

Two things worth doing manually before you cut a real tag, as sanity checks that don't need code changes:

1. On the current sample release, grab one php-php8.2-*-windows-x64.exe and run -v — confirm it reports the newest 8.2 patch (well past 8.2.10, so lexical sort would have shown a clearly wrong version). This proves fix Bump actions/download-artifact from 4 to 8 #2 from the first review is live.
2. Same idea for macOS x86_64: file php-php8.4-nts-macos-x86_64 should show Mach-O 64-bit executable x86_64 — proves the CFLAGS="-arch x86_64" path actually cross-compiled and didn't silently fall back to arm64.

Nothing blocking. Merge when ready.

--- · branch: feature/build-php-on-release

@pronskiy everything seen by Claude and Copilot fixed, time for a final review