-
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
Clean-up python detection #1582
Conversation
Try everything until Python is found.
The code here will try all the mechanisms for detecting Python. It will produce log messages at cc @nodejs/node-gyp |
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.
hard to review this so it's a bit of a rubber-stamp lgtm
@@ -1,7 +1,7 @@ | |||
{ | |||
"asi": true, | |||
"laxcomma": true, | |||
"es5": true, | |||
"esversion": 6, |
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.
This is fine for master but shouldn't be backported to 3.x (do we have the equivalent of core's dont-land labels)?
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 don't see labels for this. Didn't we already do the final v3 release? I was expecting the next one to be v4 only, though we could do v3 if there is a pressing need of some sort.
This would always be semver-major defensively. Because this is written using new javascript features and removes workarounds for old node versions, this is effectively semver-major.
Thanks @rvagg! I will land this soon if there is no objection. |
Try everything until Python is found. PR-URL: #1582 Reviewed-By: Rod Vagg <rod@vagg.org>
Landed in a6990ad |
Try everything until Python is found. PR-URL: #1582 Reviewed-By: Rod Vagg <rod@vagg.org>
This changes Python detection to make it try a sequence of possibilities. Every possibility will be tried discarding errors along the way, only giving up when there's nothing else to try. This should make Python detection more robust.
This stops using
which
to find the executable, instead relying on the mechanism that was already used for the py launcher: run Python and printsys.executable
. This means that BAT files can be used, making node-gyp work when depot_tools is the first thing in the path.@refack I based this on your commit from #1269 . Ended up changing much of it, but I used some of the info text so I include your commit for proper credit. Let me know if I should remove it or if you want to land #1269 separately. I rebased it on master and included
es6: true
in.eslintrc
to make linting work (since ES6 was already there for.jshintrc
).Error output when no Python is installed:

Checklist
npm install && npm test
passes