-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
build: shrink bloated addon binaries on windows #2060
Conversation
Are there any backward compatibility concerns with this, or is it just going to impact size? It'd be good one to repeat the problems we had with duplicate symbol checking #1891 tagged @joaocgreis too |
@rvagg to my knowledge this just affects size! |
sgtm, would prefer one of our windows reviewers to sign off tho |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Used this node-gyp to run Node vcbuild test-addons
, everything was ok.
PR-URL: #2060 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
cool, landed @ e18a61a |
Nice work. Size matters. Do we have a few anecdotes about how much smaller the new binaries are? |
btw is there any reason to not backport this to 5.x? |
@rvagg oops, this blew me by - no i don't think so, we should be fine to land this on |
@rvagg what's the typical backport process for this? I can open a PR against the |
Ah, not really a proper process or timeline for this. I normally just cherry-pick things not server-major, 5.x is super close to master anyway. I’ll try and do a batch this week and see what it looks like for a release. I guess were due for some action around here including a 7.x. |
PR-URL: #2060 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Checklist
npm install && npm test
passesDescription of change
Closes nodejs/node#29501.
Refs #1118.
Moves build-time optimizations to
node-gyp
to shrink the binary size accidentally bloated by a change tocommon.gypi
in core. Also furthers the goal of decoupling addons from being affected bycommon.gypi
changes.Feel free to tell me if this isn't quite what the issue had in mind and i'll adapt accordingly.
cc @richardlau @deepak1556