-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: drop Python 2 support in find-python.js #2333
Changes from 11 commits
a9e3219
83bf041
39403a8
9c0d980
3c48f70
f13747d
d1f2aec
2562153
d20f929
54041d8
d0bcb5c
a7b43aa
432447d
1711046
ec64f91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,26 @@ | ||
'use strict' | ||
|
||
const path = require('path') | ||
const log = require('npmlog') | ||
const semver = require('semver') | ||
const cp = require('child_process') | ||
const extend = require('util')._extend // eslint-disable-line | ||
const win = process.platform === 'win32' | ||
const logWithPrefix = require('./util').logWithPrefix | ||
|
||
const systemDrive = process.env.SystemDrive || 'C:' | ||
const username = process.env.USERNAME || process.env.USER | ||
const winDefaultLocationsArray = [] | ||
|
||
for (const majorMinor of ['39', '38', '37', '36']) { | ||
winDefaultLocationsArray.push( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of fixed Because these folders can be set to be on other disks than the system drive, and avoids potential localization issues in the path. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mfonville Thanks for the suggestion. I tried your suggestion on another branch: DeeDeeG@e57fdcc It does work. It's also a bit more involved. After some quick research and testing, I am somewhat doubtful whether there are circumstances where the paths actually change based on localization or user customization. See my thoughts below. If you have examples or more information about when/if the paths are actually different, please let me know. At the moment I am inclined to believe this is not necessary. But I would not mind being proved wrong. Thanks. More thoughts on this (click to expand):I wish I knew in what circumstances the "Program Files" or "AppData" folders are localized or moved, if there are any such circumstances. I do not think the paths are truly localized on-disk, nor movable, in Windows 10. It's unclear to me if they've ever been truly localized on-disk, or movable in previous Windows releases. (Paths were slightly different in Windows XP, but Windows XP is not compatible with the latest Regarding custom locations for Program Files or AppData: Is anyone here aware of a proper way to move these folders? (I am aware that folders such as Documents and Videos can be easily moved to another drive. But can "Program Files", user home folders, or "AppData" be moved off of the system drive? I have looked this up online, and the overall impression I am getting is that there is no official way to move these, and that moving them may break things.) Regarding localization, or Windows in languages other than English: I don't see the paths being actually localized on the disk. (On Windows 7 or 10 in Spanish, I see it presented visually in the File Explorer as " There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If XP is out of the picture, then I do think localization is probably indeed not an issue anymore. Windows 10 does this nicely. But about custom paths, to be honest I am not working with Windows daily anymore (switched to Linux many years ago). But when a couple of years ago I did do some volunteering with managing some Windows systems, we could set custom locations for these paths through policies. And also I have seen customized paths when computers were upgraded from a HDD to a SSD, where these paths were customized to optimize performance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for that explanation. I also found this official Microsoft document about moved user folders: https://docs.microsoft.com/en-us/troubleshoot/windows-server/user-profiles-and-logon/relocation-of-users-and-programdata-directories So apparently user directories can be moved to a custom location. (Maybe this applies only to Windows Server? I'd say I'm a little more inclined to do this (DeeDeeG@e57fdcc) now. Truth be told, it's not much more complex. Given even a mild justification for end-user benefit, I'd want to do it. NOTE TO MAINTAINERS/REVIEWERS: Please let me know if you'd like me to add this (DeeDeeG@e57fdcc) to the PR. Thank you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And most of all, there is no downside to the change I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Go for it. This code is easy to read/ understand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know that is the Swiss in me but... Please sort your const declarations in the order that they are used to improve readability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More research: We definitely don't have to worry about Windows XP or Vista. Node v6 refuses to run on anything older than Windows 7. Node v5 is the newest Node that will run on Windows Vista. Node v5 cannot even run the latest There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@mfonville thanks for mentioning that. I believe I will need to switch from (Note: this is important when running 32-bit Node on 64-bit Windows, which is common when building 32-bit code on a 64-bit host, such as in CI.) It appears we have to use (Official documentation says that the env var See: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I committed this in the PR now. |
||
`${systemDrive}\\Users\\${username}\\AppData\\Local\\Programs\\Python\\Python${majorMinor}\\python.exe`, | ||
`${systemDrive}\\Program Files\\Python${majorMinor}\\python.exe`, | ||
`${systemDrive}\\Users\\${username}\\AppData\\Local\\Programs\\Python\\Python${majorMinor}-32\\python.exe`, | ||
`${systemDrive}\\Program Files\\Python${majorMinor}-32\\python.exe`, | ||
`${systemDrive}\\Program Files (x86)\\Python${majorMinor}-32\\python.exe` | ||
) | ||
} | ||
|
||
function PythonFinder (configPython, callback) { | ||
this.callback = callback | ||
this.configPython = configPython | ||
|
@@ -18,17 +31,14 @@ PythonFinder.prototype = { | |
log: logWithPrefix(log, 'find Python'), | ||
argsExecutable: ['-c', 'import sys; print(sys.executable);'], | ||
argsVersion: ['-c', 'import sys; print("%s.%s.%s" % sys.version_info[:3]);'], | ||
semverRange: '2.7.x || >=3.5.0', | ||
semverRange: '>=3.6.0', | ||
|
||
// These can be overridden for testing: | ||
execFile: cp.execFile, | ||
env: process.env, | ||
win: win, | ||
pyLauncher: 'py.exe', | ||
winDefaultLocations: [ | ||
path.join(process.env.SystemDrive || 'C:', 'Python37', 'python.exe'), | ||
path.join(process.env.SystemDrive || 'C:', 'Python27', 'python.exe') | ||
], | ||
winDefaultLocations: winDefaultLocationsArray, | ||
|
||
// Logs a message at verbose level, but also saves it to be displayed later | ||
// at error level if an error occurs. This should help diagnose the problem. | ||
|
@@ -96,11 +106,6 @@ PythonFinder.prototype = { | |
before: () => { this.addLog('checking if "python" can be used') }, | ||
check: this.checkCommand, | ||
arg: 'python' | ||
}, | ||
{ | ||
before: () => { this.addLog('checking if "python2" can be used') }, | ||
check: this.checkCommand, | ||
arg: 'python2' | ||
} | ||
] | ||
|
||
|
@@ -119,7 +124,7 @@ PythonFinder.prototype = { | |
checks.push({ | ||
before: () => { | ||
this.addLog( | ||
'checking if the py launcher can be used to find Python') | ||
'checking if the py launcher can be used to find Python 3') | ||
}, | ||
check: this.checkPyLauncher | ||
}) | ||
|
@@ -188,10 +193,15 @@ PythonFinder.prototype = { | |
// 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 Python 2 and Python 3, we should | ||
// explicitly request a Python 3 version. This is done by supplying "-3" as | ||
// the first command line argument. Since "py.exe -3" 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 executable path`) | ||
this.run(this.pyLauncher, this.argsExecutable, false, | ||
`- executing "${this.pyLauncher}" to get Python 3 executable path`) | ||
this.run(this.pyLauncher, ['-3', ...this.argsExecutable], false, | ||
function (err, execPath) { | ||
// Possible outcomes: same as checkCommand | ||
if (err) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should fall back to assigning some string to
username
. Even something not very useful like''
or'User'
.(It's possible both
process.env.USERNAME
andprocess.env.USER
areundefined
. Having a not very useful path to check is better than seeingUncaught ReferenceError: username is not defined
.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like the error message better than unsuccessfully trying to sweep the problem under the carpet. The message makes it clear what the problem is.
Please do not worry about the Python 4 case. We will cross that bridge later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking a bit more...
If username is undefined and we can find a globally installed acceptable Python then we should use that Python without complaint. If we fail to find an acceptable Python and username is undefined then we should raise the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also get the username via
os.userInfo()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that suggestion @cjihrig.
os.userInfo()
should always return a username string. I committed that as a fallback. I think this can reasonably be expected to preventUncaught ReferenceError: username is not defined
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: There was apparently no risk of
Uncaught ReferenceError
after all.If you write
.someNotDefinedThing
off of an object or array, you getundefined
as the result, but noReferenceError
. Sinceprocess.env
returns an object, doingprocess.env.THIS_IS_NOT_DEFINED
does not produce aReferenceError
.For example:
Outputs the string:
So we would construct a Python path resembling this:
C:\\Users\\undefined\\AppData\\Local\\Programs\\Python\\Python39\\python.exe
(Note the
Users\\undefined
bit.)I'm happy with the
os.userInfo()
fallback, regardless. It makes it pretty much guaranteed we can find the username. This is just an interesting finding from my tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to this, still an issue when the username is not defined, only the user ID e.g. that is common in 'non-root' containers for OpenShift https://docs.openshift.com/container-platform/3.3/creating_images/guidelines.html#openshift-container-platform-specific-guidelines.
Python is in the path so a good option could be to add a fallback or not return an error for the line