build: Fix python path resolution in configure #16460

digitalinfinity · 2017-10-24T20:57:40Z

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

refack · 2017-10-24T21:10:25Z

@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.

refack · 2017-10-24T21:15:11Z

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 ;)

digitalinfinity · 2017-10-24T21:15:53Z

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

refack · 2017-10-24T21:18:33Z

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.

refack · 2017-10-24T21:28:45Z

/cc @nodejs/platform-windows

digitalinfinity · 2017-10-24T21:43:57Z

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?

gibfahn · 2017-10-24T21:45:52Z

configure

@@ -1460,7 +1460,8 @@ if options.prefix:

 config = '\n'.join(map('='.join, config.iteritems())) + '\n'

-bin_override = get_bin_override()
+# On Windows there's no reason to search for a differnt python binary


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

digitalinfinity · 2017-10-25T00:29:45Z

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

refack · 2017-10-25T00:50:15Z

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

refack · 2017-10-25T00:53:21Z

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

gibfahn · 2017-10-25T07:37:06Z

configure

-def get_bin_override():
+def make_bin_override():
+  if sys.platform == 'win32':
+    raise Exception('make_bin_override should not be called on win32.')


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>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Fix python path resolution in configure #16460

build: Fix python path resolution in configure #16460

digitalinfinity commented Oct 24, 2017

refack commented Oct 24, 2017

refack commented Oct 24, 2017 •

edited

Loading

digitalinfinity commented Oct 24, 2017 •

edited

Loading

refack commented Oct 24, 2017

refack commented Oct 24, 2017

digitalinfinity commented Oct 24, 2017

gibfahn Oct 24, 2017

bnoordhuis Oct 24, 2017

digitalinfinity commented Oct 25, 2017

refack commented Oct 25, 2017

refack commented Oct 25, 2017

gibfahn Oct 25, 2017

bzoz Oct 25, 2017

build: Fix python path resolution in configure #16460

build: Fix python path resolution in configure #16460

Conversation

digitalinfinity commented Oct 24, 2017

Checklist

Affected core subsystem(s)

refack commented Oct 24, 2017

refack commented Oct 24, 2017 • edited Loading

digitalinfinity commented Oct 24, 2017 • edited Loading

refack commented Oct 24, 2017

refack commented Oct 24, 2017

digitalinfinity commented Oct 24, 2017

gibfahn Oct 24, 2017

Choose a reason for hiding this comment

bnoordhuis Oct 24, 2017

Choose a reason for hiding this comment

digitalinfinity commented Oct 25, 2017

refack commented Oct 25, 2017

refack commented Oct 25, 2017

gibfahn Oct 25, 2017

Choose a reason for hiding this comment

bzoz Oct 25, 2017

Choose a reason for hiding this comment

refack commented Oct 24, 2017 •

edited

Loading

digitalinfinity commented Oct 24, 2017 •

edited

Loading