From e3778d99072b291fddbcd6f9d35c24eb0d6bd03b Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 13 Jul 2016 09:26:58 +0200 Subject: [PATCH 1/2] Add lots of findPython() tests. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Break up findPython() into pieces that can be tested in isolation and add tests that do so. PR-URL: https://github.com/nodejs/node-gyp/pull/992 Reviewed-By: Johan Bergström --- lib/configure.js | 187 +++++++++++++++++------------- test/test-find-python.js | 238 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 345 insertions(+), 80 deletions(-) diff --git a/lib/configure.js b/lib/configure.js index b275c23753..3034652934 100644 --- a/lib/configure.js +++ b/lib/configure.js @@ -1,6 +1,9 @@ module.exports = exports = configure -module.exports.test = { findAccessibleSync: findAccessibleSync, - findPython: findPython } +module.exports.test = { + PythonFinder: PythonFinder, + findAccessibleSync: findAccessibleSync, + findPython: findPython, +} /** * Module dependencies. @@ -16,8 +19,6 @@ var fs = require('graceful-fs') , cp = require('child_process') , extend = require('util')._extend , processRelease = require('./process-release') - , spawn = cp.spawn - , execFile = cp.execFile , win = process.platform == 'win32' , findNodeDirectory = require('./find-node-directory') , msgFormat = require('util').format @@ -336,34 +337,45 @@ function findAccessibleSync (logprefix, dir, candidates) { return undefined } -function findPython (python, callback) { - checkPython() +function PythonFinder(python, callback) { + this.callback = callback + this.python = python +} - // Check if Python is in the $PATH - function checkPython () { - log.verbose('check python', 'checking for Python executable "%s" in the PATH', python) - which(python, function (err, execPath) { +PythonFinder.prototype = { + env: process.env, + execFile: cp.execFile, + log: log, + stat: fs.stat, + which: which, + win: win, + + checkPython: function checkPython () { + this.log.verbose('check python', + 'checking for Python executable "%s" in the PATH', + this.python) + this.which(this.python, function (err, execPath) { if (err) { - log.verbose('`which` failed', python, err) - if (python === 'python2') { - python = 'python' - return checkPython() + this.log.verbose('`which` failed', this.python, err) + if (this.python === 'python2') { + this.python = 'python' + return this.checkPython() } - if (win) { - checkPythonLauncher() + if (this.win) { + this.checkPythonLauncher() } else { - failNoPython() + this.failNoPython() } } else { - log.verbose('`which` succeeded', python, execPath) - // Found the `python` exceutable, and from now on we use it explicitly. + this.log.verbose('`which` succeeded', this.python, execPath) + // Found the `python` executable, and from now on we use it explicitly. // This solves #667 and #750 (`execFile` won't run batch files // (*.cmd, and *.bat)) - python = execPath - checkPythonVersion() + this.python = execPath + this.checkPythonVersion() } - }) - } + }.bind(this)) + }, // Distributions of Python on Windows by default install with the "py.exe" // Python launcher which is more likely to exist than the Python executable @@ -373,63 +385,45 @@ function findPython (python, callback) { // 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. - function checkPythonLauncher () { - log.verbose('could not find "' + python + '". checking python launcher') - var env = extend({}, process.env) + checkPythonLauncher: function checkPythonLauncher () { + this.log.verbose( + 'could not find "' + this.python + '". checking python launcher') + var env = extend({}, this.env) env.TERM = 'dumb' var launcherArgs = ['-2', '-c', 'import sys; print sys.executable'] - execFile('py.exe', launcherArgs, { env: env }, function (err, stdout) { + this.execFile('py.exe', launcherArgs, { env: env }, function (err, stdout) { if (err) { - guessPython() - return - } - python = stdout.trim() - log.verbose('check python launcher', 'python executable found: %j', python) - checkPythonVersion() - }) - } - - // Called on Windows when "python" isn't available in the current $PATH. - // We're gonna check if "%SystemDrive%\python27\python.exe" exists. - function guessPython () { - log.verbose('could not find "' + python + '". guessing location') - var rootDir = process.env.SystemDrive || 'C:\\' - if (rootDir[rootDir.length - 1] !== '\\') { - rootDir += '\\' - } - var pythonPath = path.resolve(rootDir, 'Python27', 'python.exe') - log.verbose('ensuring that file exists:', pythonPath) - fs.stat(pythonPath, function (err, stat) { - if (err) { - if (err.code == 'ENOENT') { - failNoPython() - } else { - callback(err) - } + this.guessPython() return } - python = pythonPath - checkPythonVersion() - }) - } - - function checkPythonVersion () { - var env = extend({}, process.env) + this.python = stdout.trim() + this.log.verbose('check python launcher', + 'python executable found: %j', + this.python) + this.checkPythonVersion() + }.bind(this)) + }, + + checkPythonVersion: function checkPythonVersion () { + var args = ['-c', 'import platform; print(platform.python_version());'] + var env = extend({}, this.env) env.TERM = 'dumb' - execFile(python, ['-c', 'import platform; print(platform.python_version());'], { env: env }, function (err, stdout) { + this.execFile(this.python, args, { env: env }, function (err, stdout) { if (err) { - return callback(err) + return this.callback(err) } - log.verbose('check python version', '`%s -c "import platform; print(platform.python_version());"` returned: %j', python, stdout) + this.log.verbose('check python version', + '`%s -c "' + args[1] + '"` returned: %j', + this.python, stdout) var version = stdout.trim() if (~version.indexOf('+')) { - log.silly('stripping "+" sign(s) from version') + this.log.silly('stripping "+" sign(s) from version') version = version.replace(/\+/g, '') } if (~version.indexOf('rc')) { - log.silly('stripping "rc" identifier from version') + this.log.silly('stripping "rc" identifier from version') version = version.replace(/rc(.*)$/ig, '') } var range = semver.Range('>=2.5.0 <3.0.0') @@ -437,24 +431,59 @@ function findPython (python, callback) { try { valid = range.test(version) } catch (e) { - log.silly('range.test() error', e) + this.log.silly('range.test() error', e) } if (valid) { - callback(null, python) + this.callback(null, this.python) } else { - failPythonVersion(version) + this.failPythonVersion(version) } - }) - } + }.bind(this)) + }, + + failNoPython: function failNoPython () { + var errmsg = + 'Can\'t find Python executable "' + this.python + + '", you can set the PYTHON env variable.' + this.callback(new Error(errmsg)) + }, + + failPythonVersion: function failPythonVersion (badVersion) { + var errmsg = + 'Python executable "' + this.python + + '" is v' + badVersion + ', which is not supported by gyp.\n' + + 'You can pass the --python switch to point to ' + + 'Python >= v2.5.0 & < 3.0.0.' + this.callback(new Error(errmsg)) + }, - function failNoPython () { - callback(new Error('Can\'t find Python executable "' + python + - '", you can set the PYTHON env variable.')) - } + // Called on Windows when "python" isn't available in the current $PATH. + // We are going to check if "%SystemDrive%\python27\python.exe" exists. + guessPython: function guessPython () { + this.log.verbose('could not find "' + this.python + '". guessing location') + var rootDir = this.env.SystemDrive || 'C:\\' + if (rootDir[rootDir.length - 1] !== '\\') { + rootDir += '\\' + } + var resolve = path.win32 && path.win32.resolve || path.resolve + var pythonPath = resolve(rootDir, 'Python27', 'python.exe') + this.log.verbose('ensuring that file exists:', pythonPath) + this.stat(pythonPath, function (err, stat) { + if (err) { + if (err.code == 'ENOENT') { + this.failNoPython() + } else { + this.callback(err) + } + return + } + this.python = pythonPath + this.checkPythonVersion() + }.bind(this)) + }, +} - function failPythonVersion (badVersion) { - callback(new Error('Python executable "' + python + - '" is v' + badVersion + ', which is not supported by gyp.\n' + - 'You can pass the --python switch to point to Python >= v2.5.0 & < 3.0.0.')) - } +function findPython (python, callback) { + var finder = new PythonFinder(python, callback) + finder.checkPython() } diff --git a/test/test-find-python.js b/test/test-find-python.js index 7f5c394679..c9089be5e4 100644 --- a/test/test-find-python.js +++ b/test/test-find-python.js @@ -3,8 +3,9 @@ var test = require('tape') var configure = require('../lib/configure') var execFile = require('child_process').execFile +var PythonFinder = configure.test.PythonFinder -test('find python executable', function (t) { +test('find python', function (t) { t.plan(4) configure.test.findPython('python', function (err, found) { @@ -18,3 +19,238 @@ test('find python executable', function (t) { proc.stderr.setEncoding('utf-8') }) }) + +function poison(object, property) { + function fail() { + throw new Error('Property ' + property + ' should not have been accessed.') + } + var descriptor = { + configurable: true, + enumerable: false, + writable: true, + getter: fail, + setter: fail, + } + Object.defineProperty(object, property, descriptor) +} + +function TestPythonFinder() { PythonFinder.apply(this, arguments) } +TestPythonFinder.prototype = Object.create(PythonFinder.prototype) +poison(TestPythonFinder.prototype, 'env') +poison(TestPythonFinder.prototype, 'execFile') +poison(TestPythonFinder.prototype, 'stat') +poison(TestPythonFinder.prototype, 'which') +poison(TestPythonFinder.prototype, 'win') + +test('find python - python', function (t) { + t.plan(5) + + var f = new TestPythonFinder('python', done) + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(null, program) + } + f.execFile = function(program, args, opts, cb) { + t.strictEqual(program, 'python') + t.ok(/import platform/.test(args[1])) + cb(null, '2.7.0') + } + f.checkPython() + + function done(err, python) { + t.strictEqual(err, null) + t.strictEqual(python, 'python') + } +}) + +test('find python - python too old', function (t) { + t.plan(4) + + var f = new TestPythonFinder('python', done) + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(null, program) + } + f.execFile = function(program, args, opts, cb) { + t.strictEqual(program, 'python') + t.ok(/import platform/.test(args[1])) + cb(null, '2.3.4') + } + f.checkPython() + + function done(err, python) { + t.ok(/is not supported by gyp/.test(err)) + } +}) + +test('find python - python too new', function (t) { + t.plan(4) + + var f = new TestPythonFinder('python', done) + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(null, program) + } + f.execFile = function(program, args, opts, cb) { + t.strictEqual(program, 'python') + t.ok(/import platform/.test(args[1])) + cb(null, '3.0.0') + } + f.checkPython() + + function done(err, python) { + t.ok(/is not supported by gyp/.test(err)) + } +}) + +test('find python - no python', function (t) { + t.plan(2) + + var f = new TestPythonFinder('python', done) + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(new Error('not found')) + } + f.checkPython() + + function done(err, python) { + t.ok(/Can't find Python executable/.test(err)) + } +}) + +test('find python - no python2', function (t) { + t.plan(6) + + var f = new TestPythonFinder('python2', done) + f.which = function(program, cb) { + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(null, program) + } + t.strictEqual(program, 'python2') + cb(new Error('not found')) + } + f.execFile = function(program, args, opts, cb) { + t.strictEqual(program, 'python') + t.ok(/import platform/.test(args[1])) + cb(null, '2.7.0') + } + f.checkPython() + + function done(err, python) { + t.strictEqual(err, null) + t.strictEqual(python, 'python') + } +}) + +test('find python - no python2, no python, unix', function (t) { + t.plan(3) + + var f = new TestPythonFinder('python2', done) + poison(f, 'checkPythonLauncher') + f.win = false + + f.which = function(program, cb) { + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(new Error('not found')) + } + t.strictEqual(program, 'python2') + cb(new Error('not found')) + } + f.checkPython() + + function done(err, python) { + t.ok(/Can't find Python executable/.test(err)) + } +}) + +test('find python - no python, use python launcher', function (t) { + t.plan(8) + + var f = new TestPythonFinder('python', done) + f.env = {} + f.win = true + + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(new Error('not found')) + } + f.execFile = function(program, args, opts, cb) { + f.execFile = function(program, args, opts, cb) { + t.strictEqual(program, 'Z:\\snake.exe') + t.ok(/import platform/.test(args[1])) + cb(null, '2.7.0') + } + t.strictEqual(program, 'py.exe') + t.notEqual(args.indexOf('-2'), -1) + t.notEqual(args.indexOf('-c'), -1) + cb(null, 'Z:\\snake.exe') + } + f.checkPython() + + function done(err, python) { + t.strictEqual(err, null) + t.strictEqual(python, 'Z:\\snake.exe') + } +}) + +test('find python - no python, no python launcher, good guess', function (t) { + t.plan(6) + + var re = /C:[\\\/]Python27[\\\/]python[.]exe/ + var f = new TestPythonFinder('python', done) + f.env = {} + f.win = true + + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(new Error('not found')) + } + f.execFile = function(program, args, opts, cb) { + f.execFile = function(program, args, opts, cb) { + t.ok(re.test(program)) + t.ok(/import platform/.test(args[1])) + cb(null, '2.7.0') + } + t.strictEqual(program, 'py.exe') + cb(new Error('not found')) + } + f.stat = function(path, cb) { + t.ok(re.test(path)) + cb(null, {}) + } + f.checkPython() + + function done(err, python) { + t.ok(re.test(python)) + } +}) + +test('find python - no python, no python launcher, bad guess', function (t) { + t.plan(4) + + var f = new TestPythonFinder('python', done) + f.env = { SystemDrive: 'Z:\\' } + f.win = true + + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(new Error('not found')) + } + f.execFile = function(program, args, opts, cb) { + t.strictEqual(program, 'py.exe') + cb(new Error('not found')) + } + f.stat = function(path, cb) { + t.ok(/Z:[\\\/]Python27[\\\/]python.exe/.test(path)) + var err = new Error('not found') + err.code = 'ENOENT' + cb(err) + } + f.checkPython() + + function done(err, python) { + t.ok(/Can't find Python executable/.test(err)) + } +}) From 37ae7be114d661bae9a753c06a2dfadc8fba5984 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 13 Jul 2016 10:00:14 +0200 Subject: [PATCH 2/2] Try python launcher when stock python is python 3. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consult the python launcher when the python on the path is python 3. If a python 2 exists on the system, it will tell us. Fixes: https://github.com/nodejs/node-gyp/issues/987 PR-URL: https://github.com/nodejs/node-gyp/pull/992 Reviewed-By: Johan Bergström --- lib/configure.js | 18 ++++++---- test/test-find-python.js | 71 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 6 deletions(-) diff --git a/lib/configure.js b/lib/configure.js index 3034652934..54bb2ccb3a 100644 --- a/lib/configure.js +++ b/lib/configure.js @@ -343,6 +343,7 @@ function PythonFinder(python, callback) { } PythonFinder.prototype = { + checkPythonLauncherDepth: 0, env: process.env, execFile: cp.execFile, log: log, @@ -386,6 +387,8 @@ PythonFinder.prototype = { // executable for "execFile", we have to use the launcher to figure out // where the actual "python.exe" executable is located. checkPythonLauncher: function checkPythonLauncher () { + this.checkPythonLauncherDepth += 1 + this.log.verbose( 'could not find "' + this.python + '". checking python launcher') var env = extend({}, this.env) @@ -395,13 +398,14 @@ PythonFinder.prototype = { this.execFile('py.exe', launcherArgs, { env: env }, function (err, stdout) { if (err) { this.guessPython() - return + } else { + this.python = stdout.trim() + this.log.verbose('check python launcher', + 'python executable found: %j', + this.python) + this.checkPythonVersion() } - this.python = stdout.trim() - this.log.verbose('check python launcher', - 'python executable found: %j', - this.python) - this.checkPythonVersion() + this.checkPythonLauncherDepth -= 1 }.bind(this)) }, @@ -435,6 +439,8 @@ PythonFinder.prototype = { } if (valid) { this.callback(null, this.python) + } else if (this.win && this.checkPythonLauncherDepth === 0) { + this.checkPythonLauncher() } else { this.failPythonVersion(version) } diff --git a/test/test-find-python.js b/test/test-find-python.js index c9089be5e4..30ba6df78b 100644 --- a/test/test-find-python.js +++ b/test/test-find-python.js @@ -195,6 +195,77 @@ test('find python - no python, use python launcher', function (t) { } }) +test('find python - python 3, use python launcher', function (t) { + t.plan(10) + + var f = new TestPythonFinder('python', done) + f.env = {} + f.win = true + + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(null, program) + } + f.execFile = function(program, args, opts, cb) { + f.execFile = function(program, args, opts, cb) { + f.execFile = function(program, args, opts, cb) { + t.strictEqual(program, 'Z:\\snake.exe') + t.ok(/import platform/.test(args[1])) + cb(null, '2.7.0') + } + t.strictEqual(program, 'py.exe') + t.notEqual(args.indexOf('-2'), -1) + t.notEqual(args.indexOf('-c'), -1) + cb(null, 'Z:\\snake.exe') + } + t.strictEqual(program, 'python') + t.ok(/import platform/.test(args[1])) + cb(null, '3.0.0') + } + f.checkPython() + + function done(err, python) { + t.strictEqual(err, null) + t.strictEqual(python, 'Z:\\snake.exe') + } +}) + +test('find python - python 3, use python launcher, python 2 too old', + function (t) { + t.plan(9) + + var f = new TestPythonFinder('python', done) + f.checkedPythonLauncher = false + f.env = {} + f.win = true + + f.which = function(program, cb) { + t.strictEqual(program, 'python') + cb(null, program) + } + f.execFile = function(program, args, opts, cb) { + f.execFile = function(program, args, opts, cb) { + f.execFile = function(program, args, opts, cb) { + t.strictEqual(program, 'Z:\\snake.exe') + t.ok(/import platform/.test(args[1])) + cb(null, '2.3.4') + } + t.strictEqual(program, 'py.exe') + t.notEqual(args.indexOf('-2'), -1) + t.notEqual(args.indexOf('-c'), -1) + cb(null, 'Z:\\snake.exe') + } + t.strictEqual(program, 'python') + t.ok(/import platform/.test(args[1])) + cb(null, '3.0.0') + } + f.checkPython() + + function done(err, python) { + t.ok(/is not supported by gyp/.test(err)) + } +}) + test('find python - no python, no python launcher, good guess', function (t) { t.plan(6)