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(bundler/nsis): calculate estimated size on build system #8233

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

FabianLars
Copy link
Member

@FabianLars FabianLars commented Nov 15, 2023

So the project from the devs that reported this issues includes 200+ folders and 1000+ files via the resources config. In this case it's a node_modules folder (intentionally) and putting aside whether or not one should bundle a node_modules folder this can also easily happen in non-nodejs contexts.

Anyway, said app took ~8 minutes to install on my machine. Btw, the files are ~20mb total.

1st commit: Pointing the GetSize call directly at the file instead of the whole folder with a mask reduces that to ~12 seconds - btw the GetSize docs are super confusing, i also thought it can only point to a folder so i looked at other things before i tried this first...

2nd commit: This i'm a bit unsure about whether we can add it to v1 or only to v2 which is why i split it up into another commit. Basically the folder structure is now calculated upfront on the build system, sorted and deduplicated and in the installer the dir creation and file extraction are now 2 steps to not create the same dir again and again. This reduced the install time to ~10 seconds.

Further "improvements"

  • SetCompress off reduces it to ~3 to 4 seconds so that may be something worth to add as a config.
  • If we don't check the install size it can be basically instant (without compression) so my initial idea was to calculate file sizes on the build system and use them for the progress report. That may lead to slightly inaccurate progress bars but i don't think that matters (we can just set it too 100% at the end i guess). Buuut that's a slightly bigger change i didn't want to implement without talking about it and this PR is basically a hotfix for the devs that reached out.

@FabianLars FabianLars requested a review from a team as a code owner November 15, 2023 13:44
@amrbashir
Copy link
Member

2nd commit: This i'm a bit unsure about whether we can add it to v1 or only to v2 which is why i split it up into another commit. Basically the folder structure is now calculated upfront on the build system, sorted and deduplicated and in the installer the dir creation and file extraction are now 2 steps to not create the same dir again and again. This reduced the install time to ~10 seconds.

Yeah this can be backported, we don't provide any guarantees that this file won't be broken at any time.

  • SetCompress off reduces it to ~3 to 4 seconds so that may be something worth to add as a config.

We do have an option to control the algorithim, we can add a variant to disable compression.

  • If we don't check the install size it can be basically instant (without compression) so my initial idea was to calculate file sizes on the build system and use them for the progress report. That may lead to slightly inaccurate progress bars but i don't think that matters (we can just set it too 100% at the end i guess). Buuut that's a slightly bigger change i didn't want to implement without talking about it and this PR is basically a hotfix for the devs that reached out.

I am not sure I am following, the progress bar doesn't depend on the GetSize call and caluclating the size at build time should be okay.

@FabianLars
Copy link
Member Author

I am not sure I am following, the progress bar doesn't depend on the GetSize call and caluclating the size at build time should be okay.

Oh right. Nevermind then, i mixed something up.

@FabianLars FabianLars marked this pull request as draft November 15, 2023 15:14
@FabianLars
Copy link
Member Author

Okay turns out my GetSize change was broken (always returned 0 so ig it really only works on folders) so i'll add the rust upfront solution. Running GetSize just once at the end and let it calculate the whole dir was pretty slow too (not 8 minutes though)

@FabianLars FabianLars marked this pull request as ready for review November 15, 2023 16:13
@amrbashir amrbashir changed the title fix(bundler): Fix nsis installer taking longer than expected to install resources fix(bundler/nsis): calculate estimated size on build system Nov 15, 2023
@amrbashir amrbashir merged commit 92bc7d0 into 1.x Nov 15, 2023
12 checks passed
@amrbashir amrbashir deleted the fix/nsis-resources branch November 15, 2023 18:53
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.

None yet

2 participants