STF-1307: Add bundled libmaxminddb build mode - #265
Conversation
|
@coderabbitai review Generated by Claude Code |
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
|
Claude here, on behalf of Greg. Pushed 03ebcc4, which moves the inline self-containment checks in 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
Follow-up needed in maxmind/MaxMind-DB-Reader-php-ext#2: it should delete its own |
|
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 — The NEEDED The One thing worth recording: my first attempt at the RUNPATH pattern used I5 — I6 — Not addressed, and all outside the gate: C3 ( |
|
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 I3 + I4 I7 I2 I8 I5/I6 gate coverage Suggestions taken: Deliberately not done. Dropping 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 |
434bb7c to
87cfd09
Compare
8dc6b64 to
27ba586
Compare
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>
c4e6f19 to
fe126e5
Compare
configure
gains--with-maxminddb-bundled, which compiles the newext/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 tomain's, and thatconfig.w32`'s non-bundled path is byte-for-byte what it was.PIE's Unix installer copies only
{extension-name}.so, so a prebuilt cannotdepend 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.ymlbuilds the bundled mode across the same PHP versionmatrix and runners as
test.yml, with no libmaxminddb installed, plus onebuild with
--enable-maxminddb-debugto show the vendored sources survive-Werror. It then gates on the properties a prebuilt needs: no libmaxminddb inldd, noRUNPATH/RPATH, no undefinedMMDB_symbols, and a glibc floor ator below
GLIBC_2.38(what the current sources measure, from__isoc23_strtol). Finally it copies the lone.soto an empty directory anddoes 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_LIBin #263.Bundling puts Windows on the same pinned 1.13.3 as every other platform.
config.w32defines the same macros asconfig.m4, which matters becauseMMDB_UINT128_IS_BYTE_ARRAYdecides the layout of theMMDB_entry_data_sunionthat
maxminddb.cand the vendored sources pass between each other.HAVE_CONFIG_His the one deliberate exception:config.m4defines it only tostop PHP's Unix
CPPFLAGSfrom sending libmaxminddb after aconfig.hof itsown, and the Windows build never defines it, so defining it there would instead
make our own
maxminddb.cinclude aconfig.hthat is not present.config.m4writes the stubmaxminddb_config.hfromconfigure, which neverruns on Windows, so
config.w32writes the same stub itself rather than the filebeing 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.m4line is touched.ws2_32.libis asked for explicitly. php-src already has it in$(LIBS), butMMDB_open()callsWSAStartup()and the dependency is ours now, which is alsowhy
sapi/cliandsapi/cginame it themselves.MSVC cannot be run here, so this was desk-checked instead: every
confutils.jsfunction called was read in php-src and confirmed to have thesignature used; the
ADD_SOURCEScall follows the idiom php-src'sext/uriandext/pcreand PECL's xdebug and php-memcached use for sources in asubdirectory;
ADD_SOURCES's object-path arithmetic was replayed to confirm theobjects land in
$(BUILD_DIR)\libmaxminddb\srcwith nomaxminddb.objcollision, in both phpize and in-tree modes, and that
ADD_SOURCEScreates thatdirectory itself (there is no
PHP_ADD_BUILD_DIRto forget); and the flag stringwas replayed through
ADD_FLAG,generate_makefileand MSVC'sargvrules toconfirm
PACKAGE_VERSIONarrives as the string literal"1.13.3"and that theMakefile 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_VERSIONis passed on the command line in bundled mode, andMMDB_lib_version()returns it, so it has to be bumped whenever thesubmodule is. CI asserts it matches the submodule's
configure.ac, and bothconfig.m4andconfig.w32now carry the literal.config.w32, so the Windows path isexercised 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.xmlis untouched, as PECL keeps using the system library..github/dependabot.ymlgains agitsubmodulestanza that ignoresext/libmaxminddb. Dependabot moves submodules to the tracked branch's tipwhen no newer tag exists, which would take the vendored copy off release tags.
#264 adds the same stanza without the ignore, so that
tests/datagetsupdates; the two stanzas are otherwise identical, so whichever lands second
just needs the ignore list kept.
Generated by Claude Code