Leakcanary: Remove no-op dependency

[pyricau]

The no-op dependency was meant as a minimal JAR that would have a very small impact on an app release. The idea was to provide the necessary API needed for production code (ie RefWatcher) and LeakCanary.install as a getting started case, and then we wanted developers to override their application class in development and have more customization there.

• We've had issues reported that were build errors related to the no-op dependency
• Developers often want access to more of the LeakCanary API in the no-op dependency, for instance #679
• Some team at Square weren't comfortable with having to create a debug app class just to do this (cc @JakeWharton). APIs should minimize setup work, and we should make it easier to go beyond LeakCanary.install.

We could:

• remove the no-op dependency,
• split leakcanary-android into an API and an implementation.
• leakcanary-android-api would detect if the impl is in the classpath at runtime.
• leakcanary-android-api would have a dependency on leakcanary-watcher
• leakcanary-android-impl (better name?) would have a dependency on leakcanary-analyzer

The immediate downside is that LeakCanary will contribute more code to release builds. Developers would care about code size can configure Proguard to remove all of LeakCanary in release builds.

[autonomousapps]

FWIW, I already have a debug app class for other very useful purposes, and I don't see the problem with it, although I acknowledge initial setup could be less "painful". :shrug:

[pyricau]

Let's add some feedback on top of the 👎 :)

@autonomousapps Why do you want to add AndroidRefWatcherBuilder to the no-op library? Isn't that because you want to configure LeakCanary from your main sources instead of doing it from debug sources?

[rubengees]

@pyricau Sorry, should have commented the reason. I like that no-op adds absolutely nothing to release builds and I would like it to remain that way (without additional proguard config). LeakCanary is a debug tool and should leave no trace in release builds imho.

[JakeWharton]

The no-op dependency isn't empty. It's the same API without implementation.
You can find every app using LeakCanary by finding the classes the no-op
dependency adds in their APK.

On Sun, Apr 22, 2018, 3:18 PM Ruben Gees [e-mail] wrote:

@pyricau https://github.com/pyricau Sorry, should have commented the
reason. I like that no-op adds absolutely nothing to release builds and I
would like it to remain that way (without additional proguard config).
LeakCanary is a debug tool and should leave no trace in release build imho.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/square/leakcanary/issues/979#issuecomment-383405519,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEEEa7esRF72_fefholt39lJC0l5j5bks5trNeggaJpZM4Te4ae
.

[rubengees]

Yes, you are right! Then my argument would be that the dependency on leakcanary-watcher is much larger (117 as of version 1.5) than 9 (or 0 with proguard) methods, which seems unnecessary to me, as most apps won't need it.

[pyricau]

It will be a bit more out of the box, definitely. But you'll still get exactly 0 with Proguard :) . We could also look at splitting watcher to truly have only APIs in there.

[JakeWharton]

That would be a dependency of the implementation artifact, not the api
artifact.

On Sun, Apr 22, 2018, 3:26 PM Ruben Gees [e-mail] wrote:

Yes, you are right! Then my argument would be that the dependency on
leakcanary-watcher is much larger (117 as of 1.5) than 9 (or 0 with
proguard) methods, which seems unnecessary to me, as most apps won't need
it.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/square/leakcanary/issues/979#issuecomment-383405974,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEEEVWXsF5CSb3tPhpdcoyvBsb4ZMmzks5trNligaJpZM4Te4ae
.

[pyricau]

@JakeWharton in the initial proposal I did write:

leakcanary-android-api would have a dependency on leakcanary-watcher

Here's the list of classes in Watcher. Most of these are APIs necessary for configuring LeakCanary. It's 12 classes and a few inner classes.

The API surface isn't super small if you consider default implementations and classes that can be overridden. But it's also not very large, and should be super easy to proguard.

[autonomousapps]

@pyricau you said

Why do you want to add AndroidRefWatcherBuilder to the no-op library? Isn't that because you want to configure LeakCanary from your main sources instead of doing it from debug sources?

This was a year ago, so my memory is hazy, but it had to do with unit testing. I was using Robolectric, and a particular unit test was pulling in our debug application class, which used AndroidRefWatcherBuilder. I think that I tried using the no-op lib for the androidTest source set, but that of course failed -- hence my PR.

[jrodbx]

@pyricau: elaborating more + addressing your comments here: https://github.com/square/leakcanary/issues/979#issue-316582754

We've had issues reported that were build errors related to the no-op dependency

AFAIK, those were mainly related to Instant Run, but I'm likely thinking of different issues than you are. Do you have links to the particular issues you're considering?

Developers often want access to more of the LeakCanary API in the no-op dependency, for instance #679

Re: #679, Robolectric has been pesky in the past. However, I thought @edenman resolved that specific use case in https://github.com/square/leakcanary/pull/811.

Devs want more in the no-op dependency because, currently, customized LeakCanary configurations require variant-specific indirections.

Some team at Square weren't comfortable with having to create a debug app class just to do this (cc @JakeWharton). APIs should minimize setup work, and we should make it easier to go beyond LeakCanary.install.

Cash doesn't use a debug app for this, but it does use a variant-specific implementation of a wrapper class since it customizes the ExcludedRefs used to set up the RefWatcher. +1 to minimized setup, but I struggle to see how much less setup one can get than:

    if (LeakCanary.isInAnalyzerProcess(this)) return;
    LeakCanary.install(this);

Forcing developers to Proguard (or R8) if they don't like LeakCanary in their release builds isn't great either. Yea, it's a _few more classes_, but it's still debug code leaking into release APKs.

[JakeWharton]

Ooo Robolectric is another good motivator for this though. By having an API dep that is part of all variants and an implementation dep that isn't, you can specify the latter as debugApk. This means it's only bundled in the APK and won't be on the IDE classpath or test classpath.

[autonomousapps]

On the subject of forcing devs to use Proguard, it might be worth mentioning that's there's a whole section in the dagger.android docs devoted to discussing the expectation that devs use proguard. I think it's basically de rigueur.

[pyricau]

I do like the idea of being able to hide the impl from the classpath.

@jrodbx you're right, the setup can't get simpler than:

    if (LeakCanary.isInAnalyzerProcess(this)) return;
    LeakCanary.install(this);

It's the one setup that's the no-op supports, with the goal of letting you add these two lines in one place instead of several variants. That stops working as soon as you want to customize LeakCanary, which is more common than I initially thought (the excluded leaks and the result service are two common ones).

[JakeWharton]

@autonomousapps for what it's worth, that section is bullshit and should be removed: https://github.com/google/dagger/issues/319 & https://github.com/google/dagger/issues/407

[pyricau]

There's also a middle ground which consists in doing the split but also keeping the no-op dependency as is. Not sure if it's worth it, but essentially you pick one of two API AARS (no-op or customizable) and you have one debugApk impl. Developers can still start with no-op. If they need to customize, they can replace no-op with customizable and use proguard, or they can replace no-op with customizable in their debug variant.

[pyricau]

Another option (?) may be to auto install leakcanary when its there, and have 0 no op dependency.

I can see 3 approaches:

1. Add the debugImpl dep, 0 config, leakcanary set up in dev automatically
2. Do 1. and add customization in dev only code.
3. Add a "leak canary api" dep that makes APIs available in release builds.

Option 3 may sound like it makes things easier for devs, but it's actually very dangerous. A customization API includes customizing the result service and a bunch of other things, which means a high risk of shipping extra services and dev code in production. 0 dependency would be nice for anyone not using proguard.

https://github.com/square/leakcanary/wiki/Future#no-config-out-of-the-box

[pyricau]

We'll go with option 1, ie LeakCanary will now be just the one root dependency, no more no-ops. It'll automatically start doing work when the dependency is there on a debuggable APK, with no extra configuration. Setup code will show to use debugImplementation. Examples for configuring will recommend creating a debug application class. LeakCanary will also have production ready options, though that'll definitely require explicit configuration (it's one thing to accidentally include the dependency, it's another to have it start running)

[pyricau]

Done on master.

[lamster2018]

sometime we only have one single testing team to test release version of apk.

https://github.com/lamster2018/EmptyLeakCanary

i try to support same package class to solve problem.