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

More specific Proguard rules #217

Merged
merged 3 commits into from
Sep 21, 2018
Merged

More specific Proguard rules #217

merged 3 commits into from
Sep 21, 2018

Conversation

electrostat
Copy link
Contributor

Update proguard rules to be more specific and lower method counts. We utilize common and location within the Core library and will keep only those.

fixes #199

- make proguard rules more specific
- target location and core, which we use within CORE
@electrostat electrostat added this to the Sprint-9/21 milestone Sep 19, 2018
@electrostat electrostat self-assigned this Sep 19, 2018
@@ -1,5 +1,6 @@
# Consumer proguard rules for libcore

# --- GMS ---
-keep class com.google.android.gms.** { *; }
-keep class com.google.android.gms.location.** { *; }
-keep class com.google.android.gms.common.** { *; }
Copy link
Member

Choose a reason for hiding this comment

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

what is the reason for keeping these classes? Are you using reflection on these or any other reason why they should be kept?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per @LukasPaczos comment on the original PR that started this Proguard change (#81 (comment)). This is necessary to prevent warnings in the maps sdk when building for release.

Copy link
Member

Choose a reason for hiding this comment

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

True. However, we should only keep the required ones, or maybe we can even dontwarn some of those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could remove the keep and then keep the dontwarn. Does that implementation work for you two, without having any negative effects on the maps sdk?

Copy link
Member

Choose a reason for hiding this comment

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

Refs https://developers.google.com/android/guides/setup:

Note: ProGuard directives are included in the Play services client libraries to preserve the required classes. The Android Plugin for Gradle automatically appends ProGuard configuration files in an AAR (Android ARchive) package and appends that package to your ProGuard configuration. During project creation, Android Studio automatically creates the ProGuard configuration files and build.gradle properties for ProGuard use. To use ProGuard with Android Studio, you must enable the ProGuard setting in your build.gradle buildTypes. For more information, see the ProGuard guide.

What I understand from there is that if you are not referencing any of those compoenents via reflection, we should be good with using only dontwarn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect. I don't. I'll make the change.

- remove location and common keep rules, since we are not using reflection
@electrostat electrostat merged commit 4e4f09f into master Sep 21, 2018
@electrostat electrostat deleted the aa-proguard-targeting branch September 21, 2018 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Too generic proguard keep rule
4 participants