Skip to content
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.

Sign up for GitHub

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

gyp should use Python 3 not Python #2660

Closed
wants to merge 4 commits into from
Closed

gyp should use Python 3 not Python #2660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
91eb407
Update README.md
cclauss Mar 7, 2022
1206a82
fix: typo on readme
rubiagatra May 3, 2022
a37f186
GitHub Actions: Add Node.js 18 and drop 12
cclauss May 8, 2022
bd8fe9c
gyp should use Python 3 not Python
cclauss May 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ jobs:
fail-fast: false
max-parallel: 15
matrix:
node: [12.x, 14.x, 16.x]
python: ["3.6", "3.8", "3.10"]
node: [14.x, 16.x, 18.x]
python: ["3.7", "3.8", "3.10"]
os: [macos-latest, ubuntu-latest, windows-latest]
runs-on: ${{ matrix.os }}
steps:
- name: Checkout Repository
uses: actions/checkout@v2
uses: actions/checkout@v3
- name: Use Node.js ${{ matrix.node }}
uses: actions/setup-node@v3
with:
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ searching will be done.

### Build for Third Party Node.js Runtimes

When building modules for thid party Node.js runtimes like Electron, which have
When building modules for third party Node.js runtimes like Electron, which have
different build configurations from the official Node.js distribution, you
should use `--dist-url` or `--nodedir` flags to specify the headers of the
runtime to build for.
Expand Down
2 changes: 1 addition & 1 deletion docs/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
## Versions of `node-gyp` that are earlier than v8.x.x
## Versions of `node-gyp` that are earlier than v9.x.x

Please look thru your error log for the string `gyp info using node-gyp@` and if that version number is less than the [current release of node-gyp](https://github.com/nodejs/node-gyp/releases) then __please upgrade__ using [these instructions](https://github.com/nodejs/node-gyp/blob/master/docs/Updating-npm-bundled-node-gyp.md) and then try your command again.

Expand Down
2 changes: 1 addition & 1 deletion gyp/gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@

set -e
base=$(dirname "$0")
exec python "${base}/gyp_main.py" "$@"
exec python3 "${base}/gyp_main.py" "$@"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this variable consider python-related settings described here?
https://github.com/nodejs/node-gyp#configuring-python-dependency

Copy link
Contributor

@DeeDeeG DeeDeeG May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@DeeDeeG DeeDeeG May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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