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

Make hermes-executor-common a static lib #32683

Closed

Conversation

janicduplessis
Copy link
Contributor

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

LOCAL_STATIC_LIBRARIES := libjsireact libhermes-executor-common-release
. 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

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.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels Nov 30, 2021
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Nov 30, 2021
@janicduplessis
Copy link
Contributor Author

cc @ShikaSD @cortinico

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Nov 30, 2021
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,445,722 +80,782
android hermes armeabi-v7a 7,754,787 +63,646
android hermes x86 8,910,885 +70,162
android hermes x86_64 8,856,165 +75,197
android jsc arm64-v8a 9,833,242 +153,383
android jsc armeabi-v7a 8,799,004 +131,449
android jsc x86 9,783,917 +147,949
android jsc x86_64 10,386,807 +153,814

Base commit: 79d20a1
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 79d20a1
Branch: main

@ShikaSD
Copy link
Contributor

ShikaSD commented Nov 30, 2021

Hm, weird that it shows increase in size, I would expect the size actually decrease lol, or stay the same for jsc 🤔

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Nov 30, 2021

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.

@janicduplessis
Copy link
Contributor Author

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.

@cortinico
Copy link
Contributor

Thanks for addressing this @janicduplessis

I investigated this so file more and noticed it is incorrectly linked as a static library here

According to the docs that should not be a problem as If the current module is a shared library or an executable, this variable will force these libraries to be linked into the resulting binary.

It's anyway good to fix those (and no size change should actually be reported).
I've also noticed that hermes-inspector is also affected by this. Don't you mind fixing it as well?

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @janicduplessis in b2cf24f.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Dec 1, 2021
@janicduplessis janicduplessis deleted the hermes-exec-static branch December 1, 2021 19:22
@janicduplessis
Copy link
Contributor Author

@cortinico Sure I can open a follow up pr to fix that other one soon.

nawbc pushed a commit to NawbExplorer/react-native that referenced this pull request Dec 7, 2021
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
facebook-github-bot pushed a commit that referenced this pull request Dec 13, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. Platform: Android Android applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants