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

Provide consumer Proguard rules for Android #1121

Closed
ntsik opened this issue Oct 7, 2020 · 14 comments
Closed

Provide consumer Proguard rules for Android #1121

ntsik opened this issue Oct 7, 2020 · 14 comments

Comments

@ntsik
Copy link

ntsik commented Oct 7, 2020

Android provides a way for libraries to package their own Proguard rules such that users of the library don't need to add rules explicitly. Having consumer rules available simplifies the upgrade process (devs currently need to remember to upgrade the Proguard rules alongside the library version).

@ntsik ntsik added the feature label Oct 7, 2020
@sandwwraith
Copy link
Member

This can't be done fully on library's side, unfortunately — some proguard rules have to be rewritten using an actual package name of the application

@ntsik
Copy link
Author

ntsik commented Oct 9, 2020

I had a feeling that might be an impetus, though I wasn't sure whether there was some way to work around that. Maybe adding an annotation to the generated code that Proguard can target? (I'm not very familiar with the inner workings of the library or Proguard, so not sure how feasible it would actually be.)

@ArcticLampyrid
Copy link

One possible solution

Mark the gen code with specific annotations and add rules to keep all the classes with annotations

@ArcticLampyrid
Copy link

In fact, android defines Keep annotation
Of course, the pre-defined annotation is android-only, we need to define it by ourselves

@ArcticLampyrid
Copy link

My rules:

-keepattributes *Annotation*, InnerClasses
-dontnote kotlinx.serialization.AnnotationsKt
-keepclassmembers @kotlinx.serialization.Serializable class ** {
    *** Companion;
}
-if @kotlinx.serialization.Serializable class **
-keepclassmembers class <1>$Companion {
    kotlinx.serialization.KSerializer serializer(...);
}
-keepclasseswithmembers class **$$serializer {
    *** INSTANCE;
}

See Also #1129

@mxalbert1996
Copy link

I'm against this proposal as I said in #1129:

Obfuscation is important to me and is actually one of the reasons I migrated from Moshi as the Moshi codegen generates ProGuard rules that prevent obfuscation. I don't think there is any ways to exclude part of the rules from libraries, so I would prefer not embedding the rules as they are intrusive (they affect user code and cannot be disabled).

Actually I'm specifying the serializers of all json classes in my project manually so that I don't need any proguard rules.

@ArcticLampyrid
Copy link

Actually I'm specifying the serializers of all json classes in my project manually so that I don't need any proguard rules.

Perhaps we can use a clever compiler plugin to detect serializer staticly in most cases.
Anyway, we need to simplify the consumer's work on Android with obfuscation enabled (default on Android).

@LouisCAD
Copy link
Contributor

Hello, I bumped into runtime crashes because the rules are not bundled.

@sandwwraith most of the rules don't depend on the package of the models, so all these rules could be bundled, which would make the setup much easier as only app-specific rules would need to live in the end codebase.

Even better than that, since @Whathecode edits in #1721, it turns out you'd not need custom proguard/R8 rules at all, unless you have custom named companion objects, which is quite a rare thing AFAIK.

See one of the files provided by kotlinx.coroutines for that:

https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-core/jvm/resources/META-INF/proguard/coroutines.pro

Shall I send a PR adding those files?

@Whathecode
Copy link
Contributor

Note that the reason they weren't bundled yet is:

The following configuration keeps serializers for all serializable classes that are retained after shrinking.
...
In case you want to exclude serializable classes that are used, but never serialized at runtime, you will need to write custom rules with narrower class specifications.

But arguably that's enough of an edge case to simply apply the generic rules by default. You should consider the tradeoff with how difficult it potentially is to remove the default bundled rules (I don't know), and possibly document that as well.

@sandwwraith
Copy link
Member

I've looked at current suggested proguard rules, and it seems that most of them don't include an application package indeed. So yes, embedding rules is possible. And of course, I would be happy to see your PR!

@sandwwraith
Copy link
Member

Also pls take a look at this comment: #2050 (comment)
It seems that full mode needs additional rules

shanshin added a commit that referenced this issue Nov 8, 2022
Resolves #1121
Resolves #1899
Resolves #1900
@LouisCAD
Copy link
Contributor

Is this issue fully resolved now since #2092 has been merged?

@LouisCAD
Copy link
Contributor

Wait, I think we only need for it to be released. Anything preventing it?

@sandwwraith
Copy link
Member

@LouisCAD Yes, it should be available in the next release. We plan it shortly after Kotlin 1.8.0 (there's #2111 to be reviewed, and there's Christmas and New Year, so....)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants