Fix GH-20557: Build make -C ext/* install-modules w/o modules/*

[hakre]

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).

[devnexen]

Note that in fact it might need to target PHP-8.5 branch though.

[TimWolla]

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.

[petk]

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.

[hakre]

Thanks @petk, I couldn't have put it better myself.

[hakre]

Thank you @TimWolla for your candid feedback.

[TimWolla]

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?

[TimWolla]

If anything, it makes sense here to check whether the directory exists and contains files for installation.

@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.

[petk]

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 test -d modules few lines above in that target already, which tests if there is a modules directory present.

[hakre]

Edit: Ah, I see. And there is even test -d modules few lines above in that target already, which tests if there is a modules directory present.

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..

[petk]

Edit: Ah, I see. And there is even test -d modules few lines above in that target already, which tests if there is a modules directory present.

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

[hakre]

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.

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:

1. the change is for ext/* as in the subject.
2. install is a standard target
3. cause of effect != place of change

(sorry in a hurry, but will be back. your much more concrete feedback actually helps me to point out things more precisely, HTH @TimWolla)