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: electron-builder fails when list of node_modules files is too big to pass in a glob #8725

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

beyondkmp
Copy link
Collaborator

@beyondkmp beyondkmp commented Dec 6, 2024

fix #8705

root cause

When listing all files, if there are too many files, it will trigger glob's length limit.

how to fix

If all files in a folder are unpacked, use the folder name directly instead of listing all files.

Copy link

changeset-bot bot commented Dec 6, 2024

🦋 Changeset detected

Latest commit: 7be5628

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@beyondkmp beyondkmp marked this pull request as draft December 6, 2024 05:46
@beyondkmp
Copy link
Collaborator Author

The main issue in this MR is caused by users directly configuring the entire node_modules in the asarUnpack pattern.

asarUnpack supports '!', but electron/asar doesn't support this '!'. This problem will occur whenever users have directories containing a large number of files.

My thought is whether electron-builder's asarUnpack should also not support '!' patterns, keeping consistent with electron/asar. @mmaietta Do you have any good ideas or suggestions?

@mmaietta
Copy link
Collaborator

Is there a way we can resolve ! filters before passing to electron/asar's minimatch?

It seems like a pretty hefty decrease in electron-builder functionality if we remove the ! filter feature?

@beyondkmp beyondkmp marked this pull request as ready for review January 9, 2025 12:21
@beyondkmp beyondkmp requested a review from mmaietta January 9, 2025 12:21
if (tmpUnpackedPaths.size === fileSet.files.length) {
const relative = path.relative(this.config.defaultDestination, fileSet.destination)
const destination = path.resolve(this.rootForAppFilesWithoutAsar, relative)
unpackedPaths.add(`${destination}${path.sep}**`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you clarify this area of code with a comment? It looks like we're determining whether all files in the fileSet are being unpacked, if so, then we glob them together with an umbrella glob destination/** to reduce the number of unpacked paths that are being passed to minimatch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, exactly as you mentioned.

electron/asar doesn't implement ! filtering. Having to do the conversion at the electron-builder layer is very complicated, and there's currently no good method for conversion.

Therefore, we can only solve this problem from a different angle. Exceeding the length limit is always caused by filtering one or more directories. If we're just filtering specific files, it won't exceed the length limit. So solving these directory filtering issues would solve this problem.

Currently, there might still be some edge cases, such as when a directory contains tens of thousands of files, and the requirement is to put just a few files from this directory into asar while putting all other files into asar.unpack.
In practical use, this situation probably won't occur - it's rare to have a directory with tens of thousands of files where only a few files need to be in asar.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For unpackedPaths.add(${destination}${path.sep}**), is it possible to use ${relative}${path.sep}** to shorten the minimatch paths even further? I can't recall off the top of my head whether it requires absolute paths or if relative works.

Copy link
Collaborator Author

@beyondkmp beyondkmp Jan 11, 2025

Choose a reason for hiding this comment

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

Good idea. However, we need to use **${path.sep}${relative}${path.sep}**.

I've changed files to use this format as well, and current testing shows no issues.

const matchUnpacker = (file: string, dest: string, stat: fs.Stats, tmpUnpackedPaths: Set<string>) => {
if (this.config.unpackPattern?.(file, stat)) {
log.debug({ file }, "unpacking")
tmpUnpackedPaths.add(`**${path.sep}${dest}`)
return
}

@github-actions github-actions bot added the nsis label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

electron-builder fails when list of node_modules files is too big to pass in a glob
2 participants