PHP 7.2 Windows build broken with VS2017 15.3

[Jan-E]

@stesie When building PHP 7.2.0 RC1 on Windows with the latest Visual Studio 2017 (15.3) compiling fails. Error messages can be found here:

https://ci.appveyor.com/project/Jan-E/v8js/build/php_7_2_appveyor.27/job/dapf088hnuev7qaa#L1662

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\include\fstream(18): error C2059: syntax error: 'namespace' (compiling source file ext\v8js\v8js_v8.cc)
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\include\fstream(18): error C2143: syntax error: missing ';' before '{' (compiling source file ext\v8js\v8js_v8.cc)
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\include\fstream(18): error C2447: '{': missing function header (old-style formal list?) (compiling source file ext\v8js\v8js_v8.cc)

Appveyor has an option to use the previous VS 2017 and with that one compiling succeeds and all tests pass (or are skipped).

For both configs a did a 'type include\fstream'. Line 18 of fstream is this one: https://ci.appveyor.com/project/Jan-E/v8js/build/php_7_2_appveyor.27/job/dapf088hnuev7qaa#L148 The third line in this snippet is the faulting one:

namespace experimental {
    namespace filesystem {
        inline namespace v1 {
class path;
        }   // inline namespace v1
    }   // namespace filesystem
}   // namespace experimental

The fstream in VS2017 15.2 does not have any lines regarding namespaces: https://ci.appveyor.com/project/Jan-E/v8js/build/php_7_2_appveyor.26/job/nkibuoc7o6yw4rul#L131

The comparable VS2017 15.3 fstream starts here: https://ci.appveyor.com/project/Jan-E/v8js/build/php_7_2_appveyor.27/job/dapf088hnuev7qaa#L131

BTW: it took me 27 appveyor runs to pinpoint the problem. I deleted all but the two relevant ones: https://ci.appveyor.com/project/Jan-E/v8js/history

[stesie]

Hey, I think the problem is not with the namespace keyword, but with the inline which somehow is overwritten by php's build system on Windows.

Please try removing the line saying ZEND_WIN32_FORCE_INLINE in config.w32

[Jan-E]

No, still the same error. https://github.com/Jan-E/v8js/commits/php_7_2_appveyor https://ci.appveyor.com/project/Jan-E/v8js/build/php_7_2_appveyor.28/job/pbuxgoribdcvd0l9#L1662

[Jan-E]

Any other ideas how to fix the build? Or should we ask @weltling to look at it?

[weltling]

@Jan-E please try /DZEND_WIN32_KEEP_INLINE /UZEND_WIN32_FORCE_INLINE. I couldn't fix this before in time unfortunately, will go for 7.3 now as any of 7.0+ needs a fix for this.

Thanks.

[Jan-E]

Sorry, still no luck: https://ci.appveyor.com/project/Jan-E/v8js/history https://ci.appveyor.com/project/Jan-E/v8js/build/php_7_2_appveyor.29/job/aev3d89pv6n1ae2t#L1662

Is it something that has to be changed in PHP core?

[weltling]

@Jan-E ah, my bad, there's one tricky part yet. It should be similar to

ADD_FLAG("CFLAGS_BD_EXT_V8JS", ' /D ZEND_WIN32_KEEP_INLINE=1 /U ZEND_WIN32_FORCE_INLINE ');

It has to do with the order the flags are put in the command line, please check the makefile for the details. Perhaps need more investigation, if this didn't work out.

In general true, a fix in the core is due. There is inline namespace feature in C++11, which now collides with C. In C, we trick inline to be _forceinline, so it gets wrong with the newer C++ standard. Was no issue in C++ before 11 of course.

Thanks.

[Jan-E]

OK, will try that later. Not at my laptop now. Which is a rare occasion...

[Jan-E]

ADD_FLAG("CFLAGS_BD_EXT_V8JS", ' /D ZEND_WIN32_KEEP_INLINE=1 /U ZEND_WIN32_FORCE_INLINE ');

This had to be added after the EXTENSION command in config.w32. https://ci.appveyor.com/project/Jan-E/v8js/history https://ci.appveyor.com/project/Jan-E/v8js/build/php_7_2_appveyor.32/job/lp474ma859g3nvaf#L2282

There is a new PR with the fix in config.w32: https://github.com/phpv8/v8js/pull/327 @stesie's appveyor shows OK results for PHP 7.0 & 7.1 as well: https://ci.appveyor.com/project/stesie/v8js/build/1.0.215 https://ci.appveyor.com/project/stesie/v8js/build/1.0.215/job/b1op8dkrar830rwk#L963 https://ci.appveyor.com/project/stesie/v8js/build/1.0.215/job/u45d8wxvayocli93#L856

Please merge the PR

[stesie]

Thanks very much for sorting this out @Jan-E! What do you think, shall we already integrate PHP 7.2 into AppVeyor build?

Thanks also @weltling for the quick reply