Add R8/ProGuard rules to the codegen artifact by JakeWharton · Pull Request #143 · JakeWharton/dagger-reflect

JakeWharton · 2019-07-14T21:43:41Z

Also process the codegen test code with both R8 and ProGuard to ensure they work correctly in practice.

TWiStErRob · 2019-07-15T00:50:06Z

codegen/src/main/resources/META-INF/proguard/dagger-reflect.pro

@@ -0,0 +1,49 @@
+# When using factories or builders, the enclosing class is used to determine the component type.


I don't see a mention of @Binds which I think may be a special case as it's not referenced by generated Dagger component code. Is it covered by one of these rules?

These rules are for the codegen artifact reflection, not the reflection in the reflect artifact.

I'm not convinced anyone will be using shrinking with the reflect artifact. I'd be curious what their use case for this was.

Oh, good point, sorry.

Re shrinking with reflect: we sometimes minifyEnabled=true on debug flavor so we can test shrinking-related issues. If dagger-reflect was set up it with if (idea) this quick-change-for-debugging would fail the build unexpectedly because of reflect.

In that case, though, having dagger-reflect in your APK would already produce results that were not representative of what happens when shrinking in release. In release builds a large portion of Dagger-generated types are vertically merge and/or inlined. I've even written some non-trivial graphs that have been completely eliminated by shrinking leaving only a bunch of chained calls to constructors.

I'm not opposed to adding rules to the reflect artifact. They're probably pretty basic. But I think the concept of shrinking and using reflect are mutually exclusive in their goals so it's not a high priority.

What I was referring to here is adding minifyEnabled=true to debug so that we can verify things not related to Dagger, or dagger-reflect. But at the moment if the debug build is set up with dagger-reflect, it prevents debugging other problems., because injection crashes. I see why it's not high priority though, as it shouldn't be hard to turn off dagger-reflect, it's just inconvenient.

codegen/src/test/rules.pro

Laimiux · 2019-07-15T18:20:01Z

The ruleset crashed on my branch using the activity in integration-tests:android. https://github.com/Laimiux/dagger-reflect/tree/laimonas/r8_rule_test.

To replicate the issue:

I've replaced proguard-rules.pro with the new rules in this PR
Ran ./gradlew :integration-tests:android:installCodegenDebug

The current broader ruleset works.

JakeWharton · 2019-07-18T14:27:35Z

Thanks. I'll look soon. Although I'd be happy to land this as-is and fix that separately as a bug.

Laimiux · 2019-07-18T17:22:09Z

Using this ruleset even basic use of AppComponent crashes. I suspect that the R8 doesn't run correctly during the test you have written because that exact use case crashes for me.

Laimiux · 2019-07-18T17:24:30Z

I might be misunderstanding how it works, do we need to mark components with @Keep?

JakeWharton · 2019-07-18T17:24:52Z

When I wrote them I was inspecting the classfile output of both tools and both looked correct.

…

On Thu, Jul 18, 2019 at 1:22 PM Laimonas Turauskas ***@***.***> wrote: Using this ruleset even basic use of AppComponent crashes. I suspect that the R8 doesn't run correctly during the test you have written because that exact use case crashes for me. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#143?email_source=notifications&email_token=AAAQIEIPJPSV5GZJJVRWF4LQACREDA5CNFSM4IDSBLTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2JGBRA#issuecomment-512909508>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQIEK2NTOEMFMZISL4CALQACREDANCNFSM4IDSBLTA> .

Laimiux · 2019-07-18T18:26:10Z

Any ideas of what could be the cause for the crash?

JakeWharton · 2019-07-18T18:29:50Z

I haven't had time to look at it yet so I don't even know what's crashing.

…

On Thu, Jul 18, 2019 at 2:26 PM Laimonas Turauskas ***@***.***> wrote: Any ideas of what could be the cause for the crash? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#143?email_source=notifications&email_token=AAAQIEKL4XJOIKKPJR5XFGTQACYUFA5CNFSM4IDSBLTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2JLUDQ#issuecomment-512932366>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQIEMCOAVEK5PUBP2STP3QACYUFANCNFSM4IDSBLTA> .

TWiStErRob · 2019-08-03T02:04:24Z

@JakeWharton Played around a bit, but couldn't make it work.

In the current state applying the proguard rules, DaggerAppComponent is not kept:

// integration-tests/android/build.gradle
  buildTypes {
    debug {
      minifyEnabled true
      shrinkResources minifyEnabled
    }
  }

     at android.app.ActivityThread.handleBindApplication(ActivityThread.java:5876)
        at android.app.ActivityThread.access$1100(ActivityThread.java:199)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1650)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:193)
        at android.app.ActivityThread.main(ActivityThread.java:6669)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)
     Caused by: java.lang.IllegalStateException: Unable to find generated component implementation com.example.DaggerAppComponent for component com.example.AppComponent
        at a.b.a.c(:61)
        at a.b.a.a(:25)
        at a.a.a(:22)
        at com.example.ExampleApp.onCreate(:14)
        at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1154)
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:5871)
        at android.app.ActivityThread.access$1100(ActivityThread.java:199) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1650) 
        at android.os.Handler.dispatchMessage(Handler.java:106) 
        at android.os.Looper.loop(Looper.java:193) 
        at android.app.ActivityThread.main(ActivityThread.java:6669) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858) 
     Caused by: java.lang.ClassNotFoundException: Didn't find class "com.example.DaggerAppComponent" on path: DexPathList[[zip file "/data/app/com.example-cfNVrdQea-4kuqzGxdZGhg==/base.apk"],nativeLibraryDirectories=[/data/app/com.example-cfNVrdQea-4kuqzGxdZGhg==/lib/x86_64, /system/lib64]]
        at dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:134)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:379)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:312)
        at a.b.a.c(:54)
        at a.b.a.a(:25) 
        at a.a.a(:22) 
        at com.example.ExampleApp.onCreate(:14) 
        at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1154) 
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:5871) 
        at android.app.ActivityThread.access$1100(ActivityThread.java:199) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1650) 
        at android.os.Handler.dispatchMessage(Handler.java:106) 
        at android.os.Looper.loop(Looper.java:193) 
        at android.app.ActivityThread.main(ActivityThread.java:6669) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

The rule looks correct, but doesn't keep:

-whyareyoukeeping class com.example.AppComponent
-whyareyoukeeping class com.example.DaggerAppComponent

android.enableR8=true

com.example.AppComponent
|- is referenced in keep rule:
|  <PROGUARD_COMPATIBILITY_RULE>:Unknown
Nothing is keeping com.example.DaggerAppComponent

android.enableR8=true
android.enableR8.fullMode=true

Nothing is keeping com.example.AppComponent
Nothing is keeping com.example.DaggerAppComponent

android.enableR8=false

com.example.AppComponent
  is invoked by    com.example.DaggerAppComponent.create (32:32)
  is kept by a directive in the configuration.

com.example.DaggerAppComponent
  is kept by a directive in the configuration.

# but still crashes with java.lang.ClassNotFoundException: Didn't find class "com.example.Daggera"

Manually adding the rules, everything works with all variants of shrinking:

-keepnames class com.example.AppComponent
-keep class com.example.DaggerAppComponent {
  static ** create();
}

Whenever I looked in the APK classes.dex, it matched the whyareyoukeeping output.

mochadwi · 2020-04-08T20:10:10Z

Manually adding the rules, everything works with all variants of shrinking:
-keepnames class com.example.AppComponent
-keep class com.example.DaggerAppComponent {
  static ** create();
}
Whenever I looked in the APK classes.dex, it matched the whyareyoukeeping output.

You stated works and matched @TWiStErRob, did the point 3 & 4 solved the crashing issue?

TWiStErRob · 2020-04-08T23:43:15Z

Uuuuh, @mochadwi I have no memory of doing this investigation :D, but here are my thoughts:
3. works: I said "everything works", from me that usually means app works as expected, no warnings, no errors, no crashes, just works.
4. matched: I validated whether ProGuard/r8's -whyareyoukeeping log output matches what's actually bundled in the classes.dex inside the minified APK.

The whole point of the comment is validating @Laimiux's claim that in the current state it crashes, and trying to find out why to help @JakeWharton, which is: the keeps to achieve what's in 3. is missing (or handled differently in all 3 shrinking modes as seen in 2.) from the generic rules in this PR.

So of course, manually keeping your @Components and their reflectively accessed generated implementations works as expected, but the goal of this PR is to make it work in a general case (only 1.) without having to add our entry points manually (3.). to our ProGuard rules.

mochadwi · 2020-04-13T18:18:49Z

Uuuuh, @mochadwi I have no memory of doing this investigation :D, but here are my thoughts:
3. works: I said "everything works", from me that usually means app works as expected, no warnings, no errors, no crashes, just works.
4. matched: I validated whether ProGuard/r8's -whyareyoukeeping log output matches what's actually bundled in the classes.dex inside the minified APK.

The whole point of the comment is validating @Laimiux's claim that in the current state it crashes, and trying to find out why to help @JakeWharton, which is: the keeps to achieve what's in 3. is missing (or handled differently in all 3 shrinking modes as seen in 2.) from the generic rules in this PR.

So of course, manually keeping your @Components and their reflectively accessed generated implementations work as expected, but the goal of this PR is to make it work in a general case (only 1.) without having to add our entry points manually (3.). to our ProGuard rules.

Nice, I really appreciate it for your answer. That's explained everything about this issue still open. @TWiStErRob

Add R8/ProGuard rules to the codegen artifact

bd12a10

Also process the codegen test code with both R8 and ProGuard to ensure they work correctly in practice.

TWiStErRob reviewed Jul 15, 2019

View reviewed changes

JakeWharton commented Jul 15, 2019

View reviewed changes

codegen/src/test/rules.pro Outdated Show resolved Hide resolved

Update codegen/src/test/rules.pro

b61a55c

		@@ -0,0 +1,49 @@
		# When using factories or builders, the enclosing class is used to determine the component type.

Conversation

JakeWharton commented Jul 14, 2019

Uh oh!

TWiStErRob Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

JakeWharton Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

TWiStErRob Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

JakeWharton Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

TWiStErRob Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Laimiux commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakeWharton commented Jul 18, 2019

Uh oh!

Laimiux commented Jul 18, 2019

Uh oh!

Laimiux commented Jul 18, 2019

Uh oh!

JakeWharton commented Jul 18, 2019 via email

Uh oh!

Laimiux commented Jul 18, 2019

Uh oh!

JakeWharton commented Jul 18, 2019 via email

Uh oh!

TWiStErRob commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mochadwi commented Apr 8, 2020

Uh oh!

TWiStErRob commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mochadwi commented Apr 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Laimiux commented Jul 15, 2019 •

edited

Loading

TWiStErRob commented Aug 3, 2019 •

edited

Loading

TWiStErRob commented Apr 8, 2020 •

edited

Loading