-
Notifications
You must be signed in to change notification settings - Fork 3.8k
php8: fix and enhance configuration and dependencies #28234
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?
php8: fix and enhance configuration and dependencies #28234
Conversation
Switch to a single CONFIG_ per line, and alphabetize. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
* Add a needed BUILD_DEPENDENCY on icu package, when PHP8_GETTEXT is defined. * Make PHP8_INTL select/deselect properly depending on whether PHP8_GETTEXT is defined. Previously, deselecting PHP8_GETTEXT would select PHP8_INTL and make it impossible to deselect. * Only show option for choosing PHP8_FULLUCIDATA when all of PHP8_GETTEXT, PHP8_INTL, and the php8-mod-intl package are selected. * Make php8-mod-intl depend on php8-mod-gettext * For php8-mod-intl, depend on icu-full-data when PHP8_FULLUCIDATA is selected. * For php8-cgi, php-cli, etc, a libstdcpp dependency is only gained when PHP8_INTL is selected, therefore update those conditional depends. As some combinations of these changes can change the binaries output, PKG_RELEASE has been bumped. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
|
Nice! At first glance, it looks good to me. I would like to have a deeper look tomorrow evening. |
xmlreader was selecting package php8-mod-dom as well as depending on PHP8_DOM, while php8-mod-dom also depended on PHP8_DOM (and therefore selected PHP8_DOM when php8-mod-dom was selected). This is a Kconfig recursive dependency, so break the recursion by noting that because php8-mod-xmlreader selects php8-mod-dom, PHP8_DOM is a transitive depends, so php8-mod-xmlreader should not depend on PHP8_DOM itself. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
72ef85e to
be62019
Compare
be62019 to
faae8bd
Compare
|
Noticed an issue with configuring with openssl when php8-mod-openssl is not selected, so fixed that. |
Add more menuconfig help text descriptions, and convert some mixed tabs and spaces to spaces. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
bf8f4bc to
23ae03c
Compare
|
I've dropped the openssl changes - this looks like another can of worms. |
| CONFIG_PHP8_FULLICUDATA \ | ||
| CONFIG_PHP8_GETTEXT \ | ||
| CONFIG_PHP8_INTL \ | ||
| CONFIG_PHP8_SYSTEMTZDATA |
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.
nitpick: While at, I'd recommend to just order this alphabetically.
| depends on PHP8_LIBXML | ||
| default y | ||
| help | ||
| Without the mod-dom, this option does not provide a PHP8 |
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.
Here, I'd use "Without the dom module,..." or we could use the complete package names, e.g. "Without php8-mod-dom, this..."
| CONFIG_PHP8_INTL \ | ||
| CONFIG_PHP8_SYSTEMTZDATA | ||
|
|
||
| PKG_BUILD_DEPENDS:= PHP8_GETTEXT:icu |
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.
Do you mean PHP8_INTL here?
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.
No, due to this configure line, which needs icu to compile, but icu does not become a php8 dependency for php8-mod-gettext because of it, nor is php8-mod-gettext required to be built (in the case php8-mod-gettext is not built, it affects php8-cli, php8-*cgi, and apache-mod-php8, so is still relevant).
ifeq ($(CONFIG_PHP8_GETTEXT),y)
CONFIGURE_ARGS+= --with-gettext=shared,"$(STAGING_DIR)/usr/lib/libintl-full"
else
CONFIGURE_ARGS+= --without-gettext
endifprior to 6d6233b
this was
ifneq ($(SDK)$(CONFIG_PACKAGE_php8-mod-gettext),)
CONFIGURE_ARGS+= --with-gettext=shared,"$(STAGING_DIR)/usr/lib/libintl-full"
else
CONFIGURE_ARGS+= --without-gettext
endifThere 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.
Here I'm still confused because:
-
before we had:
PACKAGE_php8-mod-intl→--enable-intl=sharedand+PACKAGE_php8-mod-intl:icuwhich added the runtime dep and thus kind of automatically added a build-dep only when the module was selected, parallelCONFIG_PACKAGE_php8-mod-gettext→--with-gettext=shared,"$(STAGING_DIR)/usr/lib/libintl-full"and the corresponding runtime dep to libintl-full, but without any reference to icu -
now we have
PHP8_INTLdepends onPHP8_GETTEXTwith:PHP8_INTL→--enable-intl=sharedandPHP8_GETTEXT→--with-gettext=shared,"$(STAGING_DIR)/usr/lib/libintl-full"; independent of whether the modules are selected or not
In my tests, even when I don't declare the build dependency, then icu is built (I don't know why yet). But gettext seems to not have a runtime dep on icu (at least readelf does not show it), only intl does. So I assumed that it would make more sense to use PKG_BUILD_DEPENDS:= PHP8_INTL:icu here.
This is why I'm still confused - do you have other results/findings?
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.
- For me the GETTEXT
"$(STAGING_DIR)/usr/lib/libintl-fullfails with complaint about missingicuunless I usePKG_BUILD_DEPENDS:=PHP8_GETTEXT:icu. - gettext does not acquire a runtime dependency on
icudespite that.
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.
I have not tried to build mod-gettext without that config - perhaps it could be moved to PHP8_INTL? It was there from the original version, and I didn't try to change that part.
|
|
||
| config PHP8_FULLICUDATA | ||
| bool "Add dependency to full ICU Data" | ||
| depends on PHP8_INTL |
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.
PHP8_INTL already depends on PHP8_GETTEXT so this should not be required? And after looking at 72dd9ee again, since this flag is only affecting the php8-mod-intl, I think we should revert to use Package/php8-mod-intl/config stuff, so that this option is displayed near the package.
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.
I moved it because Kconfig did not want to behave properly with it as Package/php8-mod-intl/config. I don't know if it's just the vagaries of Kconfig or if there is a bug to chase down, and in another couple of instances.
| $(eval $(call BuildModule,gmp,GMP,+PACKAGE_php8-mod-gmp:libgmp)) | ||
| $(eval $(call BuildModule,iconv,iConv,$(ICONV_DEPENDS))) | ||
| $(eval $(call BuildModule,intl,Internationalization Functions,@PHP8_INTL +PACKAGE_php8-mod-intl:icu +PHP8_FULLICUDATA:icu-full-data)) | ||
| $(eval $(call BuildModule,intl,Internationalization Functions,@PHP8_GETTEXT @PHP8_INTL +PACKAGE_php8-mod-intl:php8-mod-gettext +PHP8_FULLICUDATA:icu-full-data +!PHP8_FULLICUDATA:icu)) |
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.
@PHP_GETTEXTshould not be required here since@PHP8_INTLalready depends on it, right?- I'd prefer to leave the dependency to icu and icu-full-data as is: the dependency to
icuis always required, and the dependency withPHP8_FULLICUDATAtoicu-full-datais in my eyes just a "convenience" helper.
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.
Kconfig issues again.
| config PHP8_INTL | ||
| bool "Enable Internationalization" | ||
| depends on PHP8_GETTEXT | ||
| default y |
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.
I have to do more tests, but at the moment, I don't see that PHP8_INTL must depend on PHP8_GETTEXT.
But on the other side, I think we should add to PHP8_LIBXML, PHP8_GETTEXT and PHP8_INTL a depends on depends on PACKAGE_php8 so that all options are hidden, if the "top-level" config option is not selected.
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 was because Kconfig was behaving strangely without 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.
Also adding a depends on PACKAGE_php8 gave me recursive dependency Kconfig problems.
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.
I am not sure if I did try PHP8_INTL without PHP8_GETTEXT, that may be able to be disentangled.
Fixes: php8: global package dependency changes based on module selection Fixes: #28078 As described in #28078 and #28075, Some binaries gain a dependency on libstdcpp when mod-gettext is included in the build, however this was not explicitly declared, so packaging fails with (e.g.): Package php8-cgi is missing dependencies for the following libraries: libstdc++.so.6 In contrast to #28075, this commit takes the approach: * Make use of --with-gettext depend on a configure flag (enabled by default, since that matches current full build behaviour) * Make sub-packages which require --with-gettext depend on the configure flag This means that e.g. php-cgi would not have gettext support if the configure flag was disabled, and e.g. php-mod-gettext and php-mod-intl would not be selectable. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
|
@mhei I have not given up on this - I've just not had a chance to try again. Kconfig can be a pain to work with. |
📦 Package Details
Maintainer: @mhei
Description:
This commit cleans up and enhances the changes from 6d6233b, f9591b8, e6c59b5, and properly implements the attempt from 996046e.
By reasoning carefully about the impact of combinations of configuration options, and thoroughly setting config options and checking actual dynamic dependencies using
readelf --dynamic, the configuration and dependency handling of these options has been improved.In addition, cleanups and enhancements like reorganizing CONFIG_DEPENDS, adding more Kconfig help text, and cleaning up some whitespace have been performed.
🧪 Run Testing Details
The following have been tested using Zabbix server frontend as the PHP test application:
✅ Formalities