Follow ups to bundled/system libmaxminddb reporting - #291
Conversation
php_info_print_table_row() emits one cell per argument and no colspan, so the three-argument row added in #289 was wrong output rather than an unusual layout. In text mode it read libmaxminddb library version => 1.14.0 => (bundled) which has the Directive => Local Value => Master Value shape of an ini table, so the version row stopped being key => value. In HTML it was a three-cell <tr> in a table whose other rows have two, leaving a ragged empty column. The row keeps the shape #289 intended, with the source as a suffix in the same cell: libmaxminddb library version => 1.14.0 (bundled) Selecting the string ahead of the call also removes the `);` that was duplicated across both #ifdef arms, and the hunk now formats cleanly under .clang-format, whose BinPackArguments: false would otherwise have rewritten it. lint.yml does not run clang-format, so CI had not noticed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The AC_DEFINE description read "Use bundled or system libmaxminddb". That
describes --with-maxminddb-bundled, not the macro, and autoheader copies it
verbatim into the comment above the #define, so the generated header read
/* Use bundled or system libmaxminddb */
#define HAVE_LIBMAXMINDDB_BUNDLED 1
which resolves nothing for whoever reads it. Say what being defined means
instead, following the convention libmaxminddb's own configure.ac uses for
MMDB_LITTLE_ENDIAN ("System is big-endian" / "System is little-endian").
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A macro that is defined on one path and absent on the other cannot tell "system build" apart from "the define never arrived", so the phpinfo() row would report "system" for a bundled object and nobody would know. On Windows that is not hypothetical. Under phpize, configure.js writes AC_DEFINE results to <devpack>/include/main/config.pickle.h, which is shared state in the PHP install rather than the build tree, and generate_config_pickle_h() removes only the lines whose keys the current run defines. Configure with --with-maxminddb-bundled, reconfigure without it, and the stale `#define HAVE_LIBMAXMINDDB_BUNDLED 1` survives, so a system build reports "bundled". Fresh CI containers never see this; a developer's machine does. Both build systems now define the macro as 1 or 0 on every path that builds the extension, so a system configure run overwrites whatever a bundled one left behind, and maxminddb.c tests the value with #if. An #ifndef guard turns a definition that never arrives into a compile error instead of a wrong row. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The two build files are the most heavily commented in the repository and the AC_DEFINE added in #289 was the one line in them with no rationale. Worse, config.w32 says a few lines above it that "the Windows build never defines HAVE_CONFIG_H" and that defining it "would make our own maxminddb.c include a config.h that does not exist". A maintainer trusting that prose could reasonably conclude the AC_DEFINE is dead and delete it. It is not: under phpize the value is written to <devpack>\include\main\config.pickle.h and reaches maxminddb.c through php.h, zend_portability.h, zend_config.w32.h and main\config.w32.h, a route that has nothing to do with HAVE_CONFIG_H. Both files now say what the macro is for, why it is defined as 0 rather than left absent on the system path, and, in config.w32, the include chain it travels and the stale-pickle failure that the 0 exists to overwrite. The HAVE_CONFIG_H paragraph points at that comment so the two are not read as contradicting each other. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The new row is user-visible output and #289 shipped without noting it. 1.14.0 is already released at the top of the file, so this opens the next section, undated until release the way 1.14.0 was. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The comment above the version scrape said "there is no Windows workflow in this repository" and that nothing would notice a stale PACKAGE_VERSION. 5e4a6db added the windows-bundled job after 22297cc's comment-correction pass, so the claim was missed; that job compares MMDB_LIB_VERSION against the submodule's configure.ac, which is exactly the check the comment said was absent. The same paragraph said config.m4 keeps a literal version. It has not since the literal moved to ext/bundled-include/maxminddb_config.h, and config.m4's own comment says so. The paragraph now names where the Unix literal lives and which jobs verify both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe build scripts now define whether libmaxminddb is bundled or system-linked. The extension requires this definition and displays the library version with the corresponding source label in PHP information output. ChangesLibrary source reporting
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The bundled/system source label and library-version reporting change has no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. A rabbit saw the library light, Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The Windows guard can prevent builds, and the changelog format can break the release script.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds bundled/system provenance to the libmaxminddb version shown by phpinfo() and php --ri.
Changes:
- Reports the library version with
(bundled)or(system). - Defines provenance markers for Unix and Windows builds.
- Documents the behavior in the changelog.
File summaries
| File | Summary | Review notes |
|---|---|---|
ext/maxminddb.c |
Reports library provenance. | Windows macro guard may prevent compilation; add source-suffix coverage. |
ext/config.w32 |
Defines the Windows provenance marker. | Related to the compilation guard issue. |
ext/config.m4 |
Defines the Unix provenance marker. | No blocking issue noted. |
CHANGELOG.md |
Documents the new output. | Undated heading is incompatible with the release script. |
Review details
Suppressed comments (1)
CHANGELOG.md:5
dev-bin/release.shonly matches a version heading that includes a date (lines 78-93) and then requires that date to be today (104-107). Since this new1.15.0heading is undated, the script skips it, selects the older1.14.0entry, and aborts on the stale date instead of releasing these notes; please align this changelog format with the release script or update the script to handle an unreleased heading.
1.15.0
-------------------
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #ifndef HAVE_LIBMAXMINDDB_BUNDLED | ||
| #error "HAVE_LIBMAXMINDDB_BUNDLED must be defined by the build files" |
There was a problem hiding this comment.
The guard sits after #include <php.h> (line 20), not before it. The config.h that config.w32 leaves out is a different header. On Windows the macro reaches this file through php.h → zend_portability.h → zend_config.w32.h → main/config.w32.h → config.pickle.h, which the comment above the AC_DEFINE in config.w32 spells out.
CI confirms it: the Windows bundled job in test-bundled.yml compiled and loaded the DLL on this PR, which it could not have done if this #error fired.
🤖 Comment by Claude (Claude Code) on behalf of Will.
| spprintf(&lib_version, 0, "%s (%s)", MMDB_lib_version(), lib_source); | ||
| CHECK_ALLOCATED(lib_version); | ||
| php_info_print_table_row(2, "libmaxminddb library version", lib_version); |
There was a problem hiding this comment.
Agreed that nothing in CI calls phpinfo(), so a wrong label would go unnoticed.
If we add a check, the cheapest place is dev-bin/verify-extension.php. It already receives the expected libmaxminddb version, and every job that runs it is a bundled build, so it could capture phpinfo(INFO_MODULES) with output buffering and assert the row reads libmaxminddb library version => <version> (bundled). That covers the Linux matrix and the Windows job with no package.xml change. The system side would be a one-line grep for (system) in test.yml after the build step. A phpt cannot know which mode it was built in, so it could only check the row's shape, and it would also need a package.xml entry.
Will doesn't want to bother with this right now, though.
🤖 Comment by Claude (Claude Code) on behalf of Will.
Summary by CodeRabbit
New Features
Documentation
phpinfo()andphp --ri maxminddb.