Skip to content

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