build, win: vcbuild improvements #17015

bzoz · 2017-11-14T10:02:16Z

Adds extra a check for proper Python version. If it is not found, vcbuild will exit early.

If no VS2017 installation is found, vswhere_usability_wrapper.cmd would print could not find "vswhere". This removes that message, making output look nicer for VS2015 compilation.

Finally, this adds support for DEBUG_HELPER to vcbuild.bat. Similar to vswhere_usability_wrapper.cmd and find_python.cmd, if DEBUG_HELPER environment variable is set, echo off will not be called, making debugging easier.

Fixes: #16864

Checklist

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

Affected core subsystem(s)

build

bzoz · 2017-11-14T10:03:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11419/

bzoz · 2017-11-16T09:38:18Z

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

seishun · 2017-11-16T10:04:06Z

This removes that message, making output look nicer for VS2015 compilation.

Will it still be relevant once #16969 is merged?

bzoz · 2017-11-16T10:06:13Z

Yes

refack · 2017-11-16T15:12:22Z

tools/msvs/vswhere_usability_wrapper.cmd

@@ -28,5 +28,4 @@ for /f "usebackq tokens=*" %%i in (`vswhere %VSWHERE_ARGS%`) do (

 :no-vswhere
 endlocal
-echo could not find "vswhere"


Can we keep this. Or upstream to https://github.com/node4good/msvs-com-helper

This removes that message, making output look nicer for VS2015 compilation.

we just dropped "official" VS2015 support 🤷‍♂️

If you still want to keep output clean, I'd rather you change the call in vcbuild

node/vcbuild.bat

Line 199 in d217b28

call tools\msvs\vswhere_usability_wrapper.cmd

to:

if defined DEBUG_HELPER (call tools\msvs\vswhere_usability_wrapper.cmd) else (call tools\msvs\vswhere_usability_wrapper.cmd > nul)

If you still want to keep output clean, I'd rather you change the call in vcbuild

I'd prefer to keep vcbuild clean too.

Even without 2015, if no VS2017 is found, it will look like this:

Looking for Visual Studio 2017 could not find "vswhere" Failed to find a suitable Visual Studio installation. Try to run in a "Developer Command Prompt" or consult https://github.com/nodejs/node/blob/master/BUILDING.md#windows-1

I don't think it looks good, and the could not find "vswhere" is only helpful if you know vcbuild internals. We can use DEBUG_HELPER to debug vcbuild if needed.

Ok, I'll move it upstream.

refack · 2017-11-16T15:14:03Z

vcbuild.bat

@@ -1,4 +1,4 @@
-@echo off
+@if not defined DEBUG_HELPER @ECHO OFF


YAASS! (I have this floating in all my workspaces, and I keep forgetting to upstream it)

refack · 2017-11-16T15:15:56Z

vcbuild.bat

+echo Looking for Python 2.x
+call :run-python --version > nul 2> nul
+if errorlevel 1 echo Could not find Python 2.x installation && exit /b 1
+


:run-python will echo Could not find python2 so IMHO no need to echo again. (L561)

should end subroutine with either exit /b or goto :EOF

Agree, let's not duplicate the validation.

:run-python is very verbose, especially if no Python is found (see #16864). Stdout and stderr is redirected to nul, to keep it clean.

Then maybe :run-python should be fixed. I was planning to do it myself, I think it's a simple error in the code.

bzoz · 2017-11-22T14:04:45Z

Updated, PTAL

seishun · 2017-11-23T09:44:14Z

vcbuild.bat

@@ -577,6 +577,7 @@ set TAG=
 set FULLVERSION=
 :: Call as subroutine for validation of python
 call :run-python tools\getnodeversion.py > nul
+if errorlevel 1 exit /b 1


This is ugly. Basically, we run tools\getnodeversion.py, ignore the output, exit if it failed, then run it again, this time directly using a variable that's supposed to be internal to find_python.cmd.

could change the fake call to call :run-python --version

It would still be a hacky workaround for the limitation of run-python.

No argument here. This file has a lot of cmdism hacks, mainly because you can't redirect stdout into a variable.
I'll check if there's a way to use call as the for command.

I've checked before, there isn't. So a subroutine doesn't work as a replacement for the python command.

An alternative would be to replace run-python with find-python that would just populate a variable to be used instead of python. But then one would have to make sure that find-python is run before the first usage of Python and that it's not run if Python isn't needed, whenever a significant change is introduced into vcbuild.bat.

Honestly, I'd prefer to just revert 614dbbd since it introduces a lot of complexity in order to support an edge case that has never affected anyone to my knowledge. IMO it was landed prematurely (only one approval and not thoroughly tested).

bzoz · 2017-11-29T21:21:20Z

Updated, PTAL.

bzoz · 2017-12-04T17:37:28Z

ping?

seishun · 2017-12-04T17:43:05Z

Unfortunately, my comment in #17298 (comment) applies here as well, so I cannot approve this.

bnoordhuis

Not the resident domain expert but LGTM, FWIW.

bzoz · 2017-12-07T11:11:58Z

@seishun it does not fix all issues, but at least makes vcbuild slightly more user friendly without breaking anything. IMO it will be better to wait for proper python fix with this version, rather that the one from master.

bzoz · 2017-12-21T11:19:50Z

If nobody oposes, I will land this tomorrow

seishun

Blocking until either #17293 or #17804 lands due to the aforementioned reasons.

Removes extra erroor messages when Python is not installed. Removes "vswhere not found" message when no VS2017 installation is found. Adds support for DEBUG_HELPER to vcbuild.bat. Fixes: nodejs#16864

#18621 has landed

BridgeAR · 2018-02-17T00:04:15Z

CI https://ci.nodejs.org/job/node-test-pull-request/13228/

seishun · 2018-02-17T10:25:50Z

vcbuild.bat

-call tools\msvs\find_python.cmd
-if errorlevel 1 echo Could not find python2 & goto :exit
+echo Looking for Python 2.x
+call tools\msvs\find_python.cmd > NUL 2> NUL


Can the noise not be fixed inside find_python.cmd itself?

seishun · 2018-02-17T10:26:45Z

vcbuild.bat

-if errorlevel 1 echo Could not find python2 & goto :exit
+echo Looking for Python 2.x
+call tools\msvs\find_python.cmd > NUL 2> NUL
+if errorlevel 1 echo Could not find Python installation & goto :exit


I'd drop "installation".

bzoz · 2018-02-19T17:28:12Z

Updated, PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/13277/

bzoz · 2018-02-19T17:48:40Z

I've moved error messages to find_python.cmd. This way we can have special error message when we find Python, but the version is incorrect.

bzoz · 2018-02-21T07:33:43Z

CI after update: https://ci.nodejs.org/job/node-test-pull-request/13300/

Can I get +1?

seishun · 2018-02-21T10:32:45Z

I don't really see the point in separating error messages for "No Python found at all" and "Python 2.x specifically not found". Isn't the action to be taken by the user the same in both cases?

bzoz · 2018-02-21T16:57:04Z

Its mostly for the new people, like in Code + Learn, etc. If they have Python 3 installed, vcbuild complaining about "no python" will be confusing for some people. Telling them "I've found python but the version is wrong" makes it clear whats the problem.

gibfahn

Generally LGTM, but would be good to get another review from @nodejs/platform-windows

seishun · 2018-02-21T17:28:26Z

If they have Python 3 installed, vcbuild complaining about "no python" will be confusing for some people.

I'd say "No Python 2.x" is sufficient for both cases. Mentioning a Python 3.x installation seems excessive, it's just a different product (i.e. you can have both installed).

bzoz · 2018-02-21T18:13:45Z

Sure, saying "you don't have python2.x" is sufficient. But saying "you have wrong python version" is more helpful.

seishun · 2018-02-21T19:08:47Z

It's also misleading because it suggests it should be replaced with python 2.x.

gibfahn · 2018-02-21T21:05:21Z

It's also misleading because it suggests it should be replaced with python 2.x.

In that case wouldn't the answer be to be more specific rather than less, i.e. something like:

Found Python 3 but not Python 2. Please install Python 2.6 or 2.7.

?

bzoz · 2018-02-22T10:01:25Z

It will say Python found in [...], but it is not v2.x..

Again, this is to help out new people on the project, like in Code + Learn. The more info we provide the better. And easier for people running such workshops to get everyone started with developing Node.

I would like to land this, @seishun are you -1 on this?

seishun · 2018-02-22T10:43:48Z

I can't imagine a situation where this extra info would help new people. If anything, it's confusing because they might think they should uninstall Python 3.x.

I'm -0 on this.

bzoz · 2018-02-22T10:51:22Z

I'm gonna land this then.

bzoz · 2018-02-22T11:44:52Z

Landed in 887a2ba

Removes extra erroor messages when Python is not installed. Removes "vswhere not found" message when no VS2017 installation is found. Adds support for DEBUG_HELPER to vcbuild.bat. Fixes: #16864 PR-URL: #17015 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Removes extra erroor messages when Python is not installed. Removes "vswhere not found" message when no VS2017 installation is found. Adds support for DEBUG_HELPER to vcbuild.bat. Fixes: nodejs#16864 PR-URL: nodejs#17015 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Removes extra erroor messages when Python is not installed. Removes "vswhere not found" message when no VS2017 installation is found. Adds support for DEBUG_HELPER to vcbuild.bat. Fixes: #16864 PR-URL: #17015 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Removes extra erroor messages when Python is not installed. Removes "vswhere not found" message when no VS2017 installation is found. Adds support for DEBUG_HELPER to vcbuild.bat. Fixes: nodejs#16864 PR-URL: nodejs#17015 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

MylesBorins · 2018-08-07T22:27:07Z

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

nodejs-github-bot added build Issues and PRs related to build files or the CI. 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 Nov 14, 2017

bzoz mentioned this pull request Nov 16, 2017

Node Interactive Code & Learn issues on Windows #16278

Closed

gibfahn requested a review from refack November 16, 2017 14:59

gibfahn assigned gibfahn and unassigned gibfahn Nov 16, 2017

gibfahn requested a review from joaocgreis November 16, 2017 14:59

refack reviewed Nov 16, 2017

View reviewed changes

seishun reviewed Nov 23, 2017

View reviewed changes

seishun mentioned this pull request Nov 25, 2017

build: better error message on python fail #17298

Closed

3 tasks

bnoordhuis approved these changes Dec 6, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

seishun mentioned this pull request Dec 21, 2017

build,win: replace find-python subroutine with PYTHON variable #17804

Closed

1 task

seishun previously requested changes Dec 21, 2017

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

build, win: vcbuild improvements

766aa49

Removes extra erroor messages when Python is not installed. Removes "vswhere not found" message when no VS2017 installation is found. Adds support for DEBUG_HELPER to vcbuild.bat. Fixes: nodejs#16864

bzoz force-pushed the bartek-nice-vcbuild branch from 141e517 to 766aa49 Compare February 15, 2018 10:19

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2018

seishun reviewed Feb 17, 2018

View reviewed changes

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2018

fixup: nits

9ed2399

fixup: move error messages to find_python

71cb694

gibfahn approved these changes Feb 21, 2018

View reviewed changes

bzoz closed this Feb 22, 2018

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

MylesBorins added the backport-requested-v8.x label Aug 7, 2018

		@@ -1,4 +1,4 @@
		@echo off
		@if not defined DEBUG_HELPER @ECHO OFF

build, win: vcbuild improvements #17015

build, win: vcbuild improvements #17015

Conversation

bzoz commented Nov 14, 2017

Checklist

Affected core subsystem(s)

bzoz commented Nov 14, 2017

bzoz commented Nov 16, 2017

seishun commented Nov 16, 2017

bzoz commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzoz commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzoz commented Nov 29, 2017

bzoz commented Dec 4, 2017

seishun commented Dec 4, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bzoz commented Dec 7, 2017

bzoz commented Dec 21, 2017

seishun left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzoz commented Feb 19, 2018

bzoz commented Feb 19, 2018

bzoz commented Feb 21, 2018

seishun commented Feb 21, 2018

bzoz commented Feb 21, 2018

gibfahn left a comment

Choose a reason for hiding this comment

seishun commented Feb 21, 2018

bzoz commented Feb 21, 2018

seishun commented Feb 21, 2018

gibfahn commented Feb 21, 2018

bzoz commented Feb 22, 2018

seishun commented Feb 22, 2018

bzoz commented Feb 22, 2018

bzoz commented Feb 22, 2018

MylesBorins commented Aug 7, 2018