Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

win,build: add Visual Studio 2017 support #11867

Closed
wants to merge 3 commits into from

Conversation

@joaocgreis
Copy link
Member

commented Mar 15, 2017

This uses the same method that landed in node-gyp in nodejs/node-gyp#1130, using PowerShell to compile and execute a C# script that queries the COM server installed by Visual Studio 2017. The script is almost the same as the one in node-gyp, but adapted to generate a small .bat file that sets environment variables instead of JSON output.

A backport of the Gyp patch under review in https://chromium-review.googlesource.com/c/433540 is included (cherry-picked from #11084). This is a different approach than the one used in node-gyp. There, gyp is forced to use VS2015 (generating projects that are compatible with 2017), but to do that here would require deeper changes in configure. Since we already have a few more floating patches, adding this seems cleaner and easier to maintain to me.

cc @nodejs/platform-windows @nodejs/build

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Windows, build.

Add support for creating VS2017 projects to gyp.
@joaocgreis joaocgreis added the windows label Mar 15, 2017
@seishun seishun force-pushed the nodejs:master branch to b5eccc4 Mar 15, 2017
@joaocgreis joaocgreis referenced this pull request Mar 15, 2017
3 of 3 tasks complete
@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2017

Add support for Visual Studio 2017. A C# script, compiled and
executed by PowerShell, is used to query the COM server about the
location of VS installation and installed SDK version, filtering
installations that miss the required components.
@joaocgreis joaocgreis force-pushed the JaneaSystems:joaocgreis-H3A-vs2017 branch to a1d8608 Mar 15, 2017
else
continue;

Console.WriteLine(String.Format(" - Found {0}", id));

This comment has been minimized.

Copy link
@kunalspathak

kunalspathak Mar 15, 2017

Contributor

onsole.WriteLine(String.Format(" - Found {0}", id)); [](start = 17, length = 53)

unreachable?

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Mar 15, 2017

Author Member

Is reached when any of the if conditions above is true

This comment has been minimized.

Copy link
@kunalspathak

kunalspathak Mar 16, 2017

Contributor

Ah, ofcourse. I misread the code.

if (Win10SDKVer > 0)
{
Console.WriteLine(" - Using this installation with Windows 10 SDK");
string[] lines = { String.Format("set \"VS2017_INSTALL={0}\"", path), String.Format("set \"VS2017_SDK=10.0.{0}.0\"", Win10SDKVer) };

This comment has been minimized.

Copy link
@kunalspathak

kunalspathak Mar 15, 2017

Contributor

"set "VS2017_INSTALL={0}"" [](start = 53, length = 28)

Have a string variable for this and resuse it below as well.

@@ -2662,6 +2662,15 @@ def _GetMSBuildGlobalProperties(spec, guid, gyp_file_name):
else:
properties[0].append(['ApplicationType', 'Windows Store'])

msvs_windows_sdk_version = None
if msvs_windows_sdk_version == None and version.ShortName() == '2017':
vs2017_sdk = '10.0.14393.0'

This comment has been minimized.

Copy link
@kunalspathak

kunalspathak Mar 15, 2017

Contributor

10.0.14393.0 [](start = 18, length = 12)

I didn't follow your changes in gyp, but is this always safe to consider this as default version?

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Mar 15, 2017

Author Member

It may not always be installed, but is the one selected by the installer when the "Desktop development with C++" workload is selected, so it should be the most common one. If it is not installed, compilation will fail later, but it's better than failing here unconditionally. Should this be something else?

This comment has been minimized.

Copy link
@kunalspathak

kunalspathak Mar 16, 2017

Contributor

I don't mind keeping this version. I just wanted to know where this came from.


In reply to: 106287872 [](ancestors = 106287872)

This comment has been minimized.

Copy link
@refack

refack Mar 16, 2017

Member

I would rather see an early fail then use an arbitrary default.
P.S. When "Windows Creators Edition" will be released the default version will be 10.0.15052.*

if msvs_windows_sdk_version == None and version.ShortName() == '2017':
vs2017_sdk = '10.0.14393.0'
vs2017_sdk = os.environ.get('vs2017_sdk', vs2017_sdk)
if vs2017_sdk:

This comment has been minimized.

Copy link
@kunalspathak

kunalspathak Mar 15, 2017

Contributor

vs2017_sdk [](start = 7, length = 10)

Correct me, but this will always be true, right?

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Mar 15, 2017

Author Member

I don't know of a way to set an empty variable in cmd, but it's possible that the variable is set from another program that calls this. Something like os.environ['vs2017_sdk'] = '' in python or empty variable when using gitbash.

This comment has been minimized.

Copy link
@kunalspathak

kunalspathak Mar 16, 2017

Contributor

Yeah, AFAIK, there is not way to set empty variable in cmd. Empty variable is equivalent of not defining environment variable. Python, definitely you could do that in which case the check makes sense.

vs2017_sdk = os.environ.get('vs2017_sdk', vs2017_sdk)
if vs2017_sdk:
msvs_windows_sdk_version = vs2017_sdk
if msvs_windows_sdk_version:

This comment has been minimized.

Copy link
@kunalspathak

kunalspathak Mar 15, 2017

Contributor

msvs_windows_sdk_version [](start = 5, length = 24)

and probably this as well will always be true?

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Mar 15, 2017

Author Member

Following from above, can be false.

public static void Query()
{
ISetupConfiguration query = new SetupConfiguration();
ISetupConfiguration2 query2 = (ISetupConfiguration2)query;

This comment has been minimized.

Copy link
@seishun

seishun Mar 15, 2017

Member

Use var to prevent redundancy?

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Mar 16, 2017

Author Member

We're trying to support Windows 7/2008R2 with the tools that it ships with by default, that means C# version 2. var was only introduced in version 3.

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2017

Updated.

@kunalspathak @seishun thanks for your reviews!

@kunalspathak

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

LGTM

@refack

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

  1. I think that cherry picking changes from node-gyp/gyp is gonna be a PITA.
  2. Anyway you should at least use my new GetVS2017Configuration.cs it has quite a bit of traction, and has a few bugs solved.
  3. I think you should run vcvarsall.bat as msbuild.exe would fail otherwise.
@refack refack referenced this pull request Mar 17, 2017
{
Console.WriteLine(" - Using this installation with Windows 8.1 SDK");
string[] lines = { setPath, "set \"VS2017_SDK=8.1\"" };
System.IO.File.WriteAllLines(@"Set_VS2017.bat", lines);

This comment has been minimized.

Copy link
@refack

refack Mar 17, 2017

Member

This smell bad...

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Mar 21, 2017

Author Member

Should this be done differently?

uses_vcxproj=True,
path=path,
sdk_based=sdk_based,
default_toolset='v141'),
'2015': VisualStudioVersion('2015',

This comment has been minimized.

Copy link
@refack

refack Mar 17, 2017

Member

So this is a de-novo patch, not based on the strategy in node-gyp:configure.js .
Could be done in configure which generates node.gypi

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Mar 21, 2017

Author Member

Node-gyp is very adverse to floating patches, but that is not the case here. I'd rather have the floating patch and not hack gyp. This patch is small enough to be reviewed here, and contained in a git commit so it can be easily reverted if a different approach lands upstream.

@rem Look for Visual Studio 2015
:vc-set-2015
if defined target_env if "%target_env%" NEQ "vc2015" goto msbuild-not-found

This comment has been minimized.

Copy link
@refack

refack Mar 17, 2017

Member

This breaks the current usage. vcbuild.bat should work with no arguments.

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Mar 21, 2017

Author Member

vcbuild.bat works with no arguments. This only takes effect if the user wants to select a specific version of VS to use.

This comment has been minimized.

Copy link
@refack

refack Mar 21, 2017

Member

You're right, missed the if defined target_env

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2017

@refack thanks for your review!

  1. I think that cherry picking changes from node-gyp/gyp is gonna be a PITA.

Note there are two versions of gyp in this repository, one under deps/npm/node_modules/node-gyp/gyp and another under tools/gyp. They are already different and separately maintained (the first is maintained in node-gyp).

  1. Anyway you should at least use my new GetVS2017Configuration.cs it has quite a bit of traction, and has a few bugs solved.

I'd rather use this trimmed down version that is easier to maintain here. It should need very little changes. What bugs do you see here?

  1. I think you should run vcvarsall.bat as msbuild.exe would fail otherwise.

What do you mean, instead of VsDevCmd.bat? I did not find any advantage of one over the other, only that vcvarsall.bat was not present in the RC version. msbuild.exe does not fail for me, does it for you? Can you paste your output (or make a gist perhaps, if it's big)?

@refack

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2017

Not instead, just sooner. Now it's only run before build I suggest running it before configure and use it's ENV for downstream decisions.​

It is run before configure. It is run in line 160, configure is in 208 and build in 220. Did I misunderstood?

@refack

This comment has been minimized.

Copy link
Member

commented Apr 12, 2017

It is run before configure. It is run in line 160, configure is in 208 and build in 220. Did I misunderstood?

Sorry I didn't follow the code flow correctly, I thought vcvarsall.bat was called only before the msbuild call

@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017
@ghost
ghost approved these changes Apr 16, 2017
@refack

This comment has been minimized.

Copy link
Member

commented Apr 16, 2017

@joaocgreis now that we have VS2017 in upstream GYP I think I'm gonna remove the detection code, and delegate the responsibility to the dev (like I suggested in libuv) to run vcvarsall.bat by themselves. Sounds good?

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2017

@refack making the users call vcvarsall.bat directly, increasing the complexity to build node, doesn't seem a good solution to me. What landed in Gyp upstream uses an unsupported method that may break any time, we have all the work done here so we should adapt and go forward.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

@refack making the users call vcvarsall.bat directly, increasing the complexity to build node, doesn't seem a good solution to me. What landed in Gyp upstream uses an unsupported method that may break any time, we have all the work done here so we should adapt and go forward.

hhhmn, Okay... (at least we can remove the GYP trickery)

@refack

This comment has been minimized.

Copy link
Member

commented May 16, 2017

@joaocgreis close this?

@refack

This comment has been minimized.

Copy link
Member

commented May 16, 2017

P.S. If I was too harsh or dismissive two months ago it was not meant as disrespect. It just took me some time to tune my language and behaviour to this community so that my intentions and feelings will be more easily understood.
So a general "Sorry! 🙏", and an explicit "Thank you very much for all the great work! 🥇"

@bzoz

This comment has been minimized.

Copy link
Contributor

commented May 19, 2017

Closing, see: #11852

@bzoz bzoz closed this May 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.