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

feat: Change node-gyp to @electron/node-gyp #1157

Merged
merged 9 commits into from
Oct 8, 2024
Merged

Conversation

felixrieseberg
Copy link
Member

@felixrieseberg felixrieseberg commented Oct 7, 2024

This PR switches @electron/rebuild to a fork of the original and excellent node-gyp with only one feature added: Support for Node v12. We're doing so to fix ModuleNotFoundError: No module named 'distutils' errors on macOS Sequoia, which ships with Python 3.12 by default. Here is what happened:

  • Python 3.12 has removed distutils, which node-gyp below v10 depended on.
  • macOS Sequoia updated their default version of Python to 3.12.
  • When installing a native Node addon, a lot of code bases suddenly showed ModuleNotFoundError: No module named 'distutils' error messages that are cryptic for anyone who doesn't have a full understanding of the dependency chain involved

This left us with the following choices:

  1. Tell the community to manually install either Python 3.11 or setuptools. While this fixes the issue, it requires manual user intervention and requires our users to first search for the error message.
  2. Upgrade to node-gyp v10. That would have required a bump in minimum Node.js version for @electron/rebuild and in turn all other packages that depend on it, resulting in major version bumps across the entire ecosystem. This too doesn't fix the issue for our users without them performing manual major version upgrades.
  3. Fork node-gyp v10, make it compatible with Node.js v12, and only patch our tools: We chose this option to get a fix out to our users as quickly and painless as possible.

Electron has a major version bump in required Node.js version on its roadmap. Once that is the case, we will sunset this fork and go back to the original node-gyp.

closes #1116

@felixrieseberg felixrieseberg requested a review from a team as a code owner October 7, 2024 14:37
Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @felixrieseberg! I just ran into this last week 🙇

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.09%. Comparing base (a235385) to head (1517455).

❗ There is a different number of reports uploaded between BASE (a235385) and HEAD (1517455). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (a235385) HEAD (1517455)
11 9
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1157      +/-   ##
==========================================
- Coverage   76.80%   67.09%   -9.71%     
==========================================
  Files          21       21              
  Lines         776      775       -1     
  Branches      150      150              
==========================================
- Hits          596      520      -76     
- Misses        124      212      +88     
+ Partials       56       43      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@felixrieseberg felixrieseberg changed the title fix: Change node-gyp to @electron/node-gyp feat: Change node-gyp to @electron/node-gyp Oct 8, 2024
@felixrieseberg felixrieseberg merged commit 96b462a into main Oct 8, 2024
4 checks passed
@felixrieseberg felixrieseberg deleted the felixr-gyp-fork branch October 8, 2024 17:29
Copy link

🎉 This PR is included in version 3.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

need to upgrade node-gyp
4 participants