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

cc @nodejs/build @nodejs/platform-windows

@refack

This approach sgtm, I don't really see the problem here though @seishun:

whenever significant changes are introduced in vcbuild.bat, one has to make sure find_python.cmd is called before the first Python usage

You have it at the top of the file, just under arg parsing, so any major changes are going to have to be above that line and it's not highly likely those changes would have a use for python above there anyway. To be extra sure you could just add a comment in the file to explain why it's being called so future committers have a warning at least.

Ping @seishun

@nodejs/collaborators we need to decide whether to merge this PR or #17293. The reasons why I prefer that one are mentioned in OP. That PR has 3 approvals and 1 change request (not sure if it's still relevant? cc @refack), this one has 2 approvals, so the opinions seem divided.

@seishun I think we should land this PR, since #17293 would break some user scenarios that currently work.

@bzoz If you mean the scenario where Python is installed but not added to PATH, then it doesn't work too well - it prints some error messages before proceeding with the build, then leaks PATH modifications to the cmd session:

C:\node>python
'python' is not recognized as an internal or external command,
operable program or batch file.

C:\node>vcbuild
ERROR: The system was unable to find the specified registry key or value.
ERROR: The system was unable to find the specified registry key or value.
Looking for Visual Studio 2017
calling: "C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\\Auxiliary\Build\vcvarsall.bat" amd64
^CTerminate batch job (Y/N)? y

C:\node>python
Python 2.7.14 (v2.7.14:84471935ed, Sep 16 2017, 20:19:30) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.

Since no one has reported these issues, I suspect very few people, if any, rely on this scenario.

#17015 will fix the error messages, PATH leak could be fixed also.

I guess we do not get reports, because after all compilation works, and those error messages are covered by miles of compilation output. Since the code is already here, it works, and it is not a chore to maintain I don't see a reason to drop a supported scenario.

it is not a chore to maintain

It does carry a maintenance burden as explained in OP. Whether it's greater than the usefulness of this scenario is subjective, so we probably won't convince each other. Unless TSC chimes in, I'll just land the PR that gains more approvals.

The other PR has -1 from @refack, so only this PR can land.

There was a lot of discussion after he requested changes. I asked him if it's still relevant in #17293 (comment) and didn't get a response.

I think it is still relevant and I'm also -1 for that PR. If you trace #13900 it is a fix for a real issue described in nodejs/CTC#147 (comment).

@seishun I do not really see the maintenance burden here and I feel like this one is not controversial so far. So as soon as you address @rvagg comments and there is a green CI, I would go ahead and land this.

@seishun would you be so kind and address the comments? As soon as that is done I would go ahead and land this.

I am going to close this since a mentioned alternative has landed. @seishun if this is not correct, please reopen.