build, win: vcbuild improvements by bzoz · Pull Request #17015 · nodejs/node · GitHub

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

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

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

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

Will it still be relevant once #16969 is merged?

Yes

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.

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

: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.

Updated, PTAL

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).

Updated, PTAL.

ping?

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

Not the resident domain expert but LGTM, FWIW.

@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.

If nobody oposes, I will land this tomorrow

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

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

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

I'd drop "installation".

Updated, PTAL.

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

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.

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

Can I get +1?

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?

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.

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

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).

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

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

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.

?

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?

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.

I'm gonna land this then.

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>

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 outstanding review comments, 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 outstanding review comments, 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

-              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

-              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

build, win: vcbuild improvements by bzoz · Pull Request #17015 · nodejs/node · GitHub

Uh oh!

Uh oh!

build, win: vcbuild improvements - #17015

build, win: vcbuild improvements#17015bzoz wants to merge 3 commits intonodejs:masternodejs/node:masterfrom JaneaSystems:bartek-nice-vcbuildJaneaSystems/node:bartek-nice-vcbuildCopy head branch name to clipboard

Conversation

bzoz commented Nov 14, 2017

Checklist

Affected core subsystem(s)

Uh oh!

bzoz commented Nov 14, 2017

Uh oh!

bzoz commented Nov 16, 2017

Uh oh!

seishun commented Nov 16, 2017

Uh oh!

bzoz commented Nov 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Nov 29, 2017

Uh oh!

bzoz commented Dec 4, 2017

Uh oh!

seishun commented Dec 4, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bzoz commented Dec 7, 2017

Uh oh!

bzoz commented Dec 21, 2017

Uh oh!

seishun left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Feb 19, 2018

Uh oh!

bzoz commented Feb 19, 2018

Uh oh!

bzoz commented Feb 21, 2018

Uh oh!

build, win: vcbuild improvements#17015
bzoz wants to merge 3 commits into
nodejs:masterfrom
JaneaSystems:bartek-nice-vcbuild