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: find and setup for VS2017 #1130

Closed
wants to merge 9 commits into from

Conversation

@joaocgreis
Copy link
Member

commented Feb 25, 2017

Reviewed #1103 :

  • Filter VS2017 installations using the installed components, to make sure the installation found is usable.
  • Added logging.

cc @refack @bzoz can you review?

@joaocgreis joaocgreis referenced this pull request Feb 25, 2017
@refack

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

Personally I would move logic from the C# to JS. I like that you use more of the information available to us from the COM, but I would filter and choose in JS. I also like the idea of a log, but maybe if the C# prints everything, there will be no need.
I'll take some ideas to #1103 and to windows-autoconf 😉

Reviewed-By: João Reis <reis@janeasystems.com>
@joaocgreis joaocgreis force-pushed the JaneaSystems:joaocgreis-H2O-vs2017 branch from aa8626e to 1f79c98 Feb 27, 2017
@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2017

Updated, fixed failure when there's no usable installation and store the MSBuild executable path in config so that it can respect --msvs_version. I considered filtering on the JS side, but it can be a huge list of components and is easy enough to do on the C# side. We can always improve if necessary.

Tested (with node v7 on Windows 7):

  • Several installations of VS2017, where only one is usable
  • VS2017 installed but not usable
  • After uninstalling VS2017
  • VSBT2015 and VS2017 side by side (--msvs_version works)
  • Run npm as a normal user
  • On Windows 10, node v4, running as a normal user
@bzoz
bzoz approved these changes Feb 28, 2017
@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

Is this PR taking a dependency on powershell and the C# compiler? That... seems unfortunate.

joaocgreis added a commit that referenced this pull request Feb 28, 2017
PR-URL: #1130
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

Landed in b456dd0

@bnoordhuis it does, but those two are installed by default everywhere I've looked. To make node-gyp work out of the box in the platforms we support, it's either this, comtypes as in #1101, or have an executable committed. This worked great everywhere I've tested, but it's not impossible that there is some uncommon Windows configuration that breaks. Based on actual issues, we can fix or decide for any of the other options.

@joaocgreis joaocgreis closed this Feb 28, 2017
@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

Merging is a little premature. There has been no sign-off from a committer and you didn't run the CI either.

@refack

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

LGTM(ish) I'll make a future PR with some more changes 😉

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

Took it back, I took @bzoz sign-off as enough, this has been under discussion in #1101 and #1103 for a long time. I'll run CI.

@joaocgreis joaocgreis reopened this Feb 28, 2017
@@ -14,7 +14,9 @@ var fs = require('graceful-fs')
, mkdirp = require('mkdirp')
, exec = require('child_process').exec
, processRelease = require('./process-release')
, win = process.platform == 'win32'
, win = process.platform === 'win32'
if (win)

This comment has been minimized.

Copy link
@refack

refack Feb 28, 2017

Member

You don't need this if you already injected config.variables.msbuild_path (which is smart)

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Feb 28, 2017

Author Member

Thanks!

@refack
refack approved these changes Feb 28, 2017
@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

@bnoordhuis CI is green, do you want to review? I think this is important to have, what should I do to move this forward?

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2017

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2017

Updated: changed the MSBuild dependency component. Installing only MSBuild does not make it usable for C++ if the workload is not installed. But installing "Visual Studio C++ core features" is enough, includes MSBuild.exe and the necessary files to make it usable.

@joaocgreis joaocgreis referenced this pull request Mar 8, 2017
@Fishrock123

This comment has been minimized.

Copy link
Member

commented Mar 13, 2017

@rvagg

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

working on this now FYI, had to get a few other pressing things out of the way but I'm reviewing and testing this out for myself now

Copy link
Member

left a comment

Requested some minor changes and a bit of feedback. If the 2015->2017 thing is a GYP hack then the rest are just minor fixes.

Tested in a few different configurations myself, works as it should. The .cs thing is unfortunate but I understand the rationale and don't think it's ultimately a big deal. 👍 and thanks for the hard work folks.

@@ -137,6 +139,18 @@ function configure (gyp, argv, callback) {
// disable -T "thin" static archives by default
variables.standalone_static_library = gyp.opts.thin ? 0 : 1

if (win && !(gyp.opts.msvs_version && gyp.opts.msvs_version !== '2017')) {

This comment has been minimized.

Copy link
@rvagg

rvagg Mar 14, 2017

Member

would you mind inverting this? reads really badly. (win && (!gyp.opts.msvs_version || gyp.opts.msvs_version === '2017'))

if (win && !(gyp.opts.msvs_version && gyp.opts.msvs_version !== '2017')) {
const vsSetup = findVS2017()
if (vsSetup) {
gyp.opts.msvs_version = '2015'

This comment has been minimized.

Copy link
@rvagg

rvagg Mar 14, 2017

Member

are we forcing this to make GYP happy, or is this (and the next line) a mistake?

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Mar 14, 2017

Author Member

We are forcing this to make GYP happy, and to avoid pulling https://chromium-review.googlesource.com/#/c/433540/ (which did not land yet) as a floating patch, since floating patches seem to be avoided here.


var vsSetup
try {
const vsSetupRaw = execSync(psQuery, { encoding: 'utf8' })

This comment has been minimized.

Copy link
@rvagg

rvagg Mar 14, 2017

Member

var for consistency, also given that node-gyp still works for very old versions of Node

This comment has been minimized.

Copy link
@rvagg

rvagg Mar 14, 2017

Member

sorry, I mean, s/var/const in this entire file

@@ -137,6 +139,18 @@ function configure (gyp, argv, callback) {
// disable -T "thin" static archives by default
variables.standalone_static_library = gyp.opts.thin ? 0 : 1

if (win && !(gyp.opts.msvs_version && gyp.opts.msvs_version !== '2017')) {
const vsSetup = findVS2017()

This comment has been minimized.

Copy link
@rvagg

rvagg Mar 14, 2017

Member

s/const/var

@@ -0,0 +1,262 @@
// powershell -ExecutionPolicy Unrestricted -Version "2.0" -Command "&{Add-Type -Path Find-VS2017.cs; [VisualStudioConfiguration.Main]::Query()}"

This comment has been minimized.

Copy link
@rvagg

rvagg Mar 14, 2017

Member

does this file need source attribution and/or copyright? or is it ours?

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Mar 14, 2017

Author Member

This was submitted by @refack in #1103 , I believe it was adapted from the python COM API by @bzoz in #1101 , which in turn got the GUIDs and API from the nuget package upstream. @refack can you confim?

This comment has been minimized.

Copy link
@refack

refack Mar 14, 2017

Member

I wrote it myself. GUIDs extracted from interop

This comment has been minimized.

Copy link
@refack

refack Mar 14, 2017

Member

Please add

// Copyright 2017 - Refael Ackermann
// Distributed under MIT style license
// See accompanying file LICENSE at https://github.com/node4good/windows-autoconf

This comment has been minimized.

@rvagg rvagg referenced this pull request Mar 14, 2017
3 of 3 tasks complete
@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2017

@rvagg updated, PTAL. About the 2015->2017 GYP hack, my comment above is hidden by GitHub:

We are forcing this to make GYP happy, and to avoid pulling https://chromium-review.googlesource.com/#/c/433540/ (which did not land yet) as a floating patch, since floating patches seem to be avoided here.

@refack

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

I recommand using updated file https://github.com/node4good/windows-autoconf/blob/master/tools/GetVS2017Configuration.cs
Same as one that landed in boost/build
And gets proper MS style CI at AppVeyor, which BTW runs node-gyp test suite

Copy link
Member

left a comment

LGTM. Should I run the CI now or wait for the update to the .cs file?

@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2017

Updated the COM API part of the file. I'd rather keep the code part as is here, with the filtering and the logging.

Started CI:

@bnoordhuis should we run other versions?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

Maybe do v4.x and v6.x just in case?

@refack refack referenced this pull request Mar 14, 2017
3 of 3 tasks complete
@rvagg
rvagg approved these changes Mar 15, 2017
@rvagg

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

@joaocgreis let's get this published today. Clean it up and get it merged and we'll publish and then PR npm to get it there too. Ping me on IRC when you're around and want to move forward.

joaocgreis added a commit that referenced this pull request Mar 15, 2017
PR-URL: #1130
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@joaocgreis

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2017

Landed in ae141e1

@joaocgreis joaocgreis closed this Mar 15, 2017
@joaocgreis joaocgreis referenced this pull request Mar 15, 2017
3 of 3 tasks complete
@rvagg

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

v3.6.0 released, thanks for the hard work on this @refack, @joaocgreis, @bzoz and anyone else who contributed, not a small effort

@rvagg

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

Attempted PRs to npm (I always get seem to get this process wrong):

@rvagg

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

OK, looks like we might not be getting v4 compatibility for VS 2017 from npm unless we patch it ourselves I guess @nodejs/lts npm/npm#16049

@refack

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

What's needed for v4 compat? Can I help?

@gibfahn

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

@refack issue is that Node 4 is going into maintenance mode soon, so there might not be another update to npm before that time. The ping to @nodejs/lts is for the LTS team to decide whether it's worth doing another v4 release to pick up the npm update.

FWIW, I think it's worth updating to support VS2017 in Node 4 (but then I won't have to do the release).

@refack

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

@gibfahn, thanks
Best I can do is make sure VS2017 changes to node-gyp are fully compatible with v4 (I'll add it to my CI matrix)

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

OK, looks like we might not be getting v4 compatibility for VS 2017 from npm unless we patch it ourselves I guess

I'm not sure that was read from the comment correctly but it could be the case.

@gibfahn

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

Unless Node.js is planning on doing a new v4 release for this(?), I don't see us doing a new npm@2 release.

Based on that comment, if we can get it into Node v4, we can get it into npm.

Node v4 goes into maintenance mode on 2017-04-01, i.e. in ten days. Whichever way we go, we need to make a decision ASAP.

@refack

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

👎
#1151 v3.6.0 might break a lot of actions out there 😟

@refack

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

@joaocgreis, @rvagg Let's fast track #1153

@gibfahn gibfahn referenced this pull request Mar 22, 2017
28 of 31 tasks complete
@refack

This comment has been minimized.

Copy link
Member

commented Mar 24, 2017

"Bring back VS150COMNTOOLS"
I urge everyone else to up vote at https://developercommunity.visualstudio.com/content/problem/35325/missing-vs150comntools.html

else if (id == "Microsoft.VisualStudio.Component.VC.Tools.x86.x64")
hasVCTools = true;
else if (id.StartsWith(Win10SDKPrefix))
Win10SDKVer = Math.Max(Win10SDKVer, UInt32.Parse(id.Substring(Win10SDKPrefix.Length)));

This comment has been minimized.

Copy link
@refack

refack Apr 18, 2017

Member

Bug found with new SDK #1144

@AndrewPardoe

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

The officially supported vswhere tool is now available as part of the install starting today with Visual Studio 15.2 preview 2: https://blogs.msdn.microsoft.com/heaths/2017/04/21/vswhere-is-now-installed-with-visual-studio-2017/. This doesn't fix all the existing installs of VS 2017 but going forward you can now detect the VS install locations and whether C++ tools are available from a scripted environment. I hope this helps your scenario.

@refack refack referenced this pull request Apr 23, 2017
2 of 2 tasks complete
@am11

This comment has been minimized.

Copy link

commented Feb 19, 2018

One approach is to download VSWhere as a fallback if not available and completely rely on it to find VS2017:

# powershell
$vswherePath = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe";
if (!(Test-Path $vswherePath)) {
    $latestReleaseTag = $(curl https://api.github.com/repos/Microsoft/vswhere/releases/latest | `
                          ConvertFrom-Json) | % { $_.tag_name };

    $vswherePath = "$pwd\vswhere.exe";
    curl -O $vswherePath `
        https://github.com/Microsoft/vswhere/releases/download/$latestReleaseTag/vswhere.exe;
}

$vsBase = &$vswherePath -latest -products * `
              -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 `
              -property installationPath;

if (!$vsBase) {
    throw "VS or MSVC component is not installed";
}

&"$vsBase\vc\Auxiliary\Build\vcvars64.bat";
# now link.exe, cl.exe etc. are in PATH

# or write vsvars path to stdout for the caller (MSBuild Exec task)
echo $vsBase\vc\Auxiliary\Build\vcvars64.bat;

However, in practice, vswhere is available in a deterministic location when we have VS or buildtools installed, then downloading is sort of moot if we don't want to support non-ProgramFiles installs (e.g. https://github.com/dotnet/corert/blob/378ad6a/src/BuildIntegration/findvcvarsall.bat).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.