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

Only log debug and verbose output for DEBUG builds. #118

Closed
wants to merge 1 commit into from
Closed

Only log debug and verbose output for DEBUG builds. #118

wants to merge 1 commit into from

Conversation

ardevd
Copy link

@ardevd ardevd commented Feb 11, 2018

Ideally, verbose logging should not be compiled into an application except during development and debug logging should be stripped at runtime according to the Android developer documentation.

This PR at prevents debug and verbose output logging on non-DEBUG builds.

Fixes #116

Ideally, verbose logging should not be compiled into an application except during development and debug logging should be stripped at runtime.
@przybylski
Copy link
Member

This and #116 can coexist. The bigger issue is that probably ProGuard is not correctly configured. It should take care of removing debug logs instead of doing it with if check.

@ardevd
Copy link
Author

ardevd commented Feb 11, 2018

I'll be happy to create a PR with ProGuard configuration that strips out the debug logs if that's a preferred solution for you.

You can surely merge #116 if you want but it's a useful debug print imo.

@ardevd
Copy link
Author

ardevd commented Feb 12, 2018

Since you've already created a wrapper class for your logging function I think this PR is a good solution.

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Feb 12, 2018

👍 I am fine with both PRs and would say that this is a "quick" fix while I agree that the real solution would be to go for ProGuard afterwards. Simply to get rid of any potentially expensive string concatenation within a debug statement which of course gets executed before the debug level gets checked.

cc @tobiasKaminsky @mario

Approved with PullApprove

@tobiasKaminsky
Copy link
Member

As we do not have to rush, I think the best way would be to go with proGuard and even revert #116.

@mario
Copy link
Contributor

mario commented Feb 20, 2018

What's the verdict here?

@AndyScherzinger
Copy link
Member

@mario @tobiasKaminsky what's your opinion?

I am fine with merging this PR as-is and let @ardevd go for the proGuard implementation also (in a separate PR, as stated).

@ardevd
Copy link
Author

ardevd commented Feb 20, 2018

I'll get started on the ProGuard stuff soon. However I'll have to make a PR for the Android project as well since it's there the actual obfuscation and stripping will take place.

@tobiasKaminsky
Copy link
Member

https://medium.com/@elye.project/checking-debug-build-the-right-way-d12da1098120 indicates that it might be a problem, if this is used in a module.

boolean isDebug = ((getContext().getApplicationInfo().flags &
ApplicationInfo.FLAG_DEBUGGABLE) != 0);

seems to be a better way.

@tobiasKaminsky
Copy link
Member

debug logging should be stripped at runtime according to the Android developer documentation.

(proguard)/It should take care of removing debug logs instead of doing it with if check.

I also think it is better to do it with ProGuard so that we do not even use the log calls.
I will close this, keep it as a reference and open up a new issue, so that we can tackle this at some point.

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

Successfully merging this pull request may close these issues.

None yet

5 participants