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

Embed proguard/R8 rules in kotlin-reflect artifact jar #2893

Closed

Conversation

ZacSweers
Copy link
Contributor

@ZacSweers ZacSweers commented Dec 18, 2019

The Android build pipeline can extract embedded proguard configurations from dependencies and merge them automatically. This adds a conservative proguard configuration to the kotlin-reflect JVM artifact in support of that. This focuses mostly on just retaining what's necessary for kotlin-reflect's own functionality to operate, but could be expanded if community feedback discovers other good candidate rules.

With this in place - most Android projects using R8 or Proguard should Just Work™️ with kotlin-reflect.

@ZacSweers
Copy link
Contributor Author

ZacSweers commented Dec 18, 2019

The Android build pipeline can extract embedded proguard configurations from dependencies and merge them automatically. This adds a conservative proguard configuration to the kotlin-reflect JVM artifact in support of that. This focuses mostly on just retaining what's necessary for kotlin-reflect's own functionality to operate, but could be expanded if community feedback discovers other good candidate rules.

With this in place - most Android projects using R8 or Proguard should Just Work™️ with kotlin-reflect.
@ZacSweers ZacSweers force-pushed the z/kotlinreflectProguardRules branch from 4c3c64a to 7bb0f84 Compare Dec 18, 2019
@@ -0,0 +1,14 @@
##---------------Begin: Kotlin-reflect ----------
# Keep Metadata annotations so they can be parsed at runtime.
-keep class kotlin.Metadata { *; }
Copy link
Contributor

@jsjeon jsjeon Dec 18, 2019

Choose a reason for hiding this comment

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

I wonder this is not actually needed. Recently, I started working on synthesizing metadata from scratch (for R8-processed Kotlin library), and kept adding more tests. During that, I found that, even without this rule, R8 will still keep metadata (annotation) associated to classes.

More conversion/details are here: https://r8-review.googlesource.com/c/r8/+/46240/1/src/test/java/com/android/tools/r8/kotlin/metadata/MetadataRenameInParametertypeTest.java#70

Copy link
Contributor

@jsjeon jsjeon Dec 18, 2019

Choose a reason for hiding this comment

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

Ah, one caveat is, though, R8 with this fix (https://r8-review.googlesource.com/c/r8/+/44220) will keep metadata annotation without definition. For prior versions of R8, it may be the case that we still need this rule.

Copy link
Contributor Author

@ZacSweers ZacSweers Dec 23, 2019

Choose a reason for hiding this comment

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

Yeah I think it's best to keep this as a conservative rule. We've had reports in Moshi that Metadata is stripped in android builds without a custom keep rule for some users, though unsure what version they're using or if they're just using proguard.

Copy link
Contributor Author

@ZacSweers ZacSweers Jan 6, 2020

Choose a reason for hiding this comment

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

@jsjeon what version of R8 would that fix be in? I'm not sure how to check the latest release history or what's in them off-hand there

Copy link
Contributor

@jsjeon jsjeon Jan 8, 2020

Choose a reason for hiding this comment

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

Ah, it's even difficult for me to check which version includes it. :\ Based on the timeline, I guess AGP 3.6 will include that.

However, that fix I mentioned was about keeping annotations associated with classes/members if the definition of annotations is missing. Therefore, if annotation class Metadata is given (via kotlin reflect artifact jar of course), i.e., not missing, it is still the case that R8 will take it away if it thinks it's not used.


I was about to mention the case like https://issuetracker.google.com/issues/146534384 where kotlin.Metadata itself is retrieved at runtime. In that case, yeah, this rule is necessary.

Copy link
Contributor Author

@ZacSweers ZacSweers Jan 14, 2020

Choose a reason for hiding this comment

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

Sounds like it's safest to just keep this for now


# Keep generic signatures and annotations at runtime.
# R8 requires InnerClasses and EnclosingMethod if you keepattributes Signature.
-keepattributes InnerClasses,Signature,*Annotation*,EnclosingMethod
Copy link
Contributor

@jsjeon jsjeon Dec 18, 2019

Choose a reason for hiding this comment

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

Do we need broad *Annotation* or just RuntimeVisibleAnnotation (because metadata is runtime visible) ?

Copy link
Contributor Author

@ZacSweers ZacSweers Dec 23, 2019

Choose a reason for hiding this comment

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

I didn't know that RuntimeVisibleAnnotations was a thing! Historically any project reading annotations at runtime has always used *Annotation*, but maybe that's just a bespoke pattern that isn't needed. Let me try in our codebase

Copy link
Member

@udalov udalov Jan 3, 2020

Choose a reason for hiding this comment

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

There are also at least RuntimeVisibleParameterAnnotations and RuntimeVisibleTypeAnnotations, so maybe the pattern should be RuntimeVisible*Annotations, if that works...

Copy link
Contributor Author

@ZacSweers ZacSweers Jan 6, 2020

Choose a reason for hiding this comment

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

That seems to work b843558

-keep class kotlin.Metadata { *; }

# Keep kotlin-reflect internals.
-keep class kotlin.reflect.jvm.** { *; }
Copy link
Contributor

@jsjeon jsjeon Dec 18, 2019

Choose a reason for hiding this comment

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

This seems way too broad. At least, we can try public APIs only.

Copy link
Contributor Author

@ZacSweers ZacSweers Dec 23, 2019

Choose a reason for hiding this comment

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

Let me try it in our project. To be totally honest I'm not convinced this is truly needed, but it's frequently suggested as a fix in the community. The last comprehensive list we were using internally was based on this:

square/moshi#883

Though all of those are public classes as well, so it would work. @udalov any advice on what scope should be kept?

Copy link
Member

@udalov udalov Jan 3, 2020

Choose a reason for hiding this comment

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

The list from the issue mentioned above is actually the combination of all services and implementations used in kotlin-reflect -- see META-INF/services in kotlin-reflect.jar. Shouldn't they be handled correctly in some other way by Proguard/R8? E.g. is it possible to "keep" the directory META-INF/services, and everything it references, to avoid enumerating classes used there?

Other than that, we do need to fully keep the contents of packages kotlin.reflect.full and kotlin.reflect.jvm, but not their subpackages (i.e. kotlin.reflect.jvm.internal).

Copy link
Contributor

@JakeWharton JakeWharton Jan 3, 2020

Choose a reason for hiding this comment

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

Types declared inside META-INF/services/ are handled automatically provided that the code which calls into ServiceLoader with that service type is not being removed. You shouldn't need to do anything for them.

Copy link
Contributor Author

@ZacSweers ZacSweers Jan 6, 2020

Choose a reason for hiding this comment

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

They're handled automatically by R8, but not proguard iirc. Can we have rules that are only picked up by one or the other?

Copy link
Contributor

@JakeWharton JakeWharton Jan 6, 2020

Choose a reason for hiding this comment

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

Yes. See kotlinx.coroutines for an example that targets R8 1.6 and newer only.

Copy link
Contributor Author

@ZacSweers ZacSweers Jan 6, 2020

Choose a reason for hiding this comment

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

Done in 08616fb

Copy link
Contributor Author

@ZacSweers ZacSweers Jan 6, 2020

Choose a reason for hiding this comment

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

Also tweaked to just keep service-loaded APIs, only in proguard or R8 <1.6 c15aac4

Copy link
Contributor

@jsjeon jsjeon Jan 8, 2020

Choose a reason for hiding this comment

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

Thanks for taking care of this thoroughly! Those specific rules that aim for only interface and services look great!

@@ -0,0 +1,14 @@
##---------------Begin: Kotlin-reflect ----------
Copy link
Member

@udalov udalov Jan 3, 2020

Choose a reason for hiding this comment

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

Are these Begin/End tags required?

Also, please consider renaming the file itself to kotlin-reflect.pro

Copy link
Contributor

@JakeWharton JakeWharton Jan 3, 2020

Choose a reason for hiding this comment

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

Are these Begin/End tags required?

Not needed. They will be added automatically by R8 now for blaming purposes.

https://issuetracker.google.com/issues/134372926

Copy link
Contributor Author

@ZacSweers ZacSweers Jan 6, 2020

Choose a reason for hiding this comment

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

By R8 yes. Do we care about it in a proguard context? They're not strictly required but the blaming purpose is nice

Copy link
Contributor Author

@ZacSweers ZacSweers Jan 6, 2020

Choose a reason for hiding this comment

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

Removed in c6f922b

Copy link
Contributor Author

@ZacSweers ZacSweers left a comment

Took a significant pass at all the feedback and updated. The structure is now

/META-INF
  /com.android.tools
    /proguard/kotlin-reflect.pro       // Proguard-only, with custom serviceloader rules
    /r8-from-1.6.0/kotlin-reflect.pro  // R8 1.6+ with serviceloader handling
    /r8-upto-1.6.0/kotlin-reflect.pro  // R8 <1.6 with customservice loader rules
  /proguard/kotlin-reflect.pro         // Default proguard rules location, with custom serviceloader rules

Let me know what you think

-keep class kotlin.Metadata { *; }

# Keep implementations of service loaded interfaces
# R8 will automatically handle these these in 1.6+
Copy link
Contributor

@jsjeon jsjeon Jan 8, 2020

Choose a reason for hiding this comment

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

Nit: this file is not quite related to R8. :) (and below too)

Copy link
Contributor Author

@ZacSweers ZacSweers Jan 14, 2020

Choose a reason for hiding this comment

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

D'oh, fixed in 5091302

@ZacSweers
Copy link
Contributor Author

ZacSweers commented Jan 14, 2020

@udalov anything else needed for this PR?

@udalov
Copy link
Member

udalov commented Jan 17, 2020

@ZacSweers Thank you for the contribution!

Merged manually in 482874f. The changes will be available in 1.4-M1.

@udalov udalov closed this Jan 17, 2020
@ZacSweers
Copy link
Contributor Author

ZacSweers commented Jan 17, 2020

Thanks!

@ColtonIdle
Copy link

ColtonIdle commented Apr 20, 2021

@udalov any chance something changed here in 1.4.32?

We noticed this issue happening but it wasn't happening in the past (we've been using proguard since kotlin 1.4) but with 1.4.32 we need to manually add

-keep class kotlin.reflect.jvm.internal.** { *; }

for our app not to crash.

Edit: Filed on youtrack here as there was no github issue tab. Hope that's the right place. https://youtrack.jetbrains.com/issue/KT-46236

@ZacSweers
Copy link
Contributor Author

ZacSweers commented Apr 20, 2021

File an issue with repro steps, commenting on an old PR isn't really the best venue

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