disable hermes for debug android builds & enable hermes for release builds
#18675
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
siddarthkay
force-pushed
the
disable-hermes-locally
branch
from
9c9652e to
7982101
Compare
Jenkins BuildsClick to see older builds (28)
|
siddarthkay
force-pushed
the
disable-hermes-locally
branch
from
658218e to
d282e93
Compare
siddarthkay
force-pushed
the
disable-hermes-locally
branch
2 times, most recently
from
afd3ec6 to
22b243c
Compare
siddarthkay
changed the title
hermes for debug android buildshermes for debug android builds & enable hermes for release builds
siddarthkay
requested review from
yakimant,
flexsurfer,
ilmotta and
Parveshdhull
siddarthkay
force-pushed
the
disable-hermes-locally
branch
2 times, most recently
from
4e13259 to
06fb04c
Compare
jakubgs
approved these changes
Feb 1, 2024
siddarthkay
force-pushed
the
disable-hermes-locally
branch
from
06fb04c to
de799ae
Compare
ilmotta
approved these changes
Feb 1, 2024
Parveshdhull
approved these changes
Feb 1, 2024
siddarthkay
force-pushed
the
disable-hermes-locally
branch
from
de799ae to
60686ad
Compare
88% of end-end tests have passedFailed tests (4)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Expected to fail tests (2)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
siddarthkay
force-pushed
the
disable-hermes-locally
branch
from
60686ad to
a63446a
Compare
fixes #18493 We enabled `hermes` for android in the `react-native` upgrade to `0.72.5` Although things seemed fine but developers were seeing frequent crashes in their local environment. After some investigation the crashes were traced to max native call stack depth in `hermes` engine. Disabling `hermes` for local debug builds helps fix that issue. This commit disables `hermes` by default with the help of a exporting an environment variable in the `make run-android` command. It is annoying that this also modifies `android/gradle.properties` so we keep `hermesEnabled` as `false` there as well. We also enable `hermes` when generating release builds so that we can take advantage of `hermes` engine in release builds. We also add a log to print whether `hermes` is enabled or not. I think its helpful to have this so that we know whether `hermes` is enabled or not.
siddarthkay
force-pushed
the
disable-hermes-locally
branch
from
a63446a to
a8baeba
Compare
ulisesmac
pushed a commit
that referenced
this pull request
Feb 2, 2024
fixes #18493 We enabled `hermes` for android in the `react-native` upgrade to `0.72.5` Although things seemed fine but developers were seeing frequent crashes in their local environment. After some investigation the crashes were traced to max native call stack depth in `hermes` engine. Disabling `hermes` for local debug builds helps fix that issue. This commit disables `hermes` by default with the help of a exporting an environment variable in the `make run-android` command. It is annoying that this also modifies `android/gradle.properties` so we keep `hermesEnabled` as `false` there as well. We also enable `hermes` when generating release builds so that we can take advantage of `hermes` engine in release builds. We also add a log to print whether `hermes` is enabled or not. I think its helpful to have this so that we know whether `hermes` is enabled or not.
siddarthkay
added a commit
that referenced
this pull request
Feb 6, 2024
We recently disabled `hermes` for debug builds here -> #18675 A side effect of that is when we run `make nix-update-gradle` the `hermes` pom gets removed from `deps.list` This pom is necessary for release builds. Currently that pom exists in `deps.list` but someone may accidentally remove it when running `make nix-update-gradle` In this commit we remove the conditional implementation of `hermes` or `jsc` in `build.gradle` This ensures that the `hermes` pom we need during release builds is not removed from `deps.list`. I also ran `make nix-update-gradle` just to be sure. I compared the apk in this commit to other PRs and the size increased by 2MB. A small price to pay so that the team can run Malli locally without crashing their debug app frequently. I've added a FIXME comment in the code for when we want to cut back on bundle size later.
siddarthkay
added a commit
that referenced
this pull request
Feb 6, 2024
We recently disabled `hermes` for debug builds here -> #18675 A side effect of that is when we run `make nix-update-gradle` the `hermes` pom gets removed from `deps.list` This pom is necessary for release builds. Currently that pom exists in `deps.list` but someone may accidentally remove it when running `make nix-update-gradle` In this commit we remove the conditional implementation of `hermes` or `jsc` in `build.gradle` This ensures that the `hermes` pom we need during release builds is not removed from `deps.list`. I also ran `make nix-update-gradle` just to be sure. I compared the apk in this commit to other PRs and the size increased by 2MB. A small price to pay so that the team can run Malli locally without crashing their debug app frequently. I've added a FIXME comment in the code for when we want to cut back on bundle size later.
siddarthkay
added a commit
that referenced
this pull request
Feb 6, 2024
We recently disabled `hermes` for debug builds here -> #18675 A side effect of that is when we run `make nix-update-gradle` the `hermes` pom gets removed from `deps.list` This pom is necessary for release builds. Currently that pom exists in `deps.list` but someone may accidentally remove it when running `make nix-update-gradle` In this commit we remove the conditional implementation of `hermes` or `jsc` in `build.gradle` This ensures that the `hermes` pom we need during release builds is not removed from `deps.list`. I also ran `make nix-update-gradle` just to be sure. I compared the apk in this commit to other PRs and the size increased by 2MB. A small price to pay so that the team can run Malli locally without crashing their debug app frequently. I've added a FIXME comment in the code for when we want to cut back on bundle size later.
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #18493
Summary
We enabled
hermesfor android in thereact-nativeupgrade to0.72.5Although things seemed fine but developers were seeing frequent crashes in their local environment.
After some investigation the crashes were traced to max native call stack depth in
hermesengine.Disabling
hermesfor local debug builds helps fix that issue.This PR disables
hermesby default with the help of a exporting an environment variable in themake run-androidcommand.It is annoying that this also modifies
android/gradle.propertiesso we keephermesEnabledasfalsethere as well.We also enable
hermeswhen generating release builds so that we can take advantage ofhermesengine in release builds.We also add a log to print whether
hermesis enabled or not. I think its helpful to have this so that we know whetherhermesis enabled or not.Testing notes
Test locally to see if max native call stack error goes away.
Testing notes
no change in release builds,
hermeswas enabled in release builds since the upgrade was merged.Testing is not required.
Platforms
status: ready