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

[linear-gradient] fix crash when r8 enabled #21580

Merged
merged 3 commits into from
Mar 8, 2023
Merged

[linear-gradient] fix crash when r8 enabled #21580

merged 3 commits into from
Mar 8, 2023

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Mar 7, 2023

Why

fixes #21562

How

the code here will be stripped by R8:

public LinearGradientView(Context context) {
super(context);
}
and then our ViewDefinitionBuilder cannot find the primary constructor. this pr adds the keep annotation.

i didn't find a way to fix it systematically from expo-modules-core. let's address case by case and expo-linear-gradient first.

Test Plan

https://github.com/GaelCO/repro_linearGradient with patch-package for the fix

Checklist

@expo-bot
Copy link
Collaborator

expo-bot commented Mar 7, 2023

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider adding an appropriate entry to the following changelogs:


Generated by ExpoBot 🤖 against 6d4f4f1

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Mar 7, 2023
@Kudo Kudo marked this pull request as ready for review March 7, 2023 15:34
@Kudo Kudo requested a review from brentvatne as a code owner March 7, 2023 15:34
@Kudo Kudo requested review from lukmccall and removed request for brentvatne March 7, 2023 15:34
@@ -22,6 +24,8 @@ public class LinearGradientView extends View {
private int[] mSize = {0, 0};
private float[] mBorderRadii = {0, 0, 0, 0, 0, 0, 0, 0};

// Keeps this primary constructor from Proguard/R8 for ViewDefinitionBuilder
@Keep
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any difference between @keep and @DoNotStrip?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! though @keep is the default androidx annotation and R8 has default rules to keep it, using @DoNotStrip is more aligned to our code base.

@Kudo Kudo merged commit 36421ad into main Mar 8, 2023
@Kudo Kudo deleted the @kudo/fix-21562 branch March 8, 2023 01:30
Kudo added a commit that referenced this pull request Mar 8, 2023
# Why

fixes #21562

# How

the code here will be stripped by R8: https://github.com/expo/expo/blob/4d234bbfb32b66b3944a0e652eda1e6c67287059/packages/expo-linear-gradient/android/src/main/java/expo/modules/lineargradient/LinearGradientView.java#L25-L27 and then our `ViewDefinitionBuilder` cannot find the primary constructor. this pr adds the keep annotation.

i didn't find a way to fix it systematically from expo-modules-core. let's address case by case and expo-linear-gradient first.

# Test Plan

https://github.com/GaelCO/repro_linearGradient with patch-package for the fix
(cherry picked from commit 36421ad)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: suggestions ExpoBot has some suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[expo-linear-gradient] android - crash when enable Proguard
3 participants