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

DeeDeeG · 2020-05-15T18:58:02Z

Checklist

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

Description of change

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 · 2020-05-15T19:00:17Z

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 · 2020-05-15T19:37:53Z

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 · 2020-05-15T19:47:22Z

lib/find-python.js

-      `- executing "${this.pyLauncher}" to get Python 2 executable path`)
-    this.run(this.pyLauncher, ['-2', ...this.argsExecutable], false,
+      `- executing "${this.pyLauncher}" to get Python executable path`)
+    this.run(this.pyLauncher, [...this.argsExecutable], false,


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

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

Tests pass with the following:

Suggested change

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

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

Will wait for updates from reviewers, thanks.

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

rvagg · 2020-05-16T04:01:20Z

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 · 2020-05-16T21:21:17Z

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 · 2020-05-16T21:23:41Z

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

Now that node-gyp supports both Python 2 and Python 3, We don't need to explicitly try to find only Python 2. Fixes: nodejs#2130 Refs: https://docs.python.org/3/using/windows.html#launcher

We no longer use this flag, so testing for it would understandably fail!

DeeDeeG · 2020-05-16T22:09:04Z

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 · 2020-05-17T06:05:01Z

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 · 2020-05-17T10:02:46Z

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 · 2020-05-17T19:51:40Z

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

DeeDeeG · 2020-05-18T00:05:36Z

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.

Now that node-gyp supports both Python 2 and Python 3, We don't need to explicitly try to find only Python 2. Fixes: #2130 Refs: https://docs.python.org/3/using/windows.html#launcher PR-URL: #2131 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Rod Vagg <rod@vagg.org>

rvagg · 2020-05-18T01:10:10Z

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

cclauss approved these changes May 15, 2020

View reviewed changes

cclauss assigned sam-github, joaocgreis, rvagg, cjihrig, gengjiawen and richardlau May 15, 2020

DeeDeeG force-pushed the pylauncher-python3 branch from ee90b5c to ae8e2b3 Compare May 15, 2020 19:31

DeeDeeG commented May 15, 2020

View reviewed changes

DeeDeeG added 2 commits May 16, 2020 18:04

lib: drop "-2" flag for "py.exe" launcher

7d646ec

Now that node-gyp supports both Python 2 and Python 3, We don't need to explicitly try to find only Python 2. Fixes: nodejs#2130 Refs: https://docs.python.org/3/using/windows.html#launcher

test: drop "-2" flag in test-find-python.js

ee0b793

We no longer use this flag, so testing for it would understandably fail!

DeeDeeG force-pushed the pylauncher-python3 branch from ae8e2b3 to ee0b793 Compare May 16, 2020 22:06

rvagg closed this May 18, 2020

rvagg mentioned this pull request May 26, 2020

v7.0.0 Proposal #2124

Closed

DeeDeeG mentioned this pull request Aug 18, 2020

[QUESTION] node-gyp@7 for npm@7 please npm/cli#1281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DeeDeeG commented May 15, 2020 •

edited

Loading

DeeDeeG commented May 15, 2020

cclauss left a comment

DeeDeeG commented May 15, 2020

DeeDeeG May 15, 2020

cclauss May 15, 2020

DeeDeeG May 15, 2020

DeeDeeG May 15, 2020

rvagg May 16, 2020

rvagg commented May 16, 2020

DeeDeeG commented May 16, 2020

DeeDeeG commented May 16, 2020 •

edited

Loading

DeeDeeG commented May 16, 2020

cclauss commented May 17, 2020 •

edited

Loading

rvagg commented May 17, 2020

cclauss commented May 17, 2020

DeeDeeG commented May 18, 2020

rvagg commented May 18, 2020

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

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

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

Conversation

DeeDeeG commented May 15, 2020 • edited Loading

Checklist

Description of change

DeeDeeG commented May 15, 2020

cclauss left a comment

Choose a reason for hiding this comment

DeeDeeG commented May 15, 2020

DeeDeeG May 15, 2020

Choose a reason for hiding this comment

cclauss May 15, 2020

Choose a reason for hiding this comment

DeeDeeG May 15, 2020

Choose a reason for hiding this comment

DeeDeeG May 15, 2020

Choose a reason for hiding this comment

rvagg May 16, 2020

Choose a reason for hiding this comment

rvagg commented May 16, 2020

DeeDeeG commented May 16, 2020

DeeDeeG commented May 16, 2020 • edited Loading

DeeDeeG commented May 16, 2020

cclauss commented May 17, 2020 • edited Loading

rvagg commented May 17, 2020

cclauss commented May 17, 2020

DeeDeeG commented May 18, 2020

rvagg commented May 18, 2020

DeeDeeG commented May 15, 2020 •

edited

Loading

DeeDeeG commented May 16, 2020 •

edited

Loading

cclauss commented May 17, 2020 •

edited

Loading