Don't use the "-2" flag with the py.exe launcher on Windows

[DeeDeeG]

Checklist

• npm install && npm test passes
• tests are included
• documentation is changed or added
• commit message follows commit guidelines

Description of change

Fixes #2130

The py.exe launcher for Windows helps to locate Python on the system.

The -2 or -3 flags can specify a major version to find. Now that node-gyp supports Python 3, it makes sense to not restrict the py.exe launcher to only finding Python 2. It would be advantageous to let it find Python 3 as well.

The py.exe launcher prefers newer (higher version number) Python when multiple versions are found. (i.e. it prefers Python 3 over Python 2 if both are available).

[DeeDeeG]

I will update the commit message(s) to follow the commit guidelines, and probably squash them into one commit, unless maintainers prefer it broken up like this.

[cclauss]

Very impressive first time contribution! Thanks for seeing this and fixing it.

[DeeDeeG]

Some additional notes:

• If a similar fix is desired for the node-gyp 5.x branch, where Python 2 is somewhat preferentially found before Python3, I would suggest running with the -2 flag first, then without (or with the -3 flag).
• Long-term, if node-gyp ever takes the position that Python 2 is to be actively banished, the -3 flag would be useful for finding strictly Python 3.
• This was originally coded as a special case because the -2 flag wasn't possible to pass neatly to the execFile function. (According to the inline comments of this file.)
  • There's a chance there might be some code de-duplication possible here.
• I'm open to fixing nits/small code style issues if there are any. Just point them out. JS isn't really one of my strong areas.

[DeeDeeG]

I don't think [...this.argsExecutable] needs to be an array anymore. I'm not sure if the ... is needed, either.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax

https://stackoverflow.com/questions/31048953/what-do-these-three-dots-in-react-do

[cclauss]

Small moves please. Leave this PR as is and allow maintainers time to review it as is.

[DeeDeeG]

Tests pass with the following:

Suggested change

	this.run(this.pyLauncher, [...this.argsExecutable], false,
	this.run(this.pyLauncher, this.argsExecutable, false,

[DeeDeeG]

Will wait for updates from reviewers, thanks.

[rvagg]

yeah, go with your suggestion, it's correct 👍

[rvagg]

The py.exe launcher prefers newer (higher version number) Python when multiple versions are found. (i.e. it prefers Python 3 over Python 2 if both are available).

If that's true then this change seems good to me! I don't think I have a good way to vet that claim but I imagine @cclauss has more of a clue than me about such things so 👍 .

[DeeDeeG]

Hi, @rvagg. Regarding this:

The py.exe launcher prefers newer (higher version number) Python [. . . ]

I read that in the documentation of the py.exe launcher. Here are the relevant parts:

Unlike the PATH variable, the launcher will correctly select the most appropriate version of Python. It will prefer per-user installations over system-wide ones, and orders by language version rather than using the most recently installed version.

You should find that the latest version of Python you have installed is started [. . .]

If you have multiple versions of Python installed (e.g., 2.7 and 3.8) you will have noticed that Python 3.8 was started [. . .]

If you want the latest version of Python 2.x you have installed, try the command:

py -2

---

To verify this, I started with a Windows install with no Python on the system, and installed Python 2 and 3 with the python-[version_here].exe installers from python.org. I can confirm py.exe finds all of Python 3.8/3.7 and Python 2.7.

[DeeDeeG]

By the way: The thing about "per-user installs being preferred over system-wide ones" sounded a little odd (what if Python 3 is installed system-wide, and Python 2 is installed just for the current user? Would it launch Python 2 by default?) I tried to reproduce this behavior as described in the documentation, but I couldn't reproduce it.

I tested with the following Pythons installed:

• Python 3.8 (system-wide install)
• Python 3.7 (per-user install)
• Python 2.7 (per-user install)

py -0 (list available Pythons) gives the following output:

C:\Users\[User]>py -0
Installed Pythons found by py Launcher for Windows
 -3.8-32 *
 -3.7-32
 -2.7-32

The asterisk indicates the default Python that py.exe will run if no flags are passed to it. Indeed it does run Python 3.8:

C:\Users\[User]>py
Python 3.8.3 (tags/v3.8.3:6f8c832, May 13 2020, 22:20:19) [MSC v.1925 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.executable
'C:\\Program Files (x86)\\Python38-32\\python.exe'

I can't reproduce the "per-user install takes precedence over a system-wide install" at all. And I can't think of an official way to install the same version of Python both system-wide and per-user, because the install wizard won't let you do that. (You can install the 64-bit and 32-bit editions of the same Python version, but the py.exe launcher seems to treat these as totally separate Pythons, and the 64-bit edition is preferred regardless of which is system-wide installed or per-user installed.)

[DeeDeeG]

Force pushed with the code change discussed above.

Also added this to the bottom of the first commit:

Fixes: https://github.com/nodejs/node-gyp/issues/2130
Refs: https://docs.python.org/3/using/windows.html#launcher

Happy to adjust or remove that if requested, just let me now.

[cclauss]

The failing tests are unrelated to these changes. Please do not make further modifications. Keep it simple and give maintainers time to review your work. Further ongoning code changes and more paragraphs of text will only slow down the review process. Force push is frowned upon in Node.js repos.

[rvagg]

all sounds good, and py.exe is an official Python thing too, correct? so we'd expect to see this installed by default on Windows systems with some version of Python installed? Has it been shipping for long enough to likely be everywhere that Python is on Windows?

[cclauss]

Yes. py.exe is an official Python thing that has been around since 2011
https://www.python.org/dev/peps/pep-0397

[DeeDeeG]

I have the ability to test things and report back / explain the nitty-gritty technical details, so please feel free to direct mention me (@DeeDeeG) if there is anything you need.

[rvagg]

excellent work, landed in c255ffb, will get it into v7.0.0 this week