-
Notifications
You must be signed in to change notification settings - Fork 34
Add bundled php exts support #275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the remarks, I'm more generally concerned about this. As you can see for the extensions where you commented the build failures, the bundled extensions are not tested outside of PHP’s build system and it's probably easy to accidentally choose some different configure
options that result in subtly broken extensions, especially when considering that Linux distributions often ship with patched PHP versions, where PIE is going to miss out on those patches. The bundled extensions really should just be installed by the respective package manager.
[ | ||
'name' => 'opcache', | ||
'type' => ExtensionType::ZendExtension, | ||
'version' => '>= 5.5.0', | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extension is always loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opcache (at least for now) can be disabled when compiling with --disable-opcache
, so theoretically could be built as a shared extension, as I understand it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this meant to read "is always loaded with PHP 8.5". This is not yet the case, but will likely be with 8.5.0 Alpha 3 as part of https://wiki.php.net/rfc/make_opcache_required.
Yes, my plan here is to add an integration test that does a test
This was my expectation too, but we have had several requests, I think the most common would be PDO*, bcmath, mbstring. |
Just building extensions is not a sufficient guarantee that they'll work correctly. As an example MySQLnd relies on OpenSSL for the TLS support and trying to build MySQLnd in a standalone fashion is not unlikely to result in a version without TLS support, even when OpenSSL is loaded in PHP as a shared module. |
ecfe93a
to
255855a
Compare
To expand on this a little more: It would also make it easy to accidentally run subtly different patch versions of the bundled libraries, leading to confusion when reporting issues. Consider the following:
Now a user finds a bug in the MySQL integration and reports it as “I'm using PHP 8.2.28” and folks are left confused, because the bug had already been fixed in MySQLnd with PHP 8.2.27, but the user didn't install the updated mysqlnd with PIE. Over time the versions will diverge further when half of the stuff was installed with the package manager and the other half with PIE. Or possibly some extension is installed with PIE and then later the corresponding package is installed with the package manager, leading to both the package manager and the PIE version being installed and fighting with each other. Instead it would probably make sense to report a helpful message when running
or
or similar. |
This constant can be handy for tools like PIE to determine the origin of a PHP binary to provide better output / diagnostics. see php/pie#275 see php#18168
'name' => 'ffi', | ||
'require' => ['php' => '>= 7.4.0'], | ||
], | ||
// ['name' => 'gd'], // build failure - ext/gd/gd.c:79:11: fatal error: ft2build.h: No such file or directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't work this one out. The required system dependencies seem to be installed, but this header still seems to be missing in CI.
@@ -105,6 +106,12 @@ public function extensionPath(): string | |||
return $extensionPath; | |||
} | |||
|
|||
// if the path is absolute, try to create it | |||
// @todo error checking/squelching | |||
if (mkdir($extensionPath, 0777, true) && file_exists($extensionPath) && is_dir($extensionPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #263
- '8.2' | ||
- '8.3' | ||
- '8.4' | ||
#- '8.5' # @todo enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking this todo
@@ -105,6 +106,12 @@ public function extensionPath(): string | |||
return $extensionPath; | |||
} | |||
|
|||
// if the path is absolute, try to create it | |||
// @todo error checking/squelching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking this todo
Fixes #133