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

fix: allow using forks for prebuilt modules #1155

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

samuelmaddock
Copy link
Member

It may sometimes be necessary to fork a prebuilt module package to fix an issue. In such cases, the rebuilder can miss prebuilt modules, forcing them to be built from source.

To avoid this, the module rebuilders will now also search for possible forked packages.

  • @mapbox/node-pre-gyp may instead be node-pre-gyp or @namespace/node-pre-gyp
  • prebuild-install may instead be @namespace/prebuild-install
  • prebuilds may instead be @namespace/prebuilds

@samuelmaddock samuelmaddock requested a review from a team as a code owner October 1, 2024 22:28
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

I feel like this is gonna trip false positives, especially on @namespace/prebuilds. This is a massive edge case I'd rather cause slightly more work for folks doing this than Just Guess And Hope and negatively impact everyone else.

If we can come up with a good heuristic for identifying forks beyond just "is the suffix still the same" I'd be down.

E.g. Maybe we can check other fields in the suspect package.json such as repo or something and see if it matches the upstream repo url.

Either that or letting modules provide a field in the package.json that we specifically read and understand. E.g. packageJSON.electronRebuildConfig = { moduleType: 'prebuilds', builderModuleName: '@foo/prebuilds' }

@samuelmaddock
Copy link
Member Author

samuelmaddock commented Oct 2, 2024

I feel like this is gonna trip false positives, especially on @namespace/prebuilds

The nice thing about our current architecture is that even making the wrong assumption will fallback gracefully.

async rebuild(cacheKey: string): Promise<boolean> {
if (
!this.rebuilder.buildFromSource && (
(await this.findPrebuildifyModule(cacheKey)) ||
(await this.findPrebuildInstallModule(cacheKey)) ||
(await this.findNodePreGypInstallModule(cacheKey)))
) {
return true;
}
return await this.rebuildNodeGypModule(cacheKey);
}

In each case, even if @namespace/prebuildify is a false positive, findPrebuiltModule() should fail to find the module unless the APIs match exactly. Once it fails, it can fallthrough to building from source.

async findPrebuildifyModule(cacheKey: string): Promise<boolean> {
if (await this.prebuildify.usesTool()) {
d(`assuming is prebuildify powered: ${this.prebuildify.moduleName}`);
if (await this.prebuildify.findPrebuiltModule()) {
await this.writeMetadata();
await this.cacheModuleState(cacheKey);
return true;
}
}
return false;
}

Looking through the public packages for node-pre-gyp, prebuildify, and prebuild-install I see no false positives that would cause any fatal error.

If we can come up with a good heuristic for identifying forks beyond just "is the suffix still the same" I'd be down.

E.g. Maybe we can check other fields in the suspect package.json such as repo or something and see if it matches the upstream repo url.

I was considering this when looking through node-pre-gyp forks sorted by popularity on npmjs. Enough forks change the repo URL to point to their own fork where this probably wouldn't be reliable enough.

Using the bin seems to be the most reliable indicator. This is already covered as we search for the binary and skip if it's not found (implemented similarly in each prebuilt module type).

async locateBinary(): Promise<string | null> {
const packageName = await this.findPackageInDependencies('node-pre-gyp');
if (!packageName) return null;
return locateBinary(
this.modulePath,
`node_modules/${packageName}/bin/node-pre-gyp`
);
}

async findPrebuiltModule(): Promise<boolean> {
const nodePreGypPath = await this.locateBinary();
if (nodePreGypPath) {

Either that or letting modules provide a field in the package.json that we specifically read and understand. E.g. packageJSON.electronRebuildConfig = { moduleType: 'prebuilds', builderModuleName: '@foo/prebuilds' }

This is a good way to eliminate the possibility of false positives, but requires more work for folks which I'd hopefully like to avoid.


In summary, I'm less concerned by false positives since

  • rebuild gracefully fallsback to building from source if each of the prebuilt modules fail
  • rebuild already searches for the exact binary required by the prebuilt modules (e.g. node_modules/@namespace/node-pre-gyp/bin/node-pre-gyp)
  • the names for prebuilt modules (prebuildify, prebuild-install, node-pre-gyp) are fairly unique
  • none of the public @namespace/{prebuildify|prebuild-install|node-pre-gyp} modules on npm indicate that they would trip up our rebuild logic

The motivation behind this PR is to use a fork of node-pre-gyp. If we're concerned about a particular prebuild module, I'd be open to reducing the scope of this change to only support forks of node-pre-gyp. That'd at least reduce the blast radius of potential false positives.

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Agreed that given other code stars would have to align to false-positive. CI is still broken, but approving the concept

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 76.80%. Comparing base (9ebf9bd) to head (39af3f2).

Files with missing lines Patch % Lines
src/module-type/index.ts 85.71% 0 Missing and 1 partial ⚠️
src/module-type/node-pre-gyp.ts 80.00% 0 Missing and 1 partial ⚠️
src/module-type/prebuild-install.ts 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1155      +/-   ##
==========================================
- Coverage   76.99%   76.80%   -0.19%     
==========================================
  Files          21       21              
  Lines         765      776      +11     
  Branches      144      150       +6     
==========================================
+ Hits          589      596       +7     
- Misses        122      124       +2     
- Partials       54       56       +2     

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

@VerteDinde VerteDinde merged commit 683129a into electron:main Oct 2, 2024
3 checks passed
Copy link

🎉 This PR is included in version 3.6.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@samuelmaddock samuelmaddock deleted the fix/allow-forks branch October 2, 2024 18:43
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.

4 participants