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

Try python launcher when stock python is python 3. #992

Merged
merged 2 commits into from
Oct 8, 2016
Merged

Try python launcher when stock python is python 3. #992

merged 2 commits into from
Oct 8, 2016

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jul 13, 2016

See #987 (comment). Add lots and lots of tests that verify all code paths.

R=@rvagg?

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-commit/8/

@bnoordhuis
Copy link
Member Author

https://ci.nodejs.org/view/All/job/nodegyp-test-commit/8/nodes=win2012r2/console - building the test add-on fails but I don't think it's related to this PR.

Let's see what a CI run of master does: https://ci.nodejs.org/view/All/job/nodegyp-test-commit/10/

@rvagg
Copy link
Member

rvagg commented Jul 14, 2016

ack

@bnoordhuis
Copy link
Member Author

Rebased, new CI: https://ci.nodejs.org/view/All/job/nodegyp-test-commit/31/

@rvagg Was your ACK an implicit LGTM or an 'I am aware this pull request exists'?

@jbergstroem
Copy link
Member

LGTM

Break up findPython() into pieces that can be tested in isolation and
add tests that do so.

PR-URL: #992
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
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: #987
PR-URL: #992
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@bnoordhuis bnoordhuis merged commit 37ae7be into nodejs:master Oct 8, 2016
@bnoordhuis bnoordhuis deleted the fix987 branch October 8, 2016 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants