-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Make hermes-executor-common a static lib #32683
Conversation
Base commit: 79d20a1 |
Base commit: 79d20a1 |
Hm, weird that it shows increase in size, I would expect the size actually decrease lol, or stay the same for jsc 🤔 |
Yea that's weird, not sure if its an issue with the size calculation or there's actually a size change. Could be some cache that causes the old executor-common.so file to still be there but also the increased size of the executor.so file. I did verify that the common.so file is no longer there after a clean build. |
I verified the size for my app and this causes a ~0.1mb size decrease according to the play store bundle explorer comparing versions before and after this change. |
Thanks for addressing this @janicduplessis
According to the docs that should not be a problem as It's anyway good to fix those (and no size change should actually be reported). |
@cortinico has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @janicduplessis in b2cf24f. When will my fix make it into a release? | Upcoming Releases |
@cortinico Sure I can open a follow up pr to fix that other one soon. |
Summary: I've been seeing a couple crashes related to missing hermes-executor-common.so, seems to happen on specific android versions, but can't repro. I investigated this so file more and noticed it is incorrectly linked as a static library here https://github.com/facebook/react-native/blob/b8f415eb6cdc0e0e7a7413b6f9defdcee304d9e8/ReactAndroid/src/main/java/com/facebook/hermes/reactexecutor/Android.mk#L20. There doesn't seem to be any reason for this to be a shared lib so I changed it to be compiled as a static lib. ## Changelog [Android] [Fixed] - Make hermes-executor-common a static lib Pull Request resolved: facebook#32683 Test Plan: - Verify there is no more hermes-executor-common-{release,debug}.so - Test locally in an app to make sure it build and run properly. - Verify that the crash happening on play store pre-launch report doesn't happen anymore. Reviewed By: ShikaSD Differential Revision: D32754968 Pulled By: cortinico fbshipit-source-id: cb57e2d81edb4cbdb1f003dab45c53e594a5a62a
Summary: Follow up to #32683 to also link hermes-inspector statically. ## Changelog [Android] [Fix] - Static link for hermes-inspector Pull Request resolved: #32694 Test Plan: Tested a clean build and made sure hermes-inspector.so doesn't exist anymore. Reviewed By: cortinico Differential Revision: D32791208 Pulled By: ShikaSD fbshipit-source-id: 6076b263469b9e2c6176d488d13292d4f1731dcc
Summary
I've been seeing a couple crashes related to missing hermes-executor-common.so, seems to happen on specific android versions, but can't repro. I investigated this so file more and noticed it is incorrectly linked as a static library here
react-native/ReactAndroid/src/main/java/com/facebook/hermes/reactexecutor/Android.mk
Line 20 in b8f415e
Changelog
[Android] [Fixed] - Make hermes-executor-common a static lib
Test Plan