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: drop Python 2 support in find-python.js #2333

Merged
merged 15 commits into from
Mar 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 27 additions & 14 deletions lib/find-python.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,29 @@
'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 || require('os').userInfo().username
const localAppData = process.env.LOCALAPPDATA || `${systemDrive}\\${username}\\AppData\\Local`
const programFiles = process.env.ProgramW6432 || process.env.ProgramFiles || `${systemDrive}\\Program Files`
const programFilesX86 = process.env['ProgramFiles(x86)'] || `${programFiles} (x86)`

const winDefaultLocationsArray = []
for (const majorMinor of ['39', '38', '37', '36']) {
winDefaultLocationsArray.push(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of fixed Users and Program Files shouldn't this be with the environment variables %ProgramFiles%, %ProgramFiles(x86)%, %ProgramW6432% and %LocalAppData%?

Because these folders can be set to be on other disks than the system drive, and avoids potential localization issues in the path.

Copy link
Contributor Author

@DeeDeeG DeeDeeG Feb 20, 2021

Choose a reason for hiding this comment

The 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 node-gyp. Python 3.4 is the last version supporting Windows XP. node-gyp master branch now requires Python 3.6 and up, so no XP support anymore. And only Windows 10 is in "mainstream" support by Microsoft now.)

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 "C: > Usuarios > [user]", but the moment you try to edit the path in the address bar, it changes to C:\Users\[user]. The env var LOCALAPPDATA is still C:\Users\[user]\AppData\Local. The env var ProgramFiles is still C:\Program Files. Even while this shows as C: > Archivos de programa in File Explorer, the moment you try to edit it, it changes to C:\Program Files. I believe the "non-localized" versions are the true paths, and the localizations are superficial. At least this is the case in my testing on the latest Windows 10, and on an install made from an old Windows 7 iso.)

Choose a reason for hiding this comment

The 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.
I am not sure how Windows 10 handles it these days, but imho those environment variables are more reliable than any hardcoded path. So better to use the variables :-)

Copy link
Contributor Author

@DeeDeeG DeeDeeG Feb 20, 2021

Choose a reason for hiding this comment

The 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 node-gyp is not any less important for Windows Server users. It still seems like a bit of an obscure feature and is discouraged in the docs I linked.)

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.

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go for it. This code is easy to read/ understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@DeeDeeG DeeDeeG Feb 22, 2021

Choose a reason for hiding this comment

The 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 node-gyp, as node-gyp user newer JavaScript features that aren't supported in Node 5. In conclusion: modern node-gyp won't run at all on Windows XP or Vista, regardless of this PR.

Copy link
Contributor Author

@DeeDeeG DeeDeeG Feb 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

environment variables [. . .] %ProgramW6432% [. . .]

@mfonville thanks for mentioning that. I believe I will need to switch from process.env.ProgramFiles to process.env.ProgramW6432 to get the "not (x86)" Program Files.

(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 process.env.ProgramW6432 to get the not-architecture-specific Program Files location. From 32-bit Node on a 64-bit host, process.env.ProgramFiles returns [driveLetter]:\Program Files (x86). As such, ProgramW6432 is the only easy way to get [driveLetter]:\Program Files from the environment across all combinations of host arch/node arch.

(Official documentation says that the env var ProgramW6432 only exists as of Windows 7 or newer, but that's okay. node-gyp doesn't even run on Windows Vista or older.)

See:

Copy link
Contributor Author

@DeeDeeG DeeDeeG Feb 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I committed this in the PR now. Program Files, Program Files (x86) and AppData\Local are set from environment variables now.

`${localAppData}\\Programs\\Python\\Python${majorMinor}\\python.exe`,
`${programFiles}\\Python${majorMinor}\\python.exe`,
`${localAppData}\\Programs\\Python\\Python${majorMinor}-32\\python.exe`,
`${programFiles}\\Python${majorMinor}-32\\python.exe`,
`${programFilesX86}\\Python${majorMinor}-32\\python.exe`
)
}

function PythonFinder (configPython, callback) {
this.callback = callback
this.configPython = configPython
Expand All @@ -18,17 +34,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.
Expand Down Expand Up @@ -96,11 +109,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'
}
]

Expand All @@ -119,7 +127,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
})
Expand Down Expand Up @@ -188,10 +196,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) {
Expand Down
22 changes: 9 additions & 13 deletions test/test-find-python.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,8 @@ test('find python', function (t) {
t.strictEqual(err, null)
var proc = execFile(found, ['-V'], function (err, stdout, stderr) {
t.strictEqual(err, null)
if (/Python 2/.test(stderr)) {
t.strictEqual(stdout, '')
t.ok(/Python 2/.test(stderr))
} else {
t.ok(/Python 3/.test(stdout))
t.strictEqual(stderr, '')
}
t.ok(/Python 3/.test(stdout))
t.strictEqual(stderr, '')
})
proc.stdout.setEncoding('utf-8')
proc.stderr.setEncoding('utf-8')
Expand Down Expand Up @@ -66,7 +61,7 @@ test('find python - python', function (t) {
poison(f, 'execFile')
t.strictEqual(program, '/path/python')
t.ok(/sys\.version_info/.test(args[1]))
cb(null, '2.7.15')
cb(null, '3.9.1')
}
t.strictEqual(program,
process.platform === 'win32' ? '"python"' : 'python')
Expand Down Expand Up @@ -146,13 +141,14 @@ test('find python - no python2, no python, unix', function (t) {
})

test('find python - no python, use python launcher', function (t) {
t.plan(3)
t.plan(4)

var f = new TestPythonFinder(null, done)
f.win = true

f.execFile = function (program, args, opts, cb) {
if (program === 'py.exe') {
t.notEqual(args.indexOf('-3'), -1)
t.notEqual(args.indexOf('-c'), -1)
return cb(null, 'Z:\\snake.exe')
}
Expand All @@ -162,7 +158,7 @@ test('find python - no python, use python launcher', function (t) {
cb(new Error('not found'))
} else if (/sys\.version_info/.test(args[args.length - 1])) {
if (program === 'Z:\\snake.exe') {
cb(null, '2.7.14')
cb(null, '3.9.0')
} else {
t.fail()
}
Expand All @@ -181,17 +177,17 @@ test('find python - no python, use python launcher', function (t) {
test('find python - no python, no python launcher, good guess', function (t) {
t.plan(2)

var re = /C:[\\/]Python37[\\/]python[.]exe/
var f = new TestPythonFinder(null, done)
f.win = true
const expectedProgram = f.winDefaultLocations[0]

f.execFile = function (program, args, opts, cb) {
if (program === 'py.exe') {
return cb(new Error('not found'))
}
if (/sys\.executable/.test(args[args.length - 1])) {
cb(new Error('not found'))
} else if (re.test(program) &&
} else if (program === expectedProgram &&
/sys\.version_info/.test(args[args.length - 1])) {
cb(null, '3.7.3')
} else {
Expand All @@ -202,7 +198,7 @@ test('find python - no python, no python launcher, good guess', function (t) {

function done (err, python) {
t.strictEqual(err, null)
t.ok(re.test(python))
t.ok(python === expectedProgram)
}
})

Expand Down