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

Crash in non-debuggable builds #1806

Merged
merged 1 commit into from May 1, 2020
Merged

Crash in non-debuggable builds #1806

merged 1 commit into from May 1, 2020

Conversation

cketti
Copy link
Contributor

@cketti cketti commented Apr 25, 2020

Crash in release builds (really, any APK without android:debuggable="true"). Can be overridden by setting @bool/leak_canary_allow_in_non_debuggable_build to true.

Fixes #1804

@CLAassistant
Copy link

CLAassistant commented Apr 25, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@pyricau pyricau left a comment

Choose a reason for hiding this comment

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

Left some feedback, but other than that I like the general direction!

What do I run to check this crashes as expected? Also, probably need to update the XML in the sample app?


if (application.resources.getBoolean(R.bool.leak_canary_allow_in_non_debuggable_build)) {
// AppWatcher is disabled by default for non-debuggable builds, but we have a developer opt-in.
AppWatcher.config = AppWatcher.config.copy(enabled = true)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's interesting! Hadn't thought of this, makes sense to reenable.

If you're sure you want to include LeakCanary in a non-debuggable build, you can disable this
check by overriding the bool/leak_canary_allow_in_non_debuggable_build resource and setting
the value to 'true'.
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to add a corresponding section in the Code recipes and have the error message point to that page with an anchor to a detailed description with xml snippet to copy&paste. Source code of those docs is located in /leakcanary/docs/recipes.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A link for just the last part on how to disable the runtime check? Or replace the whole message with a link?

@cketti
Copy link
Contributor Author

cketti commented Apr 28, 2020

PR updated.

What do I run to check this crashes as expected?

The easiest way is manual testing. Setting up a release build of the sample app was painful. Adding debuggable false to the debug buildTypes config of the sample app also does the trick and is less hassle.

Also, probably need to update the XML in the sample app?

I added a code recipe with an XML snippet you can use to override the check during a manual test. I wouldn't modify the sample app in the repository because you don't want to encourage people to actually use this override.

Copy link
Member

@Armaxis Armaxis left a comment

Choose a reason for hiding this comment

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

LGTM! Added couple comments related to wording.

docs/recipes.md Outdated Show resolved Hide resolved
docs/recipes.md Outdated Show resolved Hide resolved
@cketti
Copy link
Contributor Author

cketti commented Apr 29, 2020

Updated with suggested wording changes.

@@ -105,6 +105,18 @@ In your leak reporting code:
val retainedInstanceCount = AppWatcher.objectWatcher.retainedObjectCount
```

## LeakCanary in release builds

We **do not recommend** including LeakCanary in your production build. To avoid accidentally including the `com.squareup.leakcanary:leakcanary-android` dependency in a release build, LeakCanary crashes during initialization if the APK is not debuggable.
Copy link
Member

Choose a reason for hiding this comment

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

nit: this says both "production" and "release". We should ideally pick one, as changing vocabulary can be confusing. Probably release?

Copy link
Member

Choose a reason for hiding this comment

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

Welp, I wanted to make a PR but I pushed to master instead. Oh well.

28d5378

🤫 don't tell anyone

@pyricau
Copy link
Member

pyricau commented May 1, 2020

Thanks!! I'll merge and follow up with the wording nit.

@pyricau pyricau merged commit 40ece74 into square:master May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LeakCanary should crash in release builds
4 participants