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

[Feat] - consumer proguard rules #257

Closed
Alireza-Farahani opened this issue Nov 4, 2021 · 7 comments · Fixed by #264
Closed

[Feat] - consumer proguard rules #257

Alireza-Farahani opened this issue Nov 4, 2021 · 7 comments · Fixed by #264

Comments

@Alireza-Farahani
Copy link
Contributor

Since this a android library and you publish an aar artifact, you can include proguard rules in the final artifact and remove the burden from end user. See here

android {
    defaultConfig {
        consumerProguardFiles("lib-proguard-rules.txt")
    }
    ...
}

By the way, is it necessary to keep all of -keep class androidx.appcompat.widget.** { *; } classes or could it be more narrowed?

@Canato
Copy link
Member

Canato commented Nov 4, 2021

@Alireza-Farahani this is some I heritage and never had the time to take a look, would you mind of opening a PR with this amazing changes? ^^

@Alireza-Farahani
Copy link
Contributor Author

Alireza-Farahani commented Nov 4, 2021

I can create the PR and I like to, since it seems a great "first PR". It could take a while because I'm not very familiar with PR process.

@Canato
Copy link
Member

Canato commented Nov 4, 2021

No issue, take your time to make it good, just keep this thread update!

If you need any help let me know and I will review the code with you before merge =)

@Alireza-Farahani
Copy link
Contributor Author

Alireza-Farahani commented Nov 4, 2021

I saw in an issue of ArthurHub ImageCropper that the proguard rule is not necessary. I tested your sample via

minifyEnabled true
shrinkResources true
proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro'

and it just worked! Do you have previously faced Proguard issues at all, or it is something copied from the old ImageCropper library?

@Canato
Copy link
Member

Canato commented Nov 5, 2021

This is very sensible, even what we keep on the proguard is not related to this library. But AndroidX.

This may have issue in some usages of AndroidX with this lib, you are right about that is something we just copied to this library.

  • If we decide to remove we need to keep and eye if someone who use this library will have it break and fix on the fly.
  • If we add to the library, like you suggest, we can sleep calm hahahha.

I let you decide what is the better cause I like have more people opinions and we work together to improve the code and people usage XD

@Alireza-Farahani
Copy link
Contributor Author

Alireza-Farahani commented Nov 6, 2021

I would go with the first,

  • The library as you confirmed and as I search it, doesn't reference any appcompat.widget classes. All of the library compile dependencies are also androidx libraries which have consumer proguard rules included. For example appcompat.appcompat library has these rules:
# aapt is not able to read app::actionViewClass and app:actionProviderClass to produce proguard
# keep rules. Add a commonly used SearchView to the keep list until b/109831488 is resolved.
-keep class androidx.appcompat.widget.SearchView { <init>(...); }

# Never inline methods, but allow shrinking and obfuscation.
-keepclassmembernames,allowobfuscation,allowshrinking class androidx.appcompat.widget.AppCompatTextViewAutoSizeHelper$Impl* {
  <methods>;
}

So it seems our proguard rule should not be needed.

  • We are using this library in our app, that is near its first release. We certainly will test it in obfuscated mode
  • The library is keeping appcompat.widget.** sub package which is pretty larger

@Canato
Copy link
Member

Canato commented Nov 9, 2021

Perfect! Feel free to drop the PR @Alireza-Farahani

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 a pull request may close this issue.

2 participants