tools: bump vswhere helper to 2.0.0 by refack · Pull Request #14557 · nodejs/node

refack · 2017-07-31T18:25:27Z

enable explicit detection of prerelease VS versions with
set VSWHERE_WITH_PRERELASE=1

/cc @nodejs/platform-windows @nodejs/build

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tools,build,windows

refack · 2017-07-31T18:26:39Z

Windows CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/10754/

refack · 2017-07-31T18:52:34Z

New CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/10755/

seishun

Switching between release and preview requires closing and re-opening cmd. Is that intended?

refack · 2017-07-31T21:18:07Z

Switching between release and preview requires closing and re-opening cmd. Is that intended?

So that is a tricky one, since we short-circuit the call to vcvarsall because it can take up to 60s you need to break it by resetting VisualStudioVersion=.
so set VisualStudioVersion=& VSWHERE_WITH_PRERELASE=1& vcbuild vs2017

refack · 2017-07-31T21:22:16Z

@seishun what I could do is treat vs2017 as a "force redetection argument" so:
vcbuild - use best version or the one currently set up
vcbuild vs2017 - force redetection of VS2017
vcbuild vs2015 - uses VS140COMNTOOLS which is a singelton anyway

But I'm not sure 🤔 ...

tniessen

I can't really test this locally as I am stuck to VS 2015 which does not have vswhere as far as I know, but the changes themselves LGTM. Hoping for some feedback from @nodejs/platform-windows who use VS 2017.

tniessen · 2017-07-31T23:36:07Z

tools/msvs/vswhere_usability_wrapper.cmd

Even though I don't think quotes are strictly necessary here, I would prefer consistency with line 8.

I usually don't like adding stuff "just to be safe", but since I don't know exactly why or how this might break (but it probably will for someone), I added them.

Actually I can't remember why I use the %Path% trick... But I do remember that "%InstallerPath%"\vswhere.exe doesn't work is some cases

seishun · 2017-08-01T06:45:01Z

we short-circuit the call to vcvarsall because it can take up to 60s

Really? It takes less than a second for me.

bzoz · 2017-08-01T10:50:42Z

vcbuild.bat

Is this on purpose or leftover from debugging?

🤦‍♂️
Removed.

bzoz · 2017-08-01T10:50:54Z

tools/msvs/vswhere_usability_wrapper.cmd

Missing new line

refack · 2017-08-01T11:08:11Z

we short-circuit the call to vcvarsall because it can take up to 60s

Really? It takes less than a second for me.

with VS2017? it used to be 10s, but it keeps getting worse (maybe because I have 3 installation side by side)

refack · 2017-08-20T01:58:41Z

Updated to v2.0.0 because there was a typo in VSWHERE_WITH_PREREALASE
PTAL

BridgeAR · 2017-08-31T17:31:49Z

Ping @bzoz

bzoz · 2017-09-01T05:52:40Z

LGTM if the CI is green. https://ci.nodejs.org/job/node-test-pull-request/9910/

benjamingr · 2017-09-01T09:05:19Z

Build failures look unrelated at https://ci.nodejs.org/job/node-test-commit-osx/12086//console (cc @nodejs/build ).

New CI: https://ci.nodejs.org/job/node-test-pull-request/9911/console

bzoz · 2017-09-01T14:23:47Z

I guess the only thing that really matters here is if node-compile-windows is green. It is, tests also look OK, so LGTM.

* enable explicit detection of prerelease VS versions with `set VSWHERE_WITH_PRERELEASE=1` PR-URL: nodejs#14557 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>

* enable explicit detection of prerelease VS versions with `set VSWHERE_WITH_PRERELEASE=1` PR-URL: nodejs/node#14557 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>

* enable explicit detection of prerelease VS versions with `set VSWHERE_WITH_PRERELEASE=1` PR-URL: #14557 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>

MylesBorins · 2017-09-20T19:50:10Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

MylesBorins · 2017-10-16T22:11:26Z

ping

MylesBorins · 2017-11-14T20:19:29Z

ping @refack

refack · 2017-11-14T20:20:27Z

No need as there's no VS2017 support on v6.x

nodejs-github-bot added install Issues and PRs related to the installers. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Jul 31, 2017

refack force-pushed the bump-vswhere-helper-1.15.3 branch from 97abb71 to 104cc4e Compare July 31, 2017 18:49

jasnell approved these changes Jul 31, 2017

View reviewed changes

refack requested review from seishun and tniessen July 31, 2017 20:05

refack self-assigned this Jul 31, 2017

refack changed the title ~~tools: bump vswhere helper to 1.15.3~~ tools: bump vswhere helper to 1.15.4 Jul 31, 2017

refack requested review from bzoz and joaocgreis July 31, 2017 20:06

seishun reviewed Jul 31, 2017

View reviewed changes

tniessen approved these changes Jul 31, 2017

View reviewed changes

refack changed the title ~~tools: bump vswhere helper to 1.15.4~~ tools: bump vswhere helper to 1.15.5 Aug 1, 2017

bzoz suggested changes Aug 1, 2017

View reviewed changes

refack force-pushed the bump-vswhere-helper-1.15.3 branch from 8020eb0 to 7b51cb1 Compare August 1, 2017 11:06

refack changed the title ~~tools: bump vswhere helper to 1.15.5~~ tools: bump vswhere helper to 2.0.0 Aug 31, 2017

refack force-pushed the bump-vswhere-helper-1.15.3 branch from a101565 to 75fe971 Compare August 31, 2017 18:32

bzoz approved these changes Sep 1, 2017

View reviewed changes

refack force-pushed the bump-vswhere-helper-1.15.3 branch from 75fe971 to 98d8db3 Compare September 3, 2017 21:17

refack merged commit 98d8db3 into nodejs:master Sep 3, 2017

refack deleted the bump-vswhere-helper-1.15.3 branch September 4, 2017 21:53

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the lts-watch-v6.x label Sep 20, 2017

refack added dont-land-on-v4.x and removed lts-watch-v6.x labels Nov 14, 2017

refack removed their assignment Oct 12, 2018

Uh oh!

Conversation

refack commented Jul 31, 2017

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Jul 31, 2017

Uh oh!

refack commented Jul 31, 2017

Uh oh!

seishun left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jul 31, 2017

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen Jul 31, 2017

Choose a reason for hiding this comment

Uh oh!

refack Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

refack Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

seishun commented Aug 1, 2017

Uh oh!

bzoz Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

refack Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

bzoz Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

refack Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 1, 2017

Uh oh!

refack commented Aug 20, 2017

Uh oh!

BridgeAR commented Aug 31, 2017

Uh oh!

bzoz commented Sep 1, 2017

Uh oh!

benjamingr commented Sep 1, 2017

Uh oh!

bzoz commented Sep 1, 2017

Uh oh!

MylesBorins commented Sep 20, 2017

Uh oh!

MylesBorins commented Oct 16, 2017

Uh oh!

MylesBorins commented Nov 14, 2017

Uh oh!

refack commented Nov 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

refack commented Jul 31, 2017 •

edited

Loading