build: Fix python path resolution in configure by digitalinfinity · Pull Request #16460 · nodejs/node · GitHub

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), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

@digitalinfinity what's the use case? Are you using a shebang simulating mechanism? IMHO on windows we should use sys.executable and err if it's not v2.x.

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 which vs paths derived through sys.executable and it looks like os.path.realpath doesn't canonicalize it. I ran into it while running vcbuild on my machine because I have python.exe in C:\Python27 but my PATH environment variable contains C:\python27. I could work around it by updating my PATH but I figure it would be better for configure to be resilient to this difference.

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

So vcbuild already works hard to figure out where a suitible python resides, so there's no need for the which trick (POSIX uses that because the shebang - #!/bin/sh might not resolve to the right python).

I'll push a suggestion.

/cc @nodejs/platform-windows

I like your suggestion better- I'd just add a check in bin_override to error out if it's Windows then, if we never expect it to be called?

differnt -> different (@refack)

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()?

Thanks @refack - the new changes look good to me!

I won't ✔️ my own code, so we need a 3rd party 😃

CI: https://ci.nodejs.org/job/node-test-commit/13416/

Does WSL's sys.platform identify as win32?

It says linux2

PR-URL: nodejs#16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: nodejs/node#16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: nodejs/node#16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 24, 2017

gibfahn reviewed Oct 24, 2017

View reviewed changes

gibfahn approved these changes Oct 25, 2017

View reviewed changes

gibfahn reviewed Oct 25, 2017

View reviewed changes

bnoordhuis approved these changes Oct 25, 2017

View reviewed changes

benjamingr approved these changes Oct 26, 2017

View reviewed changes

build: skip bin override on windows

90640f7

PR-URL: nodejs#16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

refack force-pushed the pypath_fix branch from 3e26589 to 90640f7 Compare October 26, 2017 21:30

refack merged commit 90640f7 into nodejs:master Oct 26, 2017

digitalinfinity deleted the pypath_fix branch October 28, 2017 01:34

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn pushed a commit that referenced this pull request Oct 30, 2017

build: skip bin override on windows

dcb34d8

PR-URL: #16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

gibfahn pushed a commit that referenced this pull request Oct 30, 2017

build: skip bin override on windows

bd98f66

PR-URL: #16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

gibfahn pushed a commit that referenced this pull request Oct 31, 2017

build: skip bin override on windows

0a1f7fe

PR-URL: #16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins pushed a commit that referenced this pull request Nov 16, 2017

build: skip bin override on windows

e0fab59

PR-URL: #16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

MylesBorins pushed a commit that referenced this pull request Nov 21, 2017

build: skip bin override on windows

da28abb

PR-URL: #16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MylesBorins pushed a commit that referenced this pull request Nov 28, 2017

build: skip bin override on windows

706812b

PR-URL: #16460 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

build: Fix python path resolution in configure by digitalinfinity · Pull Request #16460 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: Fix python path resolution in configure - #16460

build: Fix python path resolution in configure#16460refack merged 1 commit intonodejs:masternodejs/node:masterfrom digitalinfinity:pypath_fixdigitalinfinity/node:pypath_fixCopy head branch name to clipboard

Conversation

digitalinfinity commented Oct 24, 2017

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Oct 24, 2017

Uh oh!

refack commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

digitalinfinity commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Oct 24, 2017

Uh oh!

refack commented Oct 24, 2017

Uh oh!

digitalinfinity commented Oct 24, 2017

Uh oh!

gibfahn Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

digitalinfinity commented Oct 25, 2017

Uh oh!

refack commented Oct 25, 2017

Uh oh!

refack commented Oct 25, 2017

Uh oh!

gibfahn Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

bzoz Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

build: Fix python path resolution in configure#16460
refack merged 1 commit into
nodejs:masterfrom
digitalinfinity:pypath_fix

refack commented Oct 24, 2017 •
edited
Loading

digitalinfinity commented Oct 24, 2017 •
edited
Loading