Add hermesFlagsForVariant and deleteDebugFilesForVariant #32281

geraintwhite · 2021-09-28T12:58:48Z

Summary

The hermesFlagsRelease option only works with the release build type, but not with other build types.

This PR allows hermes flags on a per variant basis to be specified using the hermesFlagsForVariant lambda.

It also allows the hermes debugger cleanup to be run on a per variant basis using the deleteDebugFilesForVariant lambda.

Changelog

[Android] [Fixed] - Fix hermesFlags not working with multiple variants

Test Plan

Set the following options in android/app/build.gradle and ensure warnings are hidden when running ./gradlew assembleRelease and ./gradlew assembleLive.

    hermesFlagsForVariant: {
        def v -> v.name.toLowerCase().contains('release') || v.name.toLowerCase().contains('live') ? ['-w'] : []
    },
    deleteDebugFilesForVariant: {
        def v -> v.name.toLowerCase().contains('release') || v.name.toLowerCase().contains('live')
    },

analysis-bot · 2021-09-28T13:27:12Z

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

Base commit: 4790146

analysis-bot · 2021-09-28T13:30:22Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,717,943	+3
android	hermes	armeabi-v7a	7,246,009	+5
android	hermes	x86	8,137,528	-2
android	hermes	x86_64	8,103,016	+8
android	jsc	arm64-v8a	9,637,133	+28
android	jsc	armeabi-v7a	8,552,715	+23
android	jsc	x86	9,650,997	+29
android	jsc	x86_64	10,260,163	+16

Base commit: 4790146

facebook-github-bot · 2021-09-28T14:00:19Z

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

ShikaSD · 2021-09-28T17:21:27Z

Hey, thanks for submitting this change!
From internal discussion with @cortinico:

The preferred way of doing the per-variant setup is to use configuration lambdas, so that we could have more flexibility from the client side and not rely on concatenating strings with variant name and desired configuration. The good example of this idea is enableHermesForVariant parameter, which allows you to select variants to use Hermes at configuration time.
Could you please convert the parameters you introduced to similar lambda parameters? It would also be great to document new parameters in react.gradle 🙂
We are working on a Gradle plugin for the React Native and try to match all functionality from react.gradle there. Could you also apply the same changes to the task setup? You can expose new parameters in the extension similar to the enableHermesForVariant.

geraintwhite · 2021-09-30T07:32:48Z

Since hermesFlagsRelease and hermesFlagsDebug were already used, how would the lambda replace them without breaking existing usages?

ShikaSD · 2021-09-30T12:44:07Z

Since hermesFlagsRelease and hermesFlagsDebug were already used, how would the lambda replace them without breaking existing usages?

Yep, good point. We are thinking about deprecating these flags, so maybe lambda can just override it? Something like:

if (config.hermesFlagsForVariant != null) /* new logic */ else /* old logic */

We don't have a proper variant aware API at the moment, but it is a step into right direction

geraintwhite · 2021-10-12T10:03:11Z

@ShikaSD Sorry for the delay, but I found some time yesterday to convert the parameters into lambdas.

facebook-github-bot · 2021-10-12T11:09:54Z

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

cortinico

Thanks for following up on this. Overall looks good on my end, just left a couple of comments.

cortinico · 2021-10-12T13:53:54Z

packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactExtension.kt

+   * return [hermesFlagsRelease] for Release variants and [hermesFlagsDebug] for Debug variants.
+   */
+  var hermesFlagsForVariant: (BaseVariant) -> List<String> = {
+    variant -> if (variant.name.toLowerCase().contains("release")) hermesFlagsRelease.get() else hermesFlagsDebug.get()


Suggested change

variant -> if (variant.name.toLowerCase().contains("release")) hermesFlagsRelease.get() else hermesFlagsDebug.get()

variant -> if (variant.isRelease) hermesFlagsRelease.get() else hermesFlagsDebug.get()

After making that change the build is failing with Cannot access 'isRelease': it is private in file

See the other comment 👍

cortinico · 2021-10-12T13:54:10Z

packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactExtension.kt

+   * return True for Release variants and False for Debug variants.
+   */
+  var deleteDebugFilesForVariant: (BaseVariant) -> Boolean = {
+    variant -> variant.name.toLowerCase().contains("release")


Suggested change

variant -> variant.name.toLowerCase().contains("release")

variant -> variant.isRelease

After making that change the build is failing with Cannot access 'isRelease': it is private in file

I think it makes sense to make it internal to expose this functionality :)
@cortinico we should consider moving it to some common utils

Yup basically what @ShikaSD mentioned. If you want to export it to a VariantUtil.kt file as well, that would be fine. As long as it's internal all is good 👌

It's passing now after making it internal (the failing check is a connection timeout to jitpack)

cortinico · 2021-10-12T13:56:35Z

react.gradle

@@ -151,6 +173,7 @@ afterEvaluate {

            // Set up dev mode
            def devEnabled = !(config."devDisabledIn${targetName}"
+                || config."devDisabledIn${variant.buildType.name.capitalize()}"


This should not be needed

I will remove it from this PR, but it is added in another PR to fix an issue with devDisabledIn${buildType} not working for productFlavors.

ShikaSD

I think it looks almost good for me, the only thing left is the Variant.isRelease function. Could you make it internal to allow access to it in extension and then use it instead?
Thanks!

pull-bot · 2021-10-13T07:17:56Z

PR build artifact for f79026c is ready.
To use, download tarball from this CircleCI job then run yarn add <path to tarball> in your React Native project.

cortinico

LGTM 👍 Thanks for the follow up

facebook-github-bot · 2021-10-13T19:06:44Z

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

react-native-bot · 2021-10-13T19:55:05Z

This pull request was successfully merged by @grit96 in 91adb76.

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

geraintwhite · 2021-10-14T10:24:08Z

LGTM 👍 Thanks for the follow up

Thanks for merging. Is there any possibility of having a look at the other related PR I mentioned (fixing devDisabledIn for variants)?

ShikaSD · 2021-10-14T10:42:14Z

@grit96 sure, it seems like it will require similar updates though :)

geraintwhite mentioned this pull request Sep 28, 2021

Fix hermesFlagsRelease not working with multiple productFlavors #31040

Closed

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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 28, 2021

ShikaSD self-requested a review September 28, 2021 17:21

react-native-bot added Bug Platform: Android Android applications. labels Oct 1, 2021

geraintwhite force-pushed the fix-hermes-flags-product-flavors branch 2 times, most recently from a658dbd to 6bbc781 Compare October 11, 2021 17:25

geraintwhite changed the title ~~Fix hermesFlagsRelease not working with multiple productFlavors~~ Add hermesFlagsForVariant and deleteDebugFilesForVariant Oct 11, 2021

geraintwhite force-pushed the fix-hermes-flags-product-flavors branch from 6bbc781 to 5e07992 Compare October 11, 2021 17:31

cortinico reviewed Oct 12, 2021

View reviewed changes

geraintwhite force-pushed the fix-hermes-flags-product-flavors branch 2 times, most recently from 5e07992 to 48958d8 Compare October 12, 2021 14:27

ShikaSD approved these changes Oct 12, 2021

View reviewed changes

Add hermesFlagsForVariant and deleteDebugFilesForVariant

f79026c

geraintwhite force-pushed the fix-hermes-flags-product-flavors branch from 747ac5c to f79026c Compare October 13, 2021 07:09

geraintwhite requested a review from cortinico October 13, 2021 08:14

cortinico approved these changes Oct 13, 2021

View reviewed changes

facebook-github-bot closed this in 91adb76 Oct 13, 2021

react-native-bot added the Merged This PR has been merged. label Oct 13, 2021

ShikaSD mentioned this pull request Oct 14, 2021

Fix devDisabledInStaging not working with multiple productFlavors #30606

Closed

1natsu172 mentioned this pull request Feb 26, 2022

[Android] Couldn't find DSO to load crash in release build and hermes is enabled #33120

Closed

This was referenced Mar 14, 2022

Android with RN 0.67.3 crashes when running custom build type with hermes enabled #33177

Closed

couldn't find DSO to load: libjscexecutor.so caused by: dlopen failed: library "libjsc.so" not found #25537

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hermesFlagsForVariant and deleteDebugFilesForVariant #32281

Add hermesFlagsForVariant and deleteDebugFilesForVariant #32281

geraintwhite commented Sep 28, 2021 •

edited

Loading

analysis-bot commented Sep 28, 2021 •

edited

Loading

analysis-bot commented Sep 28, 2021 •

edited

Loading

facebook-github-bot commented Sep 28, 2021

ShikaSD commented Sep 28, 2021 •

edited

Loading

geraintwhite commented Sep 30, 2021

ShikaSD commented Sep 30, 2021

geraintwhite commented Oct 12, 2021

facebook-github-bot commented Oct 12, 2021

cortinico left a comment

cortinico Oct 12, 2021

geraintwhite Oct 12, 2021

cortinico Oct 13, 2021

cortinico Oct 12, 2021

geraintwhite Oct 12, 2021

ShikaSD Oct 12, 2021

cortinico Oct 13, 2021

geraintwhite Oct 13, 2021 •

edited

Loading

cortinico Oct 12, 2021

geraintwhite Oct 12, 2021

ShikaSD left a comment

pull-bot commented Oct 13, 2021

cortinico left a comment

facebook-github-bot commented Oct 13, 2021

react-native-bot commented Oct 13, 2021

geraintwhite commented Oct 14, 2021

ShikaSD commented Oct 14, 2021

	variant -> if (variant.name.toLowerCase().contains("release")) hermesFlagsRelease.get() else hermesFlagsDebug.get()
	variant -> if (variant.isRelease) hermesFlagsRelease.get() else hermesFlagsDebug.get()

	variant -> variant.name.toLowerCase().contains("release")
	variant -> variant.isRelease

Add hermesFlagsForVariant and deleteDebugFilesForVariant #32281

Add hermesFlagsForVariant and deleteDebugFilesForVariant #32281

Conversation

geraintwhite commented Sep 28, 2021 • edited Loading

Summary

Changelog

Test Plan

analysis-bot commented Sep 28, 2021 • edited Loading

analysis-bot commented Sep 28, 2021 • edited Loading

facebook-github-bot commented Sep 28, 2021

ShikaSD commented Sep 28, 2021 • edited Loading

geraintwhite commented Sep 30, 2021

ShikaSD commented Sep 30, 2021

geraintwhite commented Oct 12, 2021

facebook-github-bot commented Oct 12, 2021

cortinico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geraintwhite Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShikaSD left a comment

Choose a reason for hiding this comment

pull-bot commented Oct 13, 2021

cortinico left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 13, 2021

react-native-bot commented Oct 13, 2021

geraintwhite commented Oct 14, 2021

ShikaSD commented Oct 14, 2021

geraintwhite commented Sep 28, 2021 •

edited

Loading

analysis-bot commented Sep 28, 2021 •

edited

Loading

analysis-bot commented Sep 28, 2021 •

edited

Loading

ShikaSD commented Sep 28, 2021 •

edited

Loading

geraintwhite Oct 13, 2021 •

edited

Loading