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: add node-pre-gyp support #1095

Merged
merged 13 commits into from
Aug 21, 2023

Conversation

samuelmaddock
Copy link
Member

@mapbox/node-pre-gyp is a popular choice for serving prebuilt native node module binaries. These changes add support for downloading prebuilt binaries using it as well as rebuilding in the case that no prebuilt binaries are found.

A skipPrebuilds option was added to give tests the ability to force rebuilds without using any prebuilt binary modules.

node-sqlite3 is used to test for node-pre-gyp support.

resolves #367

@samuelmaddock samuelmaddock requested a review from a team as a code owner August 2, 2023 20:54
@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2023

Codecov Report

Merging #1095 (ff4bd84) into main (37babee) will decrease coverage by 3.02%.
The diff coverage is 82.05%.

@@            Coverage Diff             @@
##             main    #1095      +/-   ##
==========================================
- Coverage   76.01%   73.00%   -3.02%     
==========================================
  Files          20       21       +1     
  Lines         688      726      +38     
  Branches      129      136       +7     
==========================================
+ Hits          523      530       +7     
- Misses        118      146      +28     
- Partials       47       50       +3     
Files Changed Coverage Δ
src/types.ts 100.00% <ø> (ø)
src/module-type/node-pre-gyp.ts 75.00% <75.00%> (ø)
src/module-rebuilder.ts 91.66% <92.85%> (+0.14%) ⬆️
src/rebuild.ts 72.82% <100.00%> (+0.29%) ⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/cli.ts Outdated
@@ -128,6 +128,7 @@ process.on('unhandledRejection', handler);
useElectronClang: !!argv.useElectronClang,
disablePreGypCopy: !!argv.disablePreGypCopy,
projectRootPath,
skipPrebuilds: !!argv.skipPrebuilds,
Copy link
Member

Choose a reason for hiding this comment

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

Does this new option also impact prebuild or just node-pre-gyp?

Copy link
Member

Choose a reason for hiding this comment

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

It also appears to have a buildFromSource behavior rather than "skipping" the module entirely

Copy link
Contributor

Choose a reason for hiding this comment

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

renamed prebuild -> buildFromSource

@dsanders11
Copy link
Member

@samuelmaddock, you can rebase this on main to pick up #1098 so that Windows CI won't be flaky.

Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

The code changes look good to me

@@ -35,7 +35,7 @@ export async function resetTestModule(testModulePath: string, installModules = t
}

export async function cleanupTestModule(testModulePath: string): Promise<void> {
await fs.remove(testModulePath);
await fs.rmdir(testModulePath, { recursive: true, maxRetries: 10 });
Copy link
Member

Choose a reason for hiding this comment

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

?

it('should not fail running prebuild-install', async () => {
it('should not fail running prebuild-install', async function () {
if (process.platform === 'darwin' && process.arch === 'arm64') {
this.skip(); // farmhash module has no prebuilt binaries for ARM64
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.skip(); // farmhash module has no prebuilt binaries for ARM64
this.skip(); // farmhash module has no prebuilt binaries for ARM64
return;

@georgexu99 georgexu99 merged commit e71316b into electron:main Aug 21, 2023
3 checks passed
georgexu99 added a commit that referenced this pull request Aug 21, 2023
@georgexu99
Copy link
Contributor

o shoot, I merged before sam's comments loaded :PPP I'll make a new pr to address these

@continuous-auth
Copy link

🎉 This PR is included in version 3.3.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.

node-pre-gyp support in the same way than prebuild is supported
6 participants