win,v8: Support for MSVS 2015 in v0.12 #2843
Conversation
This is a port of 16bcd68 . Original commit message: The original change that added support for running custom actions during the install process (e7c84f8) assumed that Visual Studio 2013 is used to generate the installer file. However, that is not always the case, and older versions of Visual Studio should allow users to generate Windows installer files. This change makes the custom actions visual studio project use the visual studio version that is found by vcbuild.bat. Reviewed-By: João Reis <reis@janeasystems.com> PR-URL: nodejs/node-v0.x-archive#25569
This is a port of e192f61 . Original commit message: Older WiX versions included a header with extern "C" declaration, hence the custom action source must be C++. Reviewed-By: João Reis <reis@janeasystems.com> PR-URL: nodejs/node-v0.x-archive#25569
This is a port of a525c72 . Original commit message: Gyp update to be able to generate VS2015 projects. PR-URL: nodejs/node-v0.x-archive#25857 Reviewed-By: Alexis Campailla <alexis@janeasystems.com>
This is a port of eb459c8 , used as a floating patch over gyp. Original commit message: This issue has already submitted to the upstream in https://code.google.com/p/gyp/issues/detail?id=477 Use this commit until the upstream is to be fixed. PR-URL: nodejs#1325 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> This was ported to v0.10 in nodejs/node-v0.x-archive#25857
Backports http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=aab3560b65b9254d17770bb6fe3ca7edd7451429 from openssl upstream, to add support for Visual Studio 2015. This is already included in the newer openssl 1.0.2. Original commit message: e_os.h: limit _MSC_VER trickery to older compilers. PR: nodejs#3390 Original pull request: http://rt.openssl.org/Ticket/Display.html?user=guest&pass=guest&id=3390 This was ported to v0.10 in nodejs/node-v0.x-archive#25857
To compile with VS2015, the == and != methods in zone-allocator.h need to be marked const. This change was introduced upstream in https://chromium.googlesource.com/v8/v8.git/+/f9e4527f32c2c268cb79428c74ea9703e9db3aec
This is a port of 4208dc4 . Original commit message: PR-URL: nodejs#2036 Reviewed-By: Alexis Campailla <alexis@janeasystems.com> This was ported to v0.10 in nodejs/node-v0.x-archive#25857
This is a port of b0dd3bf . Original commit message: Windows 10 wasn't listed in the executable manifest. This caused problems with trying to detect Windows 10 via `os.release()`. PR-URL: nodejs#2332 Reviewed-By: Roman Reiss <me@silverwind.io>
joaocgreis
added
windows
|
|
||
| def GetPreferredTryMasters(_, change): | ||
| return { | ||
| 'tryserver.nacl': { t: set(['defaulttests']) for t in TRYBOTS }, |
There was a problem hiding this comment.
Use set literal, like this {'defaulttests'}
There was a problem hiding this comment.
this is from gyp upstream I believe, no point changing it here
|
sgtm rubber-stamp lgtm if it works I suppose although I'd prefer more qualified eyes to review |
|
|
||
|
|
||
| def GypTestFormat(title, format=None, msvs_version=None): | ||
| def GypTestFormat(title, format=None, msvs_version=None, tests=[]): |
There was a problem hiding this comment.
Using tests=[] is practically not advisable. Use tests=None and then inside the function do,
if tests is None:
tests = []
|
@thefourtheye no point in trying to review the deps code :P |
|
|
||
| def _Translate(value, msbuild_settings): | ||
| # Let msbuild-only properties get translated as-is from msvs_settings. | ||
| tool_settings = msbuild_settings.setdefault(tool.msbuild_name, {}) |
There was a problem hiding this comment.
This line actually modifies msbuild_settings dictionary. Is that intentional? If only the default value is required, the correct way to do this is msbuild_settings.get(tool.msbuild_name, {})
|
@Fishrock123 Ah, sorry. I find many problematic code in this :( |
|
Is JaneaSystems@b066e6a really needed? Other than that LGTM. |
|
@orangemocha That commit is a floating patch over gyp in master. It's not strictly essential, but someone probably expects MacOSX to work without XCode, and it was a straightforward cherry pick. Let me know if you think it should be removed. |
| call "%VS140COMNTOOLS%\..\..\vc\vcvarsall.bat" | ||
| SET VCVARS_VER=140 | ||
| ) | ||
| if not defined VCINSTALLDIR goto msbuild-not-found |
There was a problem hiding this comment.
Shouldn't that be goto vc-set-2013?
There was a problem hiding this comment.
master and v0.12 diverged on this. I guess which strategy is better depends on whether VCINSTALLDIR not being set mean an error in vcvarsall.bat, or a VS installation without VC++. I would err on the side of trying the next version.
|
Okay, the Xcode commit is fine by me. CI is yellow (only fail test is test-dns). |
Try the next version of Microsoft Visual Studio when vcvarsall.bat fails to set VCINSTALLDIR.
|
@bnoordhuis @orangemocha Updated |
This includes all of the commits needed to support MSVS 2015 and MSI generation with it, and a manifest update to enable
os.release()to report the correct version on Windows 10. All of this is already in v0.10.I had to mark
==and!=as const inzone-allocator.h(3rd commit from HEAD). That was done upstream in https://chromium.googlesource.com/v8/v8.git/+/f9e4527f32c2c268cb79428c74ea9703e9db3aec . I did not backport the full commit because it changes other parts that are not needed. @nodejs/v8 any issues with this?Fixes nodejs/node-v0.x-archive#25896
@rvagg I know it's late, but any chance of including in v0.12.8?
cc @nodejs/platform-windows @nodejs/lts