-
Notifications
You must be signed in to change notification settings - Fork 4
[FSSDK-11888] fix: update ProGuard rules for AGP 8+ and R8 compatibility #86
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
Conversation
- Update proguard configuration in build.gradle - Add missing dependencies for ProGuard rules - Update Android multidex support version in dependencies in app/build.gradle
- Add workflow for testing minification compatibility - Set up Flutter and Java environments - Install dependencies and create ProGuard rules - Test with minification enabled and disabled - Run unit tests and verify APK artifacts - Check for ProGuard/R8 artifacts and upload APKs - Report test results and clean up temporary files
- Update 'actions/upload-artifact' action version from v3 to v4 for workflow optimization and compatibility.
- Implement a CI workflow for testing minification compatibility - Include job for testing with minification enabled and disabled - Add steps for verifying APK artifacts and ProGuard files - Include uploading APK artifacts and reporting test results
- Update Gradle JVM and Android build options - Ensure minification is enabled - Build for single architecture to optimize memory - Refactor script for APK artifact verification - Simplify test execution line - Improve output messages for various tasks
- Update the Java version in the setup to '11' in minification workflow
…iguration - Update release build configuration in build.gradle - Remove minifyEnabled and shrinkResources settings from release build configuration
- Update Java version in the minification workflow from 11 to 17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea to have minify tests! A few more clarifications.
# Keep example app classes | ||
-keep class com.optimizely.optimizely_flutter_sdk_example.** { *; } | ||
# Keep Flutter classes | ||
-keep class io.flutter.app.** { *; } | ||
-keep class io.flutter.plugins.GeneratedPluginRegistrant { *; } | ||
# Google Play Core (for Flutter engine) | ||
-keep class com.google.android.play.core.** { *; } | ||
-dontwarn com.google.android.play.core.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we put all these in example-app proguard-rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is added into example app proguard rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once added into example proguard, we can remove these from here to simplify?
minifyEnabled true | ||
proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro' | ||
minifyEnabled true | ||
proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.txt', 'proguard-rules.pro' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change? I understand proguard-rules.pro
is new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We added 'proguard-rules.txt' here to apply in the release build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can drop "proguard-rules.pro" here?
android/proguard-rules.txt
Outdated
# Keep Jackson classes for JSON parsing | ||
-keep class com.fasterxml.jackson.** { *; } | ||
-dontwarn com.fasterxml.jackson.** | ||
|
||
# Keep Guava classes | ||
-keep class com.google.common.** { *; } | ||
-dontwarn com.google.common.** | ||
-dontwarn com.google.android.play.core.** | ||
|
||
# Keep SLF4J and Logback classes | ||
-keep class org.slf4j.** { *; } | ||
-keep class ch.qos.logback.** { *; } | ||
-dontwarn org.slf4j.** | ||
-dontwarn ch.qos.logback.** | ||
|
||
# Missing Dependencies (Android doesn't have these) | ||
-dontwarn javax.mail.** | ||
-dontwarn javax.activation.** | ||
-dontwarn javax.servlet.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these - not used in our flutter sdk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the list.
.github/workflows/minification.yml
Outdated
echo "ℹ️ No mapping file found" | ||
fi | ||
- name: Upload APK artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obfuscation can cause app crash in runtime, even if building is fine. We may need to run tests after buidling.
No need to include in this fix, but we can consider it later.
- Remove additional safety rules for Optimizely - Update APK artifacts upload path for minification test - Remove unnecessary Guava classes and warnings - Adjust SLF4J and Logback rules for ProGuard - Remove warnings for missing dependencies in ProGuard configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more suggestions.
architecture: x64 | ||
- run: flutter pub get | ||
- run: flutter test | ||
- run: flutter test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - a new line
# Keep example app classes | ||
-keep class com.optimizely.optimizely_flutter_sdk_example.** { *; } | ||
# Keep Flutter classes | ||
-keep class io.flutter.app.** { *; } | ||
-keep class io.flutter.plugins.GeneratedPluginRegistrant { *; } | ||
# Google Play Core (for Flutter engine) | ||
-keep class com.google.android.play.core.** { *; } | ||
-dontwarn com.google.android.play.core.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once added into example proguard, we can remove these from here to simplify?
if [ -f "example/android/app/build.gradle.backup" ]; then | ||
mv example/android/app/build.gradle.backup example/android/app/build.gradle | ||
echo "✅ Restored original build.gradle" | ||
fi No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - new line
minifyEnabled true | ||
proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro' | ||
minifyEnabled true | ||
proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.txt', 'proguard-rules.pro' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can drop "proguard-rules.pro" here?
# Keep Jackson classes for JSON parsing | ||
-keep class com.fasterxml.jackson.** { *; } | ||
-dontwarn com.fasterxml.jackson.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why Jackson here? Do we use Jackson in our sdk plugin?
Summary
Test plan
Issues