-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix GH-20557: Build make -C ext/* install-modules w/o modules/* #20558
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: master
Are you sure you want to change the base?
Conversation
update the build recipe template as in PHP-8.5 ext/opcache errors on
`make -C ext/opcache install` as modules/* does not resolve to actual
filenames any longer,
but to `modules/*` verbatim which fails the `install` command yielding
the following diagnostics:
cp: cannot stat 'modules/*': No such file or directory
and exit status 1 (resolved by make as build error 2).
|
Note that in fact it might need to target PHP-8.5 branch though. |
TimWolla
left a comment
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.
After both reading the issue and the description of this PR, it is still not clear to me what this is supposed to fix. Installing OPcache as a separate module is no longer supported and this is documented as part of the migration guide.
If anything, it makes sense here to check whether the directory exists and contains files for installation. Otherwise, yes. That type of installation with phpize won't work in any case anymore for opcache extension since PHP-8.5. |
|
Thanks @petk, I couldn't have put it better myself. |
|
Thank you @TimWolla for your candid feedback. |
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.
The PR description is unchanged from my previous review and does not provide an argument or example in favor of this change that doesn't involve trying to install OPcache as a shared module. This makes it impossible to evaluate the correctness of this change.
Should this check $PHP_MODULES instead?
@petk As far as I'm concerned, “no module is available” is an error situation and emitting an error message is the correct consequence of that. |
|
Actually, yes. I'm exactly at this point now also in my CMake-based build system. :D Yes, building extensions, such as opcache, json, hash, random, date, etc. in "phpize" style should emit some error, yes. Otherwise, also this bug in Docker image wouldn't be noted. It's only that install modules target in Makefile is probably a bit inconvenient place for detecting and emitting such error, yes. Edit: Ah, I see. And there is even |
Yes, this is parenting in the fix. Btw. have you figured out in CMake to define all install files for the shared objects? Great project btw.. |
Yes. It felt almost like a rocket science. Very complex but quite neat overall. It's resolved over CMake configuration files in one of my local branches and even with upcoming CPS experimentation. :D |
The build error prevents you to do that. This is why we fixed it. It stands in the way to evaluate the correctness of the solution, because you can evaluate correctness only with the solution, but never with a build error. A precondition of that is, that the build recipes are free of error. Furthermore, three things:
(sorry in a hurry, but will be back. your much more concrete feedback actually helps me to point out things more precisely, HTH @TimWolla) |
update the build recipe template as in PHP-8.5 ext/opcache errors on
make -C ext/opcache installas modules/* does not resolve to actual filenames any longer,but to
modules/*verbatim which fails theinstallcommand yielding the following diagnostics:and exit status 1 (resolved by make as build error 2).