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.

Sign up for GitHub

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

python: more informative error #1269

Closed
wants to merge 1 commit into from
+48 −28
Closed

python: more informative error #1269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion .jshintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"asi": true,
"laxcomma": true,
"es5": true,
"esversion": 6,
"node": true,
"strict": false
}
8 changes: 6 additions & 2 deletions bin/node-gyp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,12 @@ function run () {
prog.commands[command.name](command.args, function (err) {
if (err) {
log.error(command.name + ' error')
log.error('stack', err.stack)
errorMessage()
if (err.noPython) {
log.error(err.message)
} else {
log.error('stack', err.stack)
errorMessage()
}
log.error('not ok')
return process.exit(1)
}
Expand Down
35 changes: 24 additions & 11 deletions lib/configure.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,7 @@ PythonFinder.prototype = {
this.log.silly('stripping "rc" identifier from version')
version = version.replace(/rc(.*)$/ig, '')
}
var range = semver.Range('>=2.5.0 <3.0.0')
var range = semver.Range('>=2.6.0 <3.0.0')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be 2.7.0 instead of 2.6.0 at this point? Python 2.6 has been EOL for 5 years.

var valid = false
try {
valid = range.test(version)
Expand All @@ -479,19 +479,32 @@ PythonFinder.prototype = {
},

failNoPython: function failNoPython () {
var errmsg =
'Can\'t find Python executable "' + this.python +
'", you can set the PYTHON env variable.'
this.callback(new Error(errmsg))
const err = new Error(
'\n******************************************************************\n' +
`node-gyp can't use "${this.python}",\n` +
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to change the wording from "can't find" to "can't use"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because this error is printed both when python can't be found, and on windows when it's a bad version.
In #1268 the error outputed was:

stack Error: Can't find Python executable "C:\Program Files\Python35\python.EXE", you can set the PYTHON env variable.

which makes less sense.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to distinguish between "can't be found" and bad version? In the example output in the PR description (i.e. the not found case), I think the message is now worse.

node-gyp can't use "python",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's quite convoluted code...
I'll give it another look in the morning but I can't promise I can find an elegant solution. This code comes after several rounds of geusses so it difficult to know what was found but deemed unsuitable or if nothing was found at all.

Copy link

Choose a reason for hiding this comment

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

Didn't notice this PR before doing my own fix. Solves the above issue and detects if there is a wrong version + a possible infinite loop fix. See #1325

'It is recommended that you install python 2.7, set the PYTHON env,\n' +
'or use the --python switch to point to a Python >= v2.6.0 & < 3.0.0.\n' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be 2.7.0 instead of 2.6.0 at this point? Python 2.6 has been EOL for 5 years.

'For more information consult the documentation at:\n' +
'https://github.com/nodejs/node-gyp#installation\n' +
'***********************************************************************'
);
err.noPython = true;
this.callback(err)
},

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))
const err = new Error(
'\n******************************************************************\n' +
`Python executable "${this.python}" is v${badVersion}\n` +
'this version is not supported by GYP and hence by node-gyp.\n' +
'It is recommended that you install python 2.7, set the PYTHON env,\n' +
'or use the --python switch to point to a Python >= v2.6.0 & < 3.0.0.\n' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be 2.7.0 instead of 2.6.0 at this point? Python 2.6 has been EOL for 5 years.

'For more information consult the documentation at:\n' +
'https://github.com/nodejs/node-gyp#installation\n' +
'***********************************************************************'
);
err.noPython = true;
this.callback(err)
},

// Called on Windows when "python" isn't available in the current $PATH.
Expand Down
31 changes: 17 additions & 14 deletions test/test-find-python.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ test('find python - python', function (t) {
})

test('find python - python too old', function (t) {
t.plan(4)
t.plan(5)

var f = new TestPythonFinder('python', done)
f.which = function(program, cb) {
Expand All @@ -88,13 +88,14 @@ test('find python - python too old', function (t) {
}
f.checkPython()

function done(err, python) {
t.ok(/is not supported by gyp/.test(err))
function done(err) {
t.ok(/this version is not supported by GYP/.test(err))
t.ok(/For more information consult the documentation/.test(err))
}
})

test('find python - python too new', function (t) {
t.plan(4)
t.plan(5)

var f = new TestPythonFinder('python', done)
f.which = function(program, cb) {
Expand All @@ -108,8 +109,9 @@ test('find python - python too new', function (t) {
}
f.checkPython()

function done(err, python) {
t.ok(/is not supported by gyp/.test(err))
function done (err) {
t.ok(/this version is not supported by GYP/.test(err))
t.ok(/For more information consult the documentation/.test(err))
}
})

Expand All @@ -123,8 +125,8 @@ test('find python - no python', function (t) {
}
f.checkPython()

function done(err, python) {
t.ok(/Can't find Python executable/.test(err))
function done(err) {
t.ok(/For more information consult the documentation/.test(err))
}
})

Expand Down Expand Up @@ -170,8 +172,8 @@ test('find python - no python2, no python, unix', function (t) {
}
f.checkPython()

function done(err, python) {
t.ok(/Can't find Python executable/.test(err))
function done(err) {
t.ok(/For more information consult the documentation/.test(err))
}
})

Expand Down Expand Up @@ -242,7 +244,7 @@ test('find python - python 3, use python launcher', function (t) {

test('find python - python 3, use python launcher, python 2 too old',
function (t) {
t.plan(9)
t.plan(10)

var f = new TestPythonFinder('python', done)
f.checkedPythonLauncher = false
Expand Down Expand Up @@ -271,8 +273,9 @@ test('find python - python 3, use python launcher, python 2 too old',
}
f.checkPython()

function done(err, python) {
t.ok(/is not supported by gyp/.test(err))
function done(err) {
t.ok(/this version is not supported by GYP/.test(err))
t.ok(/For more information consult the documentation/.test(err))
}
})

Expand Down Expand Up @@ -334,6 +337,6 @@ test('find python - no python, no python launcher, bad guess', function (t) {
f.checkPython()

function done(err, python) {
t.ok(/Can't find Python executable/.test(err))
t.ok(/For more information consult the documentation/.test(err))
}
})