FriendsOfPHP / pickle

PHP Extension installer
Other
1.65k stars 88 forks source link

PHP Windows BC Break in compiler constants and definition #176

Closed pierrejoye closed 4 years ago

pierrejoye commented 4 years ago

Latest PHP windows breaks BC by removing all references to VC.

I understand it makes life easier for ext developers to map actual VS version, but the VC Runtime is absolutely key here.

As of now, we need to rewrite the binary and compiler detection in pickle to support it.

@weltling @cmb69 what was the reasoning for this BC change? Given that it is already in the wild, we need to deal with it. It also disappeared from phpinfo altogether. But it is kind of subobtimal, we had clear constants not human strings to define them and from a UX point of view, users are having more issues to identify which DLL they need to download (if they don't use pickle :)

pierrejoye commented 4 years ago

`C:\Apps\php>php pickle.phar install memcached

+---------------+---------------------+

| php Path | C:\Apps\php\php.exe | | php Version | 7.4.6 |

| Compiler | visual |

| Architecture | x64 |

| Thread safety | no |

| Extension dir | ext |

| php.ini | C:\Apps\php\php.ini |

+---------------+---------------------+`

This is what we get now from: https://github.com/FriendsOfPHP/pickle/blob/master/src/Engine/PHP.php#L68

pierrejoye commented 4 years ago

PHPINFO: 7.4: Compiler => Visual C++ 2017

7.3: Compiler => MSVC15 (Visual C++ 2017)

7.2.31: Compiler => MSVC15 (Visual C++ 2017)

pierrejoye commented 4 years ago

Hardcoded VC15 for now, but we lose custom PHP builds support on Windows. At least that will cover most of our users :)

cmb69 commented 4 years ago

@pierrejoye, that issue is supposed to have been resolved with PR #170.

I'm not absolutely sure why this output had been changed, but even if PHP 8.0 would report "MSVC16 (Visual C++ 2019)", the old detection would not work anymore, because of the renaming from "vc14", "vc15" to "vs16". This has been done, because "vc15" already was a misnomer, since the runtime version is actually 14.1x; and for VS 2019 the runtime version is 14.2x.

PS: PHP-7.4 has originally been built with VS 2019, so that versioning ("vc" vs. "vs") had been changed already in the development phase. Due to link instabilities (we had to rebuilt several dependencies for almost all new VS 2019 versions, and users had to always use the latest VS 2019), we postponed the switch to VS 2019 to PHP 8.

pierrejoye commented 4 years ago

It should have been vc141 :)

I think we need to find a way to be able to determine the PHP build runtime used. And map it in the binary downloads, core or external extensions. A constant as we have for windows version?

What do you think?

On Sat, May 30, 2020, 5:28 PM Christoph M. Becker notifications@github.com wrote:

@pierrejoye https://github.com/pierrejoye, that issue is supposed to have been resolved with PR #170 https://github.com/FriendsOfPHP/pickle/pull/170.

I'm not absolutely sure why this output had been changed, but even if PHP 8.0 would report "MSVC16 (Visual C++ 2019)", the old detection would not work anymore, because of the renaming from "vc14", "vc15" to "vs16". This has been done, because "vc15" already was a misnomer, since the runtime version is actually 14.1x; and for VS 2019 the runtime version is 14.2x.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FriendsOfPHP/pickle/issues/176#issuecomment-636311217, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACE6KCOIVJTDVV7EZAA3KDRUDNUZANCNFSM4NOQM4OA .

pierrejoye commented 4 years ago

The PR did not solve the VCn definition, which is used for windows extensions build naming.

Pickle took it out of php -v which per se is not so good. A more normalized way would be better.

On Sat, May 30, 2020, 7:37 PM Pierre Joye pierre.php@gmail.com wrote:

It should have been vc141 :)

I think we need to find a way to be able to determine the PHP build runtime used. And map it in the binary downloads, core or external extensions. A constant as we have for windows version?

What do you think?

On Sat, May 30, 2020, 5:28 PM Christoph M. Becker < notifications@github.com> wrote:

@pierrejoye https://github.com/pierrejoye, that issue is supposed to have been resolved with PR #170 https://github.com/FriendsOfPHP/pickle/pull/170.

I'm not absolutely sure why this output had been changed, but even if PHP 8.0 would report "MSVC16 (Visual C++ 2019)", the old detection would not work anymore, because of the renaming from "vc14", "vc15" to "vs16". This has been done, because "vc15" already was a misnomer, since the runtime version is actually 14.1x; and for VS 2019 the runtime version is 14.2x.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FriendsOfPHP/pickle/issues/176#issuecomment-636311217, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACE6KCOIVJTDVV7EZAA3KDRUDNUZANCNFSM4NOQM4OA .

cmb69 commented 4 years ago

It should have been vc141 :)

Yes, that would have reflected the runtime version best. However, we had already shipped PHP 7.2 and 7.3 as vc15, and then changing to vc141 would have been confusing (especially since the latest runtime is still named vcruntime140.dll). So we went with the internal Visual Studio version number (which is 15 for VS 2017 and 16 for VS 2019); that's why we changed "vc" to "vs".

A constant as we have for windows version?

May make sense (and could be easily accomplished). I'm not sure how relevant clang builds are on Windows, though. And off the top of my head, I wouldn't even know how the API no. is specified in that case.

Anyhow, also with regard to already existing PHP versions, it may make sense to read the linker version from the file header; basically, what https://github.com/php/php-src/pull/5576/files#diff-141d8db8904fa9829b4d608ba590f2a5 does, but I think we could read that info from the executable file directly (e.g. like https://www.powershellgallery.com/packages/PowerSploit/1.0.0.0/Content/PETools%5CGet-PEHeader.ps1).

Pickle took it out of php -v which per se is not so good.

Ah! PR #170 was only concerned with the info taken from phpinfo().

pierrejoye commented 4 years ago

Thanks @cmb69

It made me think about a tool we have in php-src exactly for that :)

C:\Apps\php>deplister.exe php.exe php7.dll,OK WS2_32.dll,OK SHELL32.dll,OK KERNEL32.dll,OK VCRUNTIME140.dll,OK api-ms-win-crt-runtime-l1-1-0.dll,NOTFOUND api-ms-win-crt-stdio-l1-1-0.dll,NOTFOUND api-ms-win-crt-heap-l1-1-0.dll,NOTFOUND api-ms-win-crt-string-l1-1-0.dll,NOTFOUND api-ms-win-crt-environment-l1-1-0.dll,NOTFOUND api-ms-win-crt-time-l1-1-0.dll,NOTFOUND api-ms-win-crt-utility-l1-1-0.dll,NOTFOUND api-ms-win-crt-convert-l1-1-0.dll,NOTFOUND api-ms-win-crt-math-l1-1-0.dll,NOTFOUND api-ms-win-crt-locale-l1-1-0.dll,NOTFOUND

How could I forget about that tool we built :)

It is part of php, and php distribution. We can try to detect and fall down to this tool if parsing fails.

cmb69 commented 4 years ago

We can try to detect and fall down to this tool if parsing fails.

We could, but currently the tool doesn't provide the desired information. I mean, it's VCRUNTIME140.dll for VS 2015-2019 (currently). Anyhow, I think PR #180 should resolve the issue for now.

pierrejoye commented 4 years ago

That PR solves the issue for now. Thanks @cmb69 :)