build: Fix python path resolution in configure#16460
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the build 
refack commented Oct 24, 2017
@digitalinfinity what's the use case? Are you using a shebang simulating mechanism? IMHO on windows we should use |
Or you can call https://github.com/nodejs/node/blob/master/tools/msvs/find_python.cmd (Windows FTW!) BTW in the new "Opinionated dev shell" I'm writing we won't need that ;) |
There is a discrepancy between the paths derived through As an aside, I'm a python neophyte so if there is a better way in python to do this, happy to update the PR Edit: formatting |
refack commented Oct 24, 2017
So I'll push a suggestion. |
refack commented Oct 24, 2017
/cc @nodejs/platform-windows |
digitalinfinity commented Oct 24, 2017
I like your suggestion better- I'd just add a check in |
configure Outdated
There was a problem hiding this comment.
Aside: the function is not well-named. It suggests it just looks up something but it does all kinds of things as a side effect. Maybe make_bin_override()?
digitalinfinity commented Oct 25, 2017
Thanks @refack - the new changes look good to me! |
refack commented Oct 25, 2017
I won't ✔️ my own code, so we need a 3rd party 😃 |
refack commented Oct 25, 2017
configure Outdated
There was a problem hiding this comment.
Does WSL's sys.platform identify as win32?
PR-URL: nodejs#16460 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: #16460 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: #16460 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: #16460 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs/node#16460 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs/node#16460 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: #16460 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: #16460 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: #16460 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs/node#16460 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
8 participants
The check to validate whether the current python process is the same as
the one resolved from the current path fails if the paths differ by case
which can happen on an operating system with case insensitive file
system behavior like Windows. Canonicalize it by converting both to
lower case if running on Windows.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build