gyp should use Python 3 not Python #2660
Conversation
Node.js 12 is end-of-life. https://nodejs.org/en/about/releases Python 3.6 is end-of-life. https://devguide.python.org/#status-of-python-branches
| @@ -5,4 +5,4 @@ | |||
|
|
|||
| set -e | |||
| base=$(dirname "$0") | |||
| exec python "${base}/gyp_main.py" "$@" | |||
| exec python3 "${base}/gyp_main.py" "$@" | |||
There was a problem hiding this comment.
Shouldn't this variable consider python-related settings described here?
https://github.com/nodejs/node-gyp#configuring-python-dependency
There was a problem hiding this comment.
My PR would effectively do what you're suggesting @biodranik. (I haven't re-reviewed my own PR in a while, so I'd want a chance to do that before/if it got merged.)
(For those familiar with the deep workings of this repo: My PR would make gyp run with the Python binary found by find-python.js... find-python.js is the script that honors all those Python binary configuration options you linked to.)
Granted, most users with a Python binary that can run gyp have it accessible on their PATH as python and/or as python3. (Or they have some compatible Python as python and/or python3 on their PATH). So for most users, my PR is not needed.
For those who don't; for the few who rely exclusively on those Python binary configuration options node-gyp offers to pass find-python.js, who don't have such a compatible-with-gyp Python binary on their PATH, it feels like the configs are a bit of a lie, and them not being used to actually run gyp is a bit of a broken promise (promise implied by the existence of the config options).
There was a problem hiding this comment.
Actually... digging deeper and doing some quick testing... the file gyp/gyp modified by this PR is apparently never run by node-gyp. This file is a tiny bash script wrapper around gyp/gyp_main.py. node-gyp skips the wrapper in favor of running gyp/gyp_main.py directly.
It does so here:
$ ~/node-gyp/bin/node-gyp.js configure
gyp info it worked if it ends with ok
gyp info using [email protected]
gyp info using [email protected] | linux | x64
gyp info find Python using Python version 3.8.10 found at "/usr/bin/python3"
gyp info spawn /usr/bin/python3
gyp info spawn args [
gyp info spawn args '/home/[user]/node-gyp/gyp/gyp_main.py',
[ . . . ]
gyp info spawn args ]I also modified the gyp/gyp script to echo some stuff and touch a file at an absolute path, then ran node-gyp rebuild to see if anything happened. Nothing was echoed from gyp/gyp, and the file was not touched/was not present on my disk. So... I conclude the gyp/gyp script was not run by node-gyp.
(end of expanded info)
Given the above, I think this PR is fully unnecessary for the node-gyp repo, should make zero difference, as this is in a file that is not ever being run. (Might be of some use at https://github.com/nodejs/gyp-next??)
There was a problem hiding this comment.
👍 to the fix. [ EDIT: Hmm, I think this PR does nothing in the context of node-gyp. (node-gyp never runs the file changed by this PR.) I explained it in another comment here: #2660 (comment) ]
nits:
- This change should also be done in https://github.com/nodejs/gyp-next at some point.
- This has a few extra/duplicate/irrelevant commits now (after a force push was apparently done on
masterbranch recently). - Please merge this with a commit message prefix that will show on the CHANGELOG: (list here) such as
fix:,gyp:, orpython:. Orfix(gyp):,fix(python):...
|
Closing in favor of #2362 |
See #2654
Checklist
npm install && npm testpassesDescription of change