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

Add default proguard config for sample app #138

Merged

Conversation

rossbacher
Copy link
Collaborator

Update AGP to 3.2.0
Update Kotlin to 1.2.51
Include consumer proguard file in every variant
Add default proguard config for sample app
Run Proguard on release build type for sample app

Fixes: #126

Update Kotline to 1.2.51
Include consumer proguard file in every variant
Add default proguard config for sample app
Run Proguard on release build type for sample app
@rossbacher rossbacher added the enhancement New feature or request label Nov 15, 2018
@rossbacher rossbacher requested a review from gpeal November 15, 2018 00:58
Copy link
Contributor

@bernaferrari bernaferrari left a comment

Choose a reason for hiding this comment

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

Nice work! That Proguard file has some really weird things. I commented on some.

sample/proguard-project.pro Show resolved Hide resolved
sample/proguard-project.pro Outdated Show resolved Hide resolved
sample/proguard-project.pro Show resolved Hide resolved
mvrx/build.gradle Show resolved Hide resolved
@bernaferrari
Copy link
Contributor

A few more suggestions: you named proguard-project.pro instead of proguard-rules.pro. Also, I counted -dontwarn javax.annotation.** there 3 times.

@rossbacher
Copy link
Collaborator Author

@bernaferrari This is on purpose, I want to make clear that these are project specific rules. The name of the file is not really a problem.

-dontwarn javax.annotation.** is in there multiple times as it is in the different configs I had to copy for okhttp3, moshi and retrofit2. These configs are unchanged on purpose. It is ok to have duplicative config.

@bernaferrari
Copy link
Contributor

Fine. I tried the new configurations on my app that crashes with MvRx. I was able to make it stop crashing with the following configuration:

# Print out the full proguard config used for every build
-printconfiguration build/outputs/fullProguardConfig.pro

# These classes are used via kotlin reflection and the keep might not be required anymore once Progurad supports
# Kotlin reflection directlty.
-keep class kotlin.reflect.jvm.internal.impl.builtins.BuiltInsLoaderImpl
-keep class kotlin.reflect.jvm.internal.impl.load.java.FieldOverridabilityCondition
-keep class kotlin.reflect.jvm.internal.impl.load.java.ErasedOverridabilityCondition
-keep class kotlin.reflect.jvm.internal.impl.load.java.JavaIncompatibilityRulesOverridabilityCondition

# If Companion objects are instantiated via Kotlin reflection and they extend/implement a class that Proguard
# would have removed or inlined we run into trouble as the inheritance is still in the Metadata annoation
# read by Kotlin reflection.
# FIXME Remove if Kotlin reflection is supported by Pro/Dexguard
-if class **$Companion extends **
-keep class <2>
-if class **$Companion implements **
-keep class <2>

# https://medium.com/@AthorNZ/kotlin-metadata-jackson-and-proguard-f64f51e5ed32
-keep class kotlin.Metadata { *; }

# Oddly need to keep that even though Evernote state is not used in the app.
-keepnames class * { @com.evernote.android.state.State *;}

I suggest adding it to the library, since everyone using mvrx+epoxy+proguard will get a crash without it (probably because of reflection), which is not fun.

@rossbacher
Copy link
Collaborator Author

I disagree. The config should be very topical and only contain keep rules for classes in the lib (not for anything it depends on).

This is why this is part of the demo app, but it should not be part of the lib itself.

@bernaferrari
Copy link
Contributor

I think you should at least add on wiki that mvrx crashes without some proguard rules, else this will catch a lot of people on surprise.

@rossbacher
Copy link
Collaborator Author

Well the rules are now in the demo app. But I agree, let me update the README...

@bernaferrari
Copy link
Contributor

bernaferrari commented Nov 17, 2018 via email

Updated proguard file to distinguish between Kotlin/Rx and lib rules
@rossbacher
Copy link
Collaborator Author

I also updated the Wiki with a Proguard/Dexguard section here: https://github.com/airbnb/MvRx/wiki#proguarddexguard

@rossbacher
Copy link
Collaborator Author

I think you should at least add on wiki that mvrx crashes without some proguard rules, else this will catch a lot of people on surprise.

Done: https://github.com/airbnb/MvRx/wiki#proguarddexguard

@rossbacher
Copy link
Collaborator Author

I also updated the Wiki with a Proguard/Dexguard section here: https://github.com/airbnb/MvRx/wiki#proguarddexguard

Done.

@bernaferrari
Copy link
Contributor

Great, awesome, thanks!

@rossbacher rossbacher merged commit b996653 into airbnb:master Dec 7, 2018
@rossbacher rossbacher deleted the addDefaultProguardConfigForSample branch December 7, 2018 21:56
gpeal pushed a commit that referenced this pull request Jan 14, 2019
…ialState (#181)

#169 Added a new initialState method and ability to not use @JvmStatic all of these need keep rules.

Closes #176

** Note: Kotlin 1.3+ requires this keep rule as well:

keep class kotlin.reflect.jvm.internal.impl.serialization.deserialization.builtins.BuiltInsLoaderImpl

Per the discussion in #138, this is not a project specific rule, so not including it in the proguard file, but updated wiki to suggest it.

Tested on sample app.

Created #182 to prevent this from the future. Would be a great first task if anyone from the community wants to jump on it :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants