Skip to content

STF-1307: Add bundled libmaxminddb build mode - #265

Open
claude[bot] wants to merge 5 commits into
mainfrom
stf-1287-bundled-libmaxminddb
Open

STF-1307: Add bundled libmaxminddb build mode#265
claude[bot] wants to merge 5 commits into
mainfrom
stf-1287-bundled-libmaxminddb

Conversation

@claude

@claude claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

configuregains--with-maxminddb-bundled, which compiles the new ext/libmaxminddbsubmodule (pinned to 1.13.3) into the extension instead of linking a system library. Bothconfig.m4andconfig.w32implement it. The default build is unchanged on either platform: on Unix it still resolves libmaxminddb through pkg-config, so distro packaging keeps working, and on Windows it still takeslibmaxminddb.libormaxminddb.lib. Verified that the default Unix build's compile and link command lines are identical to main's, and that config.w32`'s non-bundled path is byte-for-byte what it was.

PIE's Unix installer copies only {extension-name}.so, so a prebuilt cannot
depend on a system libmaxminddb. This mirrors what the Python reader does, which
compiles the same two sources on Windows as well and needs no import library
there at all.

A new test-bundled.yml builds the bundled mode across the same PHP version
matrix and runners as test.yml, with no libmaxminddb installed, plus one
build with --enable-maxminddb-debug to show the vendored sources survive
-Werror. It then gates on the properties a prebuilt needs: no libmaxminddb in
ldd, no RUNPATH/RPATH, no undefined MMDB_ symbols, and a glibc floor at
or below GLIBC_2.38 (what the current sources measure, from
__isoc23_strtol). Finally it copies the lone .so to an empty directory and
does a real lookup there.

Windows

Windows links libmaxminddb 1.5.0, published January 2021 and the only version
PHP publishes; that staleness is what forced the two-name CHECK_LIB in #263.
Bundling puts Windows on the same pinned 1.13.3 as every other platform.

config.w32 defines the same macros as config.m4, which matters because
MMDB_UINT128_IS_BYTE_ARRAY decides the layout of the MMDB_entry_data_s union
that maxminddb.c and the vendored sources pass between each other.
HAVE_CONFIG_H is the one deliberate exception: config.m4 defines it only to
stop PHP's Unix CPPFLAGS from sending libmaxminddb after a config.h of its
own, and the Windows build never defines it, so defining it there would instead
make our own maxminddb.c include a config.h that is not present.

config.m4 writes the stub maxminddb_config.h from configure, which never
runs on Windows, so config.w32 writes the same stub itself rather than the file
being committed. Committing it would mean the Unix path overwriting a tracked
file on every build, and two mechanisms for one file. Both now write identical
text, which is why the config.m4 line is touched.

ws2_32.lib is asked for explicitly. php-src already has it in $(LIBS), but
MMDB_open() calls WSAStartup() and the dependency is ours now, which is also
why sapi/cli and sapi/cgi name it themselves.

MSVC cannot be run here, so this was desk-checked instead: every
confutils.js function called was read in php-src and confirmed to have the
signature used; the ADD_SOURCES call follows the idiom php-src's ext/uri and
ext/pcre and PECL's xdebug and php-memcached use for sources in a
subdirectory; ADD_SOURCES's object-path arithmetic was replayed to confirm the
objects land in $(BUILD_DIR)\libmaxminddb\src with no maxminddb.obj
collision, in both phpize and in-tree modes, and that ADD_SOURCES creates that
directory itself (there is no PHP_ADD_BUILD_DIR to forget); and the flag string
was replayed through ADD_FLAG, generate_makefile and MSVC's argv rules to
confirm PACKAGE_VERSION arrives as the string literal "1.13.3" and that the
Makefile line ends in no stray backslash. maxmind/MaxMind-DB-Reader-php-ext#2
switches that repository's Windows lane to the flag so the four Windows legs
exercise it, on the same submodule-tree condition its Linux and macOS lanes
already use.

Two things to know:

  • PACKAGE_VERSION is passed on the command line in bundled mode, and
    MMDB_lib_version() returns it, so it has to be bumped whenever the
    submodule is. CI asserts it matches the submodule's configure.ac, and both
    config.m4 and config.w32 now carry the literal.
  • Nothing in this repository's CI builds config.w32, so the Windows path is
    exercised only by STF-1287: Build and publish PIE precompiled binaries on tag MaxMind-DB-Reader-php-ext#2, and only once that
    repository's submodule points at a commit carrying ext/libmaxminddb.

package.xml is untouched, as PECL keeps using the system library.

.github/dependabot.yml gains a gitsubmodule stanza that ignores
ext/libmaxminddb. Dependabot moves submodules to the tracked branch's tip
when no newer tag exists, which would take the vendored copy off release tags.
#264 adds the same stanza without the ignore, so that tests/data gets
updates; the two stanzas are otherwise identical, so whichever lands second
just needs the ignore list kept.


Generated by Claude Code

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Author

@coderabbitai review


Generated by Claude Code

claude Bot pushed a commit to maxmind/MaxMind-DB-Reader-php-ext that referenced this pull request Aug 3, 2026
Undoes the two TEMPORARY commits, so the gitlink is back at 2194f58
(v1.13.1) and this branch is mergeable again. .gitmodules was never
touched and still tracks main.

Their purpose is served. The bundled lanes had never executed anywhere --
the submodule pointed at a tag with no ext/libmaxminddb, so setup
resolved binaries=false, Linux and macOS skipped, and Windows took the
fetched-library fallback -- which meant the bundled path's first real run
would have been a live release, since dev-bin/release.sh moves this
pointer as part of cutting one. Moving it by hand instead ran that path
on a pull request, and it found a real defect on Windows: ext/maxminddb.c
and libmaxminddb/src/maxminddb.c both resolved to
$(BUILD_DIR)\maxminddb.obj under phpize, so all four legs linked a DLL
with no get_module() and PHP refused to load it. Fixed in
maxmind/MaxMind-DB-Reader-php#265, not here.

With that fix in place every lane was green: Linux x86_64 and arm64 (nts
and zts) at a measured glibc floor of 2.33 against a 2.36 ceiling, macOS
arm64 at minos 11.0, Windows x64 and x86 (nts and ts) building the
vendored sources with no library fetched, source-build building from the
tarball's own libmaxminddb, and verify-assets finding the expected 11
assets. Recorded in the pull request body, since the commit that proved
it is not part of this branch.

Once #265 lands and a tag carries ext/libmaxminddb, this pointer moves to
that tag through release.sh and the same lanes run for real.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Dq18Ws7Bpj9H1eq4sicaz
@oschwald

oschwald commented Aug 3, 2026

Copy link
Copy Markdown
Member

Claude here, on behalf of Greg.

Pushed 03ebcc4, which moves the inline self-containment checks in test-bundled.yml into dev-bin/gate-extension.sh so that this repo and maxmind/MaxMind-DB-Reader-php-ext#2 run one implementation instead of two.

The two copies had already drifted, in the direction of the weaker one. This workflow treated an unmeasurable glibc floor as an error; the extension repo's copy wrapped the whole measuring pipeline in || true and then skipped the assertion when the result came back empty, so a missing objdump would have certified the binary rather than failing it. Verified locally against a real shared object: with objdump off $PATH the old shape exits 0, the shared script exits non-zero.

MAX_GLIBC_VERSION is renamed to MAX_GLIBC and the limit stays at the call site — this job builds on the runner and measures 2.38, the extension repo builds in a pinned bookworm container and holds itself to 2.36. Both are right for how they build, so the script takes the number rather than hard-coding one.

file(1) is now non-fatal. Nothing asserts on its output, and it is a separate package from binutils, so it should not be able to fail the gate in a build image that lacks it.

Follow-up needed in maxmind/MaxMind-DB-Reader-php-ext#2: it should delete its own dev-bin/gate-extension.sh and call MaxMind-DB-Reader-php/dev-bin/gate-extension.sh through the submodule instead. That can only land once the submodule points at a commit carrying this script, so it is gated on this PR merging. Its gate-extension.ps1 has no second caller today and I have left it where it is.

@oschwald

oschwald commented Aug 3, 2026

Copy link
Copy Markdown
Member

Claude here, on behalf of Greg. Pushed three fixups against 03ebcc4 addressing C1, C2, I5 and I6 from review. C1 is a fair hit on code I wrote — worth stating plainly, because the earlier review of the extension repo flagged this exact pattern and I then fixed only one of the four checks while the commit message claimed the gate was sound.

C1 + C2 — 8199bd7. set -e is suspended inside an if condition, so if some-tool "$so" | grep …; then fail; fi could not tell "found nothing" from "not installed". Only check 4 ran its tool in an assignment, which is why only check 4 failed closed. Every tool now runs in its own assignment and the greps read captured output. Reproduced before and after, against a real .so with readelf and nm each removed from $PATH: before 0, after 127.

The NEEDED sed also required GNU's parenthesised (NEEDED); llvm-readelf prints it bare, so the extraction yielded nothing and the check passed on an object that did link libmaxminddb. Parens are now optional and an empty result is fatal. Verified with a shimmed readelf emitting llvm-style output naming libmaxminddb.so.0: exit 1, correct error.

The ldd check is dropped rather than converted — it duplicated the NEEDED check less reliably, and as the review notes it comes from libc-bin, not binutils. The "readelf, nm and objdump are genuinely required" comment was wrong and is corrected. Argument and file-existence checks added.

One thing worth recording: my first attempt at the RUNPATH pattern used \\{0,1\\} in a grep -E, where that is a literal brace rather than an interval — it would never have matched, i.e. another fail-open. Caught by testing the pattern rather than reading it. The sed is BRE and the same spelling is correct there, which is what made it easy to miss.

I5 — 05dd86c. at_most 11 11.0 reported that 11.0 exceeded 11. Harmless for glibc as spelled, but MACOSX_DEPLOYMENT_TARGET=11 against otool's minos 11.0 would have blocked a good release with "Requires macOS 11.0, above the documented maximum 11". Both operands are now padded to equal component counts.

I6 — 44e9fbe. All slices are measured and the assertion runs against the highest. Verified against synthetic fat output: x86_64 at 11.0 plus arm64 at 14.0 measured 11.0 before, 14.0 after. The unreadable-minos message now names LC_VERSION_MIN_MACOSX.

Not addressed, and all outside the gate: C3 (AC_C_BIGENDIAN's third argument mapping unknown to big-endian, and no action-if-universal), I1 (make test exiting 0 with zero tests), I2 (PHPUnit never running against the bundled build — the review is right that this is the best value per line, and it would give C3 real detection), I3/I4 ($srcdir vs $ext_srcdir, the unchecked stub write, the inert .gitignore line), I7 and I8. Those are all in config.m4/config.w32/test-bundled.yml rather than the shared script, so I have left them to you.

@oschwald

oschwald commented Aug 3, 2026

Copy link
Copy Markdown
Member

Claude here, on behalf of Greg. Worked through the rest of the review — 11 more commits, covering C3 and I1–I8 plus the suggestions I judged worth taking.

C3 e54b543AC_C_BIGENDIAN now passes empty if-unknown and if-universal, so MMDB_LITTLE_ENDIAN is left undefined unless the answer is known and maxminddb.h derives it from __BYTE_ORDER__ instead. That derivation is per-architecture correct under a universal build, which no configure-time value can be.

I3 + I4 69b392c — stub moves to $ext_builddir, submodule-presence check added (config.w32 has had one since it was written), write failure is fatal, dead .gitignore line removed. Confirmed the line was inert: git check-ignore refuses outright with "is in submodule".

I7 5de5c78 — flags move to PHP_NEW_EXTENSION's extra-cflags argument. The debug -Werror block moves with them; it predates this branch, but -Werror across a whole php-src tree isn't ours to impose.

I2 2e95b0b — the two PHPUnit steps from test.yml, plus gmp so ReaderTest's uint128 boundary cases run instead of skipping. Agreed this is the best value per line: MMDB_UINT128_IS_BYTE_ARRAY=1 had never been executed by any test in this repo, and the suite is also the only thing in the job that reads a float or double.

I8 2b640fe, I1 3f80c90 — the pkg-config precondition now fails when pkg-config is absent rather than passing on 127; make test asserts its output, since the generated target echoes "Cannot run tests without CLI sapi" and returns 0.

I5/I6 gate coverage 043c2b9 — the debug build now runs the gate, and four negative cases are asserted. This one I'd flag as the most valuable of the batch: nothing anywhere observed the gate failing, and the ERE/BRE mix-up in my own earlier fix is exactly what that blind spot lets through.

Suggestions taken: fdefb95 adds dev-bin/check-libmaxminddb-version.sh and a lint.yml job, closing the gap where config.w32's PACKAGE_VERSION was checked by nothing (verified passing now and failing when set to 1.13.2); 21dbb7b makes check.php assert === 'London' and guards the empty-version sed; f919f80 corrects the "builds on the runner" claim (it's an unpinned container), the shadowing claim, and the config.h one, and names the #if/#ifdef asymmetry; 434bb7c merges the CHANGELOG bullets.

Deliberately not done. Dropping b969d83 and its revert needs a force-push that would discard whatever else is in flight — worth doing in your own final cleanup rather than by me. No Windows CI job: the extension repository already builds Windows from the bundled sources and gates the DLL, so a second one here would duplicate it — the static version lint covers what that job was mainly wanted for. No ASan lane for the bundled config; the review calls it regression coverage rather than a live defect and it's a bigger change. The actions/checkout "we use v1" comment rot is repo-wide and wants its own sweep.

I could not build the extension here, so config.m4 is verified by review and by CI, not locally. The shell and the version lint were both run. If --enable-maxminddb-debug trips on the extra-cflags move, that's the change to look at first.

@oschwald
oschwald force-pushed the stf-1287-bundled-libmaxminddb branch from 434bb7c to 87cfd09 Compare August 3, 2026 20:25
@oschwald
oschwald marked this pull request as ready for review August 3, 2026 21:00
@oschwald oschwald changed the title STF-1287: Add bundled libmaxminddb build mode STF-1307: Add bundled libmaxminddb build mode Aug 3, 2026
@oschwald
oschwald force-pushed the stf-1287-bundled-libmaxminddb branch from 8dc6b64 to 27ba586 Compare August 3, 2026 21:08
oschwald and others added 5 commits August 4, 2026 15:33
Both paths in 003-open-basedir.phpt are POSIX absolute paths and the test has
no --SKIPIF--, so on Windows it fails for reasons unrelated to what it tests.
That is what aborted the Windows run in maxmind/MaxMind-DB-Reader-php-ext#2
before the self-containment gate step was reached, leaving that repository's
gate unexercised on the platform it was added for.

The condition is deliberately the platform alone. 001 and 002 both skip when
the extension is not loaded, which makes this the one test that *fails* rather
than skips when a build produces an extension that cannot load -- the property
that made run-tests.php notice the Windows object-collision bug. An
extension_loaded() guard here would remove it, so the comment says so.

PHP_OS_FAMILY is available from PHP 7.2, this extension's floor.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Refuses to ship an extension object that is not self-contained. Every check
exists because the failure it catches would otherwise reach users as a binary
that loads on the build machine and nowhere else.

On Linux: no libmaxminddb in DT_NEEDED, nothing outside the C runtime and the
loader in DT_NEEDED at all, no RUNPATH or RPATH, no undefined MMDB_ symbols, no
*exported* MMDB_ symbols, get_module still exported, no GLIBC_PRIVATE, and a
measured glibc floor within a documented maximum. On macOS the same shape
against otool and Mach-O, with the deployment target standing in for the glibc
floor and every slice of a universal object measured rather than just the first.

Two rules shape the implementation.

Every tool runs in its own assignment and the greps read captured output.
`set -e` is suspended inside an `if` condition, so `if some-tool "$so" | grep
...; then fail; fi` cannot distinguish "the tool found nothing" from "the tool
is not installed" -- both are a non-zero pipeline and both skip the fail. A
gate whose checks pass when their tools are missing is worse than no gate,
because it reports success.

An unmeasurable result is a failure, not a pass. An empty DT_NEEDED list, an
unreadable glibc floor, an unreadable LC_BUILD_VERSION minos and an empty
operand to at_most are all fatal, because each means the measurement did not
happen rather than that it came back clean.

The limit comes from the caller. MAX_GLIBC and MACOSX_DEPLOYMENT_TARGET are
read from the environment and `set -u` turns a caller that forgets them into a
failure rather than a skip. maxmind/MaxMind-DB-Reader-php-ext runs this same
script over the objects it publishes, holding itself to a lower glibc floor
because it builds in a digest-pinned bookworm container; a single
implementation is the only way that bar cannot drift between the two.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`--with-maxminddb-bundled` vendors libmaxminddb as a submodule at
ext/libmaxminddb and compiles it into the extension, so the resulting object
needs nothing but libc. That is a prerequisite for distributing precompiled
builds: an object linking a system libmaxminddb is not much use as a prebuilt,
because the user still has to install the library. The default is unchanged --
without the flag the extension links a system libmaxminddb exactly as before.

libmaxminddb's maxminddb.h includes "maxminddb_config.h", a header its own
build system generates and which we never run, so both build systems supply a
stub and pass the values it would have defined on the command line. config.m4
writes the stub to the *build* directory: writing it into ext/libmaxminddb/
would put an untracked file inside a submodule, where the superproject's
.gitignore does not apply, leaving every bundled build with a dirty submodule.

Three macro choices are load-bearing and documented where they are made.
HAVE_CONFIG_H=0 works only because libmaxminddb tests its value with `#if`
while our own maxminddb.c tests definedness with `#ifdef`; -UHAVE_CONFIG_H has
to precede it because PHP's CPPFLAGS already define it.
MMDB_UINT128_IS_BYTE_ARRAY=1 is set identically on both platforms because it
decides the layout of the MMDB_entry_data_s union that maxminddb.c and the
vendored sources pass between each other. MMDB_LITTLE_ENDIAN is deliberately
left undefined when AC_C_BIGENDIAN cannot determine the answer or the build is
universal, so maxminddb.h derives it from __BYTE_ORDER__ per architecture; a
wrong value here compiles, links, loads, and returns garbage for every float
and double while strings and integers stay correct.

-fvisibility=hidden keeps the vendored MMDB_* API out of the object's export
table. Vendoring turns those from someone else's exports into ours, and PHP
dlopens extensions with RTLD_GLOBAL on common builds, so a process that also
loads something linked against a system libmaxminddb could bind across the two.
Windows needs no equivalent, MSVC exporting nothing unmarked.

On Windows, ADD_SOURCES needs an explicit object directory: under MODE_PHPIZE
confutils.js derives it from each entry's dirname, and our entries are bare
filenames, so libmaxminddb/src/maxminddb.c and ext/maxminddb.c both resolved to
maxminddb.obj. Whichever compiled last won, and when libmaxminddb's did the DLL
linked without get_module and PHP rejected it as "Invalid library".

test-bundled.yml builds the whole matrix, 9 PHP versions on two architectures,
in containers with no system libmaxminddb and no PKG_CONFIG_PATH. It runs the
phpt suite, the PHPUnit suite against the built object, the shared gate, and a
standalone load of the object with no ini file. The PHPUnit suite matters most:
MMDB_UINT128_IS_BYTE_ARRAY=1 is the path every bundled build takes and no other
job in this repository compiles it, so ReaderTest's assertion of the exact
string returned for 2^120 is the only thing standing between a swapped
high/low word and a plausible-looking wrong answer. Its float and double cases
are likewise the only exercise MMDB_LITTLE_ENDIAN gets. Every check asserts its
own preconditions: a missing pkg-config, a `make test` that ran no tests, and an
unreadable version are all failures rather than quiet passes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The schedule and cooldown match the existing entries.

ext/libmaxminddb is excluded. It is pinned to a release tag and compiled into
the extension, and the gitsubmodule ecosystem advances a submodule to the tip
of its tracked branch, which would take us off release tags. Bumping it is a
deliberate action: the version has to be updated in ext/config.m4 and
ext/config.w32 at the same time, which dev-bin/check-libmaxminddb-version.sh
asserts.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MMDB_lib_version() returns PACKAGE_VERSION, which both build systems pass on
the command line because libmaxminddb's own configure never runs here. Three
places have to agree: the submodule's configure.ac, ext/config.m4 and
ext/config.w32.

test-bundled.yml already compares MMDB_LIB_VERSION against configure.ac at
runtime, which covers the first two. config.w32 is covered by nothing: there is
no Windows workflow in this repository, and Dependabot deliberately ignores the
submodule, so a hand bump that updates config.m4 and forgets config.w32 leaves
Windows users a version string that lies, with nothing to notice until someone
reads phpinfo().

A static comparison closes that in lint.yml without needing a Windows runner.
An unmatched pattern is fatal rather than read as agreement, which is the
failure mode a check like this would otherwise have.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@oschwald
oschwald force-pushed the stf-1287-bundled-libmaxminddb branch from c4e6f19 to fe126e5 Compare August 4, 2026 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant