Skip to content

fix: use C++ 17 compiler for ICU #414

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

Merged
merged 8 commits into from
Apr 22, 2024
Merged

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Apr 19, 2024

What does this PR do?

ICU now requires C++17.
This patch fixes the build.

Checklist before merging

If your PR involves the changes mentioned below and completed the action, please tick the corresponding option.
If a modification is not involved, please skip it directly.

  • If it's a extension or dependency update, make sure adding related extensions in src/global/test-extensions.php.
  • If you changed the behavior of static-php-cli, add docs in static-php/static-php-cli-docs .
  • If you updated config/xxxx.json content, run bin/spc dev:sort-config xxx.

@dunglas dunglas mentioned this pull request Apr 19, 2024
3 tasks
@dunglas
Copy link
Contributor Author

dunglas commented Apr 19, 2024

Thanks to a hack found by @devnexen, it's possible to force C++17!

@dunglas dunglas marked this pull request as ready for review April 19, 2024 13:36
@crazywhalecc crazywhalecc added bug Something isn't working kind/dependency Issues related to dependencies labels Apr 19, 2024
@crazywhalecc
Copy link
Owner

I always fail to compile locally (both macOS and Linux).

Random make log line contains -std=c++17 and -std=c++11, but I haven't found any scripts with c++11.

/bin/bash /home/jerry/static-php-cli/source/php-src/libtool --silent --preserve-dup-deps --tag=CXX --mode=compile g++ -std=c++17 -Iext/intl/ -I/home/jerry/static-php-cli/source/php-src/ext/intl/ -I/home/jerry/static-php-cli/source/php-src/include -I/home/jerry/static-php-cli/source/php-src/main -I/home/jerry/static-php-cli/source/php-src -I/home/jerry/static-php-cli/source/php-src/ext/date/lib -I/home/jerry/static-php-cli/buildroot/include -I/home/jerry/static-php-cli/source/php-src/TSRM -I/home/jerry/static-php-cli/source/php-src/Zend  -I/home/jerry/static-php-cli/buildroot/include -D_GNU_SOURCE  -g -O2  -I/home/jerry/static-php-cli/buildroot/include -DU_NO_DEFAULT_INCLUDE_UTF_HEADERS=1 -DU_HIDE_OBSOLETE_UTF_OLD_H=1 -Wno-write-strings -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1 -std=c++11  -DUNISTR_FROM_CHAR_EXPLICIT=explicit -DUNISTR_FROM_STRING_EXPLICIT=explicit -c /home/jerry/static-php-cli/source/php-src/ext/intl/timezone/timezone_methods.cpp -o ext/intl/timezone/timezone_methods.lo  -MMD -MF ext/intl/timezone/timezone_methods.dep -MT ext/intl/timezone/timezone_methods.lo
In file included from /home/jerry/static-php-cli/buildroot/include/unicode/unistr.h:39,
                 from /home/jerry/static-php-cli/buildroot/include/unicode/strenum.h:20,
                 from /home/jerry/static-php-cli/source/php-src/ext/intl/common/common_enum.h:21,
                 from /home/jerry/static-php-cli/source/php-src/ext/intl/common/common_enum.cpp:24:
/home/jerry/static-php-cli/buildroot/include/unicode/stringpiece.h:133:29: error: ‘enable_if_t’ in namespace ‘std’ does not name a template type
  133 |             typename = std::enable_if_t<
      |                             ^~~~~~~~~~~

@crazywhalecc
Copy link
Owner

crazywhalecc commented Apr 19, 2024

Maybe there is still a problem in specifying the std during compilation. Replacing file contents like php/php-src#14000 works fine for me: FileSystem::replaceFileStr($m4_file, 'PHP_CXX_COMPILE_STDCXX(11', 'PHP_CXX_COMPILE_STDCXX(17');

BTW glibc-based linux uses $ARCH-linux-musl-g++, and macOS uses clang++, I've changed it locally before tests.

@dunglas
Copy link
Contributor Author

dunglas commented Apr 20, 2024

I don't understand why I haven't the problem locally (Mac). Maybe should we just merge #415 in the meantime?

@crazywhalecc
Copy link
Owner

crazywhalecc commented Apr 22, 2024

My php-src build config.log part:

configure:49572: checking whether clang++ -std=gnu++11 supports C++11 features with -std=c++11
configure:49871: clang++ -std=gnu++11 -std=c++11 -c -g -O2 -I/Users/jerry/project/git-project/static-php-cli/buildroot/include -D_GNU_SOURCE conftest.cpp >&5

Possible solution: remove std=c++11 from somewhere?

@crazywhalecc
Copy link
Owner

crazywhalecc commented Apr 22, 2024

I caught this, but dont' know why: if we don't set CXX manually, it will automatically use clang++ -std=gnu++11. The solution is also simple: set CXX=clang++ -std=c++17 based on the existing patch to override.

@crazywhalecc
Copy link
Owner

Tests passed. If everything goes well I'll merge it.

@crazywhalecc crazywhalecc merged commit 6b96feb into crazywhalecc:main Apr 22, 2024
@dunglas dunglas deleted the fix/icu branch April 22, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working kind/dependency Issues related to dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants