patrickfav / armadillo

A shared preference implementation for confidential data in Android. Per default uses AES-GCM, BCrypt and HKDF as cryptographic primitives. Uses the concept of device fingerprinting combined with optional user provided passwords and strong password hashes.
https://favr.dev/opensource/armadillo
Apache License 2.0
280 stars 52 forks source link

Remove Timber and allow for more custom logging #52

Closed marukami closed 4 years ago

marukami commented 4 years ago

I would like to include this library in an SDK as a fallback to Googles Keystore Wrapper… Especially since OEMs phone models have broken the keystone… somehow, at least to the best I can tell, not an expert here 😅

So, I would like to avoid having to require Timber in an SDK if possible. I've just removed it from the Armadillo module here, if you prefer to keep the logging I also made a logger branch here

patrickfav commented 4 years ago

Hi @marukami

First of all thanks again for your contribution.

About the PR: What is the reason you want to remove Timber? Timber is basically a single Class with (idk) 400 LoCs? The advantage of Timber for me is, that the library user can choose freely if she wants logs or not - this is more complicated with regular LogCat. Not having ANY logs at all is not really a valid solution for me.

The usual solution is to just let ProGuard remove the logs (and the Timber dependency files) if you do not want it in your app. If you us this library in your own, you can add consumer ProGuard files, so that will happen automatically for everybody using your lib.

See also these 2 relevant articles:

marukami commented 4 years ago

About the PR: What is the reason you want to remove Timber? Timber is basically a single Class with (idk) 400 LoCs?

Timber also has the lint rules and I think it as an API dependency will mean consumers app will now get Lint issues that they don't expect. I need to double-check that next week when back at work.

All that aside I get what you mean with the keeping some logging. So, I updated the PR to keep logs, but allow Apps or SDKs to null or replace the logger.

I'll also take look into the consumer proguard rules. I did try them before tho the SDK aar was still including the Timber calls when used in an App. Not sure what I was doing wrong there.

@patrickfav let me know if this is OK, if not that's fine I'll keep playing with proguard rules.

patrickfav commented 4 years ago

So the thing is, while I do understand where you are coming from, I do not share your concerns. As I see it, the current solution adds complexity with basically the exact same result as using Timber right now - its kind of reinventing the wheel for me. The current solution is tested and works well.

I changed the dependency scope to implementation, which is probably the correct scope, but even with api I dont think the lint rule are a problem, since the only concern Timber calls (and are only available during development/validate), so if you are not using it, you wont even know that they are there.

If you do not plant a Tree in Timber Im very sure, that the JIT will remove the calls in runtime, since its a noop. So even if you somehow can't remove them with ProGuard they dont matter either way.

marukami commented 4 years ago

@patrickfav

I dont think the lint rule are a problem, since the only concern Timber calls

No, if you use the Android Logger then you get LogNotTimber as a lint warning

Screen Shot 2020-06-08 at 2 42 24 PM

For my purposes I think I can have a plugin and when Timber is not included have the plugin automatically add an ignore rule to the LintOptions

Thank you so all your time here and going into such detail. No worries I'll close this PR now.

Thank you.

patrickfav commented 4 years ago

Ah I see, you are right, this is a bit suboptimal, bit you can disable this lint rule. I've released 1.0.0 with your AndroidX migration and changed the scope of Timber to implementation - pls check if the lint rules are not showing for you anymore.

And I do very much appreciate your input and respect the work, in this particular instance I'd like to keep it as it is.

Thanks and have good day mate!