Add test for python executable search logic. #756
Conversation
| } | ||
| } else { | ||
| log.verbose('`which` succeeded', python, execPath) | ||
| checkPythonVersion() |
There was a problem hiding this comment.
you're missing the critical fix of using which's result
python = execPath
There was a problem hiding this comment.
Well, yes, that's intentional. Mixing a general refactor and bug fixes in a single commit is bad style.
|
Maybe R=@TooTallNate? |
| if (err) { | ||
| return callback(err) | ||
| } | ||
| log.verbose('check python version', '`%s -c "import platform; print(platform.python_version());"` returned: %j', python, stdout) |
There was a problem hiding this comment.
I guess we don't have the Max length linter rule.
|
@thefourtheye What was the verdict on this PR? |
|
I don't have time right now to review this but fwiw I extracted this logic into a separate package a while back: https://github.com/rvagg/check-python and figured one day I'd come back here and PR a full replacement. The logging is the tricky bit, however. |
| if (win) { | ||
| guessPython() | ||
| } else { | ||
| failNoPython() |
There was a problem hiding this comment.
After the refactoring, failNoPython directly calls the configure's callback and brings the execution to end. But ideally it should have called findPython's callback and that should have propagated the error back to the caller of configure. This will still work but looks like it can be improved/done better to me. Please point out if I am missing something obvious.
There was a problem hiding this comment.
But it does, doesn't it? It calls the callback from findPython()'s arguments.
There was a problem hiding this comment.
@bnoordhuis I am sorry. You are correct. As the change was lengthier, I was not able to view it properly I guess. LGTM.
|
LGTM with a suggestion/question. |
|
Landed in 817ed9b with the |
Break out the search logic into a separate function and add a regression
test.
References: #668
R=@rvagg? /cc @refack