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.

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

deps: Bump node-gyp to latest ^10.2.0 for Python 3.12 compat #137

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Aug 11, 2024

Includes an edition of nodejs/gyp-next which vendors Python packaging library and moves away from distutils, which used to be shipped with Python, but was deprecated starting in Python 3.10, and then dropped starting in Python 3.12.


This solves an error -- Python 3.12 users without distutils (or else setuptools which provides distutils) on their systems would get an error like the following when trying to build native C/C++ code:

    from distutils.version import StrictVersion
ModuleNotFoundError: No module named 'distutils'

Newer gyp-next 0.16.0+ (and by extension node-gyp 10+) avoids the error by not depending on the library that was dropped from being included "out of the box" with Python as of Python 3.12. (It vendors packaging module to do its version string parsing, rather than relying on the now-missing distutils.)

Includes an edition of nodejs/gyp-next which vendors Python 'packaging'
library and moves away from 'distutils', which used to be shipped with
Python, but was deprecated, and then dropped starting in Python 3.12.

---

This solves an error -- Python 3.12 users without 'distutils' (or
'setuptools' which provides 'distutils') on their systems would get
an error like the following when trying to build native C/C++ code:

```
    from distutils.version import StrictVersion
ModuleNotFoundError: No module named 'distutils'
```

Newer gyp-next 0.16.0+ (and by extension node-gyp 10+) avoids the error
by not depending on the library that was dropped from being included
"out of the box" with Python as of Python 3.12. (It vendors 'packaging'
module to do its version string parsing, rather than relying on the
now-missing 'distutils'.)
node-gyp v10 bumped its `engines` field and dropped Node 14 support.

Well... we're going to test CI against Node v14 anyway.

So ignore the `engines` fields of the packages we're installing,
even if they claim not to support Node 14.
Copy link
Contributor

@savetheclocktower savetheclocktower left a comment

Choose a reason for hiding this comment

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

As I understand it, the main benefit of this bump is that end users shouldn't need to install a setuptools package, but I'm having trouble verifying that it's working that way.

Here are the steps I've used:

  • Check out this branch
  • Rebase it against master (since I'm on arm64 and want to get the recent fix)
  • yarn install
  • I did an ln -s into a folder on my PATH to alias the full path to bin/ppm as ppm-test
  • Temporarily removed my setuptools installation with brew uninstall python-setuptools
  • ppm-test install --check fails with the following error:

> module-with-native-code@ install /Users/andrew/Code/JavaScript/ppm/native-module
> node-gyp rebuild

undefined

Traceback (most recent call last):
File "/Users/andrew/Code/JavaScript/ppm/node_modules/npm/node_modules/node-gyp/gyp/gyp_main.py", line 42, in
import gyp # noqa: E402
^^^^^^^^^^
File "/Users/andrew/Code/JavaScript/ppm/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/init.py", line 9, in
import gyp.input
File "/Users/andrew/Code/JavaScript/ppm/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 19, in
from distutils.version import StrictVersion
ModuleNotFoundError: No module named 'distutils'
gyp ERR! configure error
gyp ERR! stack Error: gyp failed with exit code: 1
gyp ERR! stack at ChildProcess.onCpExit (/Users/andrew/Code/JavaScript/ppm/node_modules/npm/node_modules/node-gyp/lib/configure.js:305:16)
gyp ERR! stack at ChildProcess.emit (node:events:365:28)
gyp ERR! stack at Process.ChildProcess._handle.onexit (node:internal/child_process:290:12)
gyp ERR! System Darwin 23.4.0
gyp ERR! command "/Users/andrew/Code/JavaScript/ppm/bin/node" "/Users/andrew/Code/JavaScript/ppm/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /Users/andrew/Code/JavaScript/ppm/native-module
gyp ERR! node -v v16.0.0
gyp ERR! node-gyp -v v9.4.0
gyp ERR! not ok
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! module-with-native-code@ install: node-gyp rebuild
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the module-with-native-code@ install script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR! /Users/andrew/.pulsar/.apm/_logs/2024-09-15T20_47_14_872Z-debug.log

I had initially used npm instead of yarn and figured that was the problem, since yarn supports the resolutions field in package.json and npm doesn't. But no matter what I did, I couldn't convince npm/node-gyp to resolve to 10.2.0.

Things I've tried:

  • Deleting package-lock.json before running yarn install
  • Verifying the string 9.4.0 appears nowhere in yarn.lock
  • Nuking node_modules and reinstalling
  • Adding a "npm/node-gyp": "^10.2.0", line into the resolutions field (was just trying random stuff at this point)

I can't seem to convince yarn that npm/node-gyp should be using 10.2.0. This might need some more voodoo — or maybe we just have to update our npm-cli fork to point to 10.2.0.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Sep 16, 2024

Thank you for the review, I can confirm now, after extra effort to ensure node-gyp uses Python 3.12 on this syetm, that this "fix" does not fix the issue. (Must have been using Python 3.11 inadvertently. Which of course "just works" and doesn't even need this intervention. For best practice, I should have confirmed I could reproduce the issue without the fix in place first before pushing this PR.)

I believe the node-gyp dependency's files may be directly checked in to our forked npm repo's checked in node_modules dir, following the lead of how upstream npm did it. It's weird that upstream did it, presumably for bootstrappability and CI purposes, but I probably shouldn't have followed their lead.

It's also possible git repo

I think dropping our forked npm would fix this and make node-gyp actually resolve to the intended version. Well nope, as I just mentioned but didn't fully think through the implication when writing it out above, official npm@6.14.18 also directly checks in its node-gyp dependency as plain files directly in its repo, they are in the tarball you get from the npm registry and not overrideable by yarn overrides... Only they are even older (node-gyp 5.1.1).

CONCLUSION: Okay, so this needs some messing around with our fork of npm I guess, probably to just have a branch of it that stops directly checking in deps as plain files, because dang is that approach unintuitive and way too inflexible/weird to work with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants