-
Notifications
You must be signed in to change notification settings - Fork 8k
Refactor ext/mbstring/libmbfl/config.h usage #13713
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
Conversation
@petk Thanks for working on this. I don't know enough about this part of the build system to know if this PR is a good idea or not. 😆
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 change looks good to me. This causes ext/mbstring/libmbfl/ to diverge a bit more from the upstream, but as it's officially not kept in sync, I guess it's fine.
Perhaps we don't even need phpize build for the mbstring extension since it is part of the php-src core
It can be sometimes useful to be able to build a single extension from php-src
The change looks good to me. This causes
ext/mbstring/libmbfl/to diverge a bit more from the upstream, but as it's officially not kept in sync, I guess it's fine.
😄 PHP's vendored copy of libmbfl has diverged so much from upstream that it hardly makes sense to call it "libmbfl" any more. (Well, maybe that's an exaggeration, but not a big one.)
Yes, I agree. I'll change this differently without hardcoding the config.h... The built-in config.h file is kind of weird. Like something done in half. Even if it is completely forked library only used in php-src now. It's still good to separate these things as much as possible.
35ed3c5 to
f97e8d5
Compare
Ok, in the appended new commit there is now only the main/php_config.h file included. I think that when building with phpize the extension's config.h is a bit redundant and misses few CPP macros. So the new appended commit is targeted here.
This moves libmbfl configuration to mbstring extension level and syncs the include style used as of PHP-8.4 to be able to do simultaneous in-source and out-of-source builds. The HAVE_CONFIG_H file is when doing phpize build using Autotools. Checks for strcasecmp and strings.h are also moved to mbstring extension only.
f97e8d5 to
078f9de
Compare
I've now added more changes here. Details in the commit.
Probably the #include "libmbfl/config.h" should be also changed to #include <libmbfl/config.h> and this configuration header removed from being installed as it is used only in C files (I think).
This simplifies the libmbfl/config.h usage and removes duplicate compile definitions on Windows (HAVE_STRICMP).
The _WIN32 symbol is always defined when compiling on Windows with any current compiler, the HAVE_CONFIG_H symbol is defined only when doing phpize build inside mbstring extension (an edge case but if that is used by someone perhaps), otherwise the regular main/php_config.h is used.
This is sort of an addition to make it managing changes in #13516 easier. The include style of angle brackets is based on the linked PR. Perhaps we don't even need phpize build for the mbstring extension since it is part of the php-src core. But I'm not sure what is the accepted usage for this so I've added it like it was previously in the config.m4 file (the config.h included with absolute path).
I'm adding also @arnaud-lb in the PR to have an overview a bit and when @alexdowad has time for checking this a bit.
Regarding Windows, I have another change planned there with this HAVE_STRICMP but that is beyond the scope here and can be done in the future.