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: unplug packages with native files #853

Merged
merged 8 commits into from
Jan 30, 2020
Merged

feat: unplug packages with native files #853

merged 8 commits into from
Jan 30, 2020

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Jan 30, 2020

What's the problem this PR addresses?

When using packages with native files (files that wont be used by node directly) they have to be manually unplugged. This is an annoyance for one project but more of a hurdle if package authors have to provide a "and remember to unplug [...]" step. Then there are packages silently ignoring access failure causing issues to go unnoticed.

Fixes #663

How did you fix it?

Automatically unplug packages if they contain files with an extension matching a predefined set. This set might not match all use-cases but should take care of most of them.

Running on the yarn repo it unplugs three packages because of these files

/node_modules/node-notifier/vendor/notifu/notifu.exe
/node_modules/clipboardy/fallbacks/windows/clipboard_i686.exe
/node_modules/term-size/vendor/windows/term-size.exe

Running yarn add electron-builder now unplugs these packages which had to be manually unplugged to get electron-builder to work

/node_modules/7zip-bin/win/ia32/7za.exe
/node_modules/app-builder-bin/win/ia32/app-builder.exe

@SimenB This handles the case you ran into when testing berry on the Jest repo. Where weak-napi depends on packages with C/C++ files that the compiler tries to read but fails. If this gets merged you can remove the dependenciesMeta field

@merceyz merceyz requested a review from arcanis January 30, 2020 20:24
@SimenB
Copy link

SimenB commented Jan 30, 2020

Wonderful, thank you! I think this will make it way easier to adopt/try out berry 👍

// Windows can't execute exe files inside zip archives
'.exe',
// The c/c++ compiler can't read files from zip archives
'.h', '.hh', '.hpp', '.c', '.cc', '.cpp',
Copy link

Choose a reason for hiding this comment

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

Suggested change
'.h', '.hh', '.hpp', '.c', '.cc', '.cpp',
'.h', '.hh', '.hpp', '.c', '.cc', '.cpp',
// iOS (react-native)
'.m', '.mm', '.swift',
// Android (react-native)
'.kt',

React-native is a popular node/yarn environment, and react-native often comes with native code. These should be unplugged as well.

Copy link

Choose a reason for hiding this comment

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

I just realized this was a PR from a year ago, so I don't expect anybody to see the above comment!

Copy link
Member

Choose a reason for hiding this comment

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

You'd be surprised 🙂 I'm not expert in RN but that seems reasonable enough, feel free to open a PR 👍

Copy link

Choose a reason for hiding this comment

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

@arcanis thanks for the feedback!

I checked up on the main (master) branch and found this:

if (typeof dependencyMeta.unplugged !== `undefined`)
return dependencyMeta.unplugged;
if (FORCED_UNPLUG_PACKAGES.has(pkg.identHash))
return true;
if (customPackageData.manifest.preferUnplugged !== null)
return customPackageData.manifest.preferUnplugged;
if (jsInstallUtils.extractBuildScripts(pkg, customPackageData, dependencyMeta, {configuration: this.opts.project.configuration}).length > 0 || customPackageData.misc.extractHint)
return true;
return false;

Which appears to not use a file extension system at all anymore. I think packages have to add a preferUnplugged option to my package.json files, but that's not super convenient for 3rd party packages I don't control :(

https://next.yarnpkg.com/configuration/manifest#preferUnplugged

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable indeed, PR welcome :)

Copy link
Member

Choose a reason for hiding this comment

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

@fbartho it got moved here:

const FORCED_EXTRACT_FILETYPES = new Set([
// Windows can't execute exe files inside zip archives
`.exe`,
// The c/c++ compiler can't read files from zip archives
`.h`, `.hh`, `.hpp`, `.c`, `.cc`, `.cpp`,
// The java runtime can't read files from zip archives
`.java`, `.jar`,
// Node opens these through dlopen
`.node`,
]);

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.

[Feature] Automatically unplug dependencies with binary executables
4 participants