-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
build: allow build with system python 3 #16058
Conversation
Waiting for CI to determine if changes are needed for windows. |
CI: https://ci.nodejs.org/job/node-test-commit/12832/ EDIT: CI was green. |
cc/ @nodejs/build @nodejs/python |
configure
Outdated
raise e | ||
os.symlink(sys.executable, python_link) | ||
|
||
os.environ['PATH'] = bin_override + ':' + os.environ['PATH'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a no-op as far as I can tell, it doesn't affect the parent shell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gyp does a "python -c" call, and this is required for it to work. Will add comment
configure
Outdated
bin_override = None | ||
|
||
def setup_bin_overrides(): | ||
global bin_override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason you use a global instead of returning a value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It mirrored what other stages were doing, with setting values in "o". Returning makes more sense, will change
configure
Outdated
bin_override = os.path.abspath('out/tools/bin') | ||
try: | ||
os.makedirs(bin_override) | ||
except os.error as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the rest of the file, can you use OSError
in this function?
configure
Outdated
@@ -1428,6 +1458,10 @@ if options.prefix: | |||
|
|||
config = '\n'.join(map('='.join, config.iteritems())) + '\n' | |||
|
|||
setup_bin_overrides() | |||
if bin_override: | |||
config = 'export PATH:=' + bin_override + ':${PATH}\n' + config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why ${PATH}
and not $(PATH)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's what was on the stack overflow answer. 😝 Will change if I can
When the system python is python 3, configure now creates a directory with a symlink called 'python' to python2, uses it when it calls run_gyp, and puts it in config.mk so that it propagates to everything that make launches
73bcd94
to
741a772
Compare
Updated for reviews. Diff @ forivall@528a99d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the system python is python 3, configure now creates a directory with a symlink called 'python' to python2, uses it when it calls run_gyp, and puts it in config.mk so that it propagates to everything that make launches PR-URL: #16058 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
When the system python is python 3, configure now creates a directory with a symlink called 'python' to python2, uses it when it calls run_gyp, and puts it in config.mk so that it propagates to everything that make launches PR-URL: #16058 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
When the system python is python 3, configure now creates a directory with a symlink called 'python' to python2, uses it when it calls run_gyp, and puts it in config.mk so that it propagates to everything that make launches PR-URL: nodejs/node#16058 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This change made Node.js build need install distutils packages which is a dependency change, python3 won't break old build environment but distutils dependency will, maybe we can implement a less dependent method to find default python binary? |
@hefangshi distutils is part of python's stdlib. Are you thinking of setuptools? |
Looks like @hefangshi what platform and version of Python are you on? Also this commit doesn't add the distutils dependency, it was there before (from the diff 2b5b423): |
+1 on landing on a future v6.x release, and it should land with #16241 (assuming that lands). |
Yeah, i had assumed the distutils would be fine since it would be loaded if it was python 3 or <= 2.5. there's probably an alternative version of 'which' that doesn't depend on distutils, since that's the only thing it's used for. |
waiting for #16241 before landing this on v6.x. Please LMK if we don't need to wait |
When the system python is python 3, configure now creates a directory with a symlink called 'python' to python2, uses it when it calls run_gyp, and puts it in config.mk so that it propagates to everything that make launches PR-URL: #16058 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
When the system python is python 3, configure now creates a directory with a symlink called 'python' to python2, uses it when it calls run_gyp, and puts it in config.mk so that it propagates to everything that make launches PR-URL: #16058 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
When the system python is python 3, configure now creates a directory with a symlink called 'python' to python2, uses it when it calls run_gyp, and puts it in config.mk so that it propagates to everything that make launches PR-URL: #16058 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
When the system python is python 3, configure now creates a directory
with a symlink called 'python' to python2, uses it when it calls
run_gyp, and puts it in config.mk so that it propagates to everything
that make launches
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
build