Fix getCurrentArchName detection #18
Conversation
When executing "cl.exe /? 2>&1" under some conditions, we may both get result and a return value not to be 0. For example: I install "VC++ 2017 v141 Tools(x86,x64)" and other simple and necessary components without installing "VC++ 2015.3 v140 Tools(x86,x64)" in VS 2017, I will get an exception of "Couldn't execute cl.exe." when running "phpsdk_deps -u", because the return value is 2 somehow. But actually, the script gets the right result of the cl.exe version, which should keep it running. Once "VC++ 2015.3 v140 Tools(x86,x64)" is installed, everything looks fine.
|
good job |
weltling
left a comment
There was a problem hiding this comment.
Thanks for the PR. In general looks good to me, please check the comments yet.
Thanks.
|
|
||
| exec("cl.exe /? 2>&1", $a, $status); | ||
| if ($status) { | ||
| throw new Exception("Couldn't execute cl.exe."); |
There was a problem hiding this comment.
Perhaps it would make sense to keep the exception here, but change the command to where cl.exe. In this case only the status matters. If this passed, one can proceed further and exec the actual cl.exe to parse the output.
There was a problem hiding this comment.
hmm... yep, this could be much better. I will make some tests tomorrow.
lib/php/libsdk/SDK/Config.php
Outdated
| } elseif(0 < preg_match(",x86,", $a[0])) { | ||
| self::setCurrentArchName("x86"); | ||
| } else { | ||
| throw new Exception("Couldn't execute cl.exe."); |
There was a problem hiding this comment.
The message should be like "couldn't determine arch".
|
Code changed for simpler. |
|
I might have expressed myself not clear enough previously. Please don't change the parsing part. The flow I've suggested is as follows
The latest change doesn't seem to be reliable. Fe on earlier versions it is under Thanks. |
|
Haha, I get what you mean now. |
|
Sorry, the IDE changed the code style |
When executing "cl.exe /? 2>&1" under some conditions, we may both get the output and a return value not to be 0.
For example: I install "VC++ 2017 v141 toolset (x86,x64)" and other necessary components without installing "VC++ 2015.3 v140 toolset for desktop (x86,x64)" in VS 2017, I will get an exception of "Couldn't execute cl.exe." when running "phpsdk_deps -u", because the return value is 2 somehow. But actually, the script gets the right output of the cl.exe version, which should keep it running.
Once "VC++ 2015.3 v140 toolset for desktop (x86,x64)" is installed, everything works fine.
Havn't tested it under more conditions, only mine.
My compiling environment is: Win10 Pro x64, VS 2017, no other software.
Installed components: Static analysis tools, Visual Studio C++ core features, VC++ 2017 v141 toolset (x86,x64), Windows Universal CRT SDK, Windows 8.1 SDK; with or without VC++ 2015.3 v140 toolset for desktop (x86,x64) makes it different.