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

chore (main): vendor in Python packaging for Py3.12 #214

Merged
merged 3 commits into from
Oct 23, 2023

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Oct 20, 2023

Copy link
Contributor

@rzhao271 rzhao271 left a comment

Choose a reason for hiding this comment

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

Do we still need line 15 in the TOML file?
The line that says dependencies = ["packaging>=23.1"]

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 21, 2023

I think for open source license compliance, LICENSE, LICENSE.APACHE and LICENSE.BSD from here: https://github.com/pypa/packaging should be included alongside all the vendored packaging files.

@cclauss cclauss requested a review from rzhao271 October 22, 2023 15:22
@cclauss
Copy link
Contributor Author

cclauss commented Oct 22, 2023

  • dependencies = ["packaging>=23.1"] commented out with an added explanation.
  • LICENSE, LICENSE.APACHE, and LICENSE.BSD copied into pylib/packaging

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

rubberstamp

@cclauss cclauss merged commit 004c0b0 into nodejs:main Oct 23, 2023
20 checks passed
@cclauss cclauss deleted the vendor-in-python-packaging branch October 23, 2023 12:49
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 25, 2023

This PR worked for me!

The details of my testing are posted below, for anyone who is curious.

Test method:

  • This post is about testing in Ubuntu (Docker ubuntu:latest image), since the Python package from Ubuntu package repos doesn't provide setuptools/distutils, unlike Python from HomeBrew on mac, which seems to come with several packages by default, sometimes including setuptools that provides distutils, or packaging... Wasn't totally sure about this, so I tested on Ubuntu which I know doesn't come with any of these "out of the box".
    • I initially tried testing in macOS, but gyp-next v0.15.1 already works with my copy of Python 3.12 from Homebrew. Homebrew's Python 3.12 appears to come with packaging "out of the box"...
  • Install needed packages (apt-get update && apt-get install add-apt-repository software-properties-common git curl make gcc g++ python3 pkg-config libsecret-1-dev)
  • I installed python3.12 from deadsnakes PPA (add-apt-repository ppa:deadsnakes/ppa and apt-get install python3.12)
  • Using nvm, I installed NodeJS v18.18.2, which comes with with npm v9.8.1
  • I cloned node-keytar repo, as a random repo that I know has native C/C++ code to build in it, and ran npm install in it.
  • In my local checkout of node-gyp repo, I did ./update-gyp.py v0.16.0 to sync gyp-next with this fix in it.
  • I did export PYTHON=python3.12 to make sure to use Python 3.12 with node-gyp in the following tests
  • Then I tried ~/node-gyp/bin/node-gyp.js rebuild in the node-keytar repo I cloned before

Confirming this fix was needed (Testing the "null hypothesis"):
I also wanted to confirm once again that this fix is needed and my testing setup is sound, so I checked a couple of other things.

  • node-gyp main branch (no modifications) has an error ModuleNotFoundError: No module named 'distutils'
  • node-gyp main branch + gyp-next v0.15.1 has an error ModuleNotFoundError: No module named 'packaging'

Main Result (This PR Fixes the Errors):

  • node-gyp main + gyp-next v0.16.0 (including this PR) works for me on Python 3.12 ✅

@cclauss
Copy link
Contributor Author

cclauss commented Oct 25, 2023

I confirm that macOS homebrew's brew install python@3.12 will also install python-packaging.
You can do brew list to verify its presence.

archlinux-github pushed a commit to archlinux/aur that referenced this pull request Jun 13, 2024
`node-gyp` already resolved python `setuptools` dependency by vendor in the `packaging` package, as described [here](nodejs/gyp-next#214), last year and released `node-gyp` v10 containing this fix this March (see [here](nodejs/node-gyp#2869 (comment))). But true, without `python-distutils-extra`, `makepkg` will fail with `node-gyp` not found.
buffcode added a commit to buffcode/electron-printer that referenced this pull request Jun 19, 2024
Adds support for Python 3.12+

See nodejs/gyp-next#214
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.

Python tests broken
6 participants