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

Only deref top-level symlinks in node_modules when copying assets #18570

Merged
merged 4 commits into from Dec 13, 2018

Conversation

Projects
None yet
3 participants
@daviwil
Copy link
Member

daviwil commented Dec 11, 2018

Identify the Bug

Fixes #18490

Description of the Change

This change fixes an issue that caused some of Atom's installation packages (like the .rpm package) to gain around 2x in size (135 MB to 270 MB) between Atom 1.30 and 1.31.

The core issue came from a small change in our packaging logic that caused all symlinks under node_modules to be dereferenced. This was a big problem for dugite which includes a full Git binary distribution where many Git commands are symlinked back to the core git binary. Since we dereferenced symlinks, that caused all of these command symlinks to turn into full-sized git binaries, bloating our distribution size greatly.

Alternate Designs

Instead of dereferencing all top-level symlinks under Atom's node_modules folder , we could scan the folders under /packages and copy those directly into the output folder. I ultimately thought this was less flexible and forward-looking than scanning symlinks in node_modules, but I'm happy to go with the other approach if people think it's necessary.

Possible Drawbacks

With this logic, other non-package symlinked folders under node_modules will also be copied into the output folder if they exist, but this should only be a problem (or benefit?) when producing local Atom builds.

Verification Process

  • Produce full build of Atom and ensure that git-* commands are still symlinked inside of:
    • out/app/node_modules/dugite/git/libexec/gitcore
    • out/atom-*/resources/app.asar.unpacked/node_modules/dugite/git/libexec/git-core
  • Produce Atom .rpm package and ensure that it's once again in the neighborhood of 150MB
  • Produce release packages on all OSes to ensure that Git functionality works in the GitHub package
    • Windows
    • Linux
    • macOS

Release Notes

Fixed an issue causing some Atom release packages to double in size

@daviwil daviwil requested a review from jasonrudolph Dec 11, 2018

@jasonrudolph
Copy link
Member

jasonrudolph left a comment

  • Produce Atom .rpm package and ensure that it's once again in the neighborhood of 150MB

Nice!

Are there user-facing operations we should perform in Atom to verify that this change hasn't introduced any regressions?

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Dec 12, 2018

Yep! I'm gonna produce packaged builds on all 3 platforms and verify that Git functionality works in the GitHub package, will add that to the verification list.

@daviwil daviwil merged commit 61e7017 into master Dec 13, 2018

3 checks passed

Atom Pull Requests #20181213.2 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daviwil daviwil deleted the dw-fix-dugite-duplication branch Dec 13, 2018

@tbodt

This comment was marked as off-topic.

daviwil added a commit that referenced this pull request Dec 19, 2018

Merge pull request #18570 from atom/dw-fix-dugite-duplication
Only deref top-level symlinks in node_modules when copying assets

daviwil added a commit that referenced this pull request Dec 19, 2018

Merge pull request #18570 from atom/dw-fix-dugite-duplication
Only deref top-level symlinks in node_modules when copying assets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment