-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove no-op dependency #979
Comments
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". 🤷 |
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? |
@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. |
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 ***@***.***> 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
<#979 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEEEa7esRF72_fefholt39lJC0l5j5bks5trNeggaJpZM4Te4ae>
.
|
Yes, you are right! Then my argument would be that the dependency on |
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. |
That would be a dependency of the implementation artifact, not the api
artifact.
…On Sun, Apr 22, 2018, 3:26 PM Ruben Gees ***@***.***> 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
<#979 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEEEVWXsF5CSb3tPhpdcoyvBsb4ZMmzks5trNligaJpZM4Te4ae>
.
|
@JakeWharton in the initial proposal I did write:
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. |
@pyricau you said
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 |
@pyricau: elaborating more + addressing your comments here: #979 (comment)
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?
Re: #679, Robolectric has been pesky in the past. However, I thought @edenman resolved that specific use case in #811. Devs want more in the no-op dependency because, currently, customized LeakCanary configurations require variant-specific indirections.
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:
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. |
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 |
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. |
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:
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). |
@autonomousapps for what it's worth, that section is bullshit and should be removed: google/dagger#319 & google/dagger#407 |
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. |
Another option (?) may be to auto install leakcanary when its there, and have 0 no op dependency. I can see 3 approaches:
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 |
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) |
Done on master. |
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. |
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 could:
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.
The text was updated successfully, but these errors were encountered: