Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: try to find python after python3 #1907

Closed
wants to merge 3 commits into from
Closed

lib: try to find python after python3 #1907

wants to merge 3 commits into from

Conversation

sam-github
Copy link
Contributor

Unadorned python can be either Python 2 or Python 3, but it is likely
to be Python 2 for quite a while.

To find Python3, it is recommended to use the explicit name python3.

See:

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

Unadorned `python` can be either Python 2 or Python 3, but it is likely
to be Python 2 for quite a while.

To find Python3, it is recommended to use the explicit name `python3`.

See:
- https://www.python.org/dev/peps/pep-0394/#for-python-runtime-distributors
- #1892 (comment)
@sam-github
Copy link
Contributor Author

I'm not sure what to do about

node-gyp/lib/find-python.js

Lines 186 to 210 in f36bd22

// Check if the py launcher can find a valid Python to use.
// Will exit the Python finder on success.
// Distributions of Python on Windows by default install with the "py.exe"
// Python launcher which is more likely to exist than the Python executable
// being in the $PATH.
// Because the Python launcher supports all versions of Python, we have to
// explicitly request a Python 2 version. This is done by supplying "-2" as
// the first command line argument. Since "py.exe -2" would be an invalid
// executable for "execFile", we have to use the launcher to figure out
// where the actual "python.exe" executable is located.
checkPyLauncher: function checkPyLauncher (errorCallback) {
this.log.verbose(
`- executing "${this.pyLauncher}" to get Python 2 executable path`)
this.run(this.pyLauncher, ['-2', ...this.argsExecutable], false,
function (err, execPath) {
// Possible outcomes: same as checkCommand
if (err) {
this.addLog(
`- "${this.pyLauncher}" is not in PATH or produced an error`)
return errorCallback(err)
}
this.addLog(`- executable path is "${execPath}"`)
this.checkExecPath(execPath, errorCallback)
}.bind(this))
},

I don't have any Windows systems available ATM, so if I duplicate/refactor the Py Launcher stuff to look for 3 first, then 2, I've no way to test, unless its tested in CI, but I'm thinking it isn't.

I left it alone for now, though I did switch the order of the Windows default locations:

path.join(process.env.SystemDrive || 'C:', 'Python27', 'python.exe'),
path.join(process.env.SystemDrive || 'C:', 'Python37', 'python.exe')

@cclauss
Copy link
Contributor

cclauss commented Oct 3, 2019

I agree with this order for v6.0.0:

  1. python3
  2. python
  3. python2

However, the only acceptable Pythons are 2.7, 3.5, 3.6, and 3.7. So if python3 is Python 3.4 then we do not use it and we continue down the list.

@sam-github
Copy link
Contributor Author

I note with relief that there are some Windows tests in Travis.

@joaocgreis
Copy link
Member

@sam-github The change of order of default locations LGTM. You can simply move the PyLauncher to after the default location check, swap the blocks starting on lines 108 and 115. I plan to review the PyLauncher code for Python 3 sometime, and add registry detection like we have in core.

@sam-github
Copy link
Contributor Author

@joaocgreis reordered as you suggested, which required some of the test monkey-patching to be adjusted for the new order of execFile()s.

rvagg pushed a commit that referenced this pull request Oct 4, 2019
Unadorned `python` can be either Python 2 or Python 3, but it is likely
to be Python 2 for quite a while.

To find Python3, it is recommended to use the explicit name `python3`.

See:
- https://www.python.org/dev/peps/pep-0394/#for-python-runtime-distributors
- #1892 (comment)

PR-URL: #1907
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
@rvagg
Copy link
Member

rvagg commented Oct 4, 2019

landed in dd0e97e, let's pull the trigger then eh?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants