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

Add consumerProguardFiles configuration. Fixes #646 #761

Merged
merged 1 commit into from Jan 11, 2016
Merged

Add consumerProguardFiles configuration. Fixes #646 #761

merged 1 commit into from Jan 11, 2016

Conversation

vanniktech
Copy link
Contributor

According to the gradle.properties file you ship the library in an AAR package. So including the proguard rules should work.

Once a new version is released, the README should be changed to say that this library automatically ships the proguard rules starting version xyz.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@vanniktech
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@TWiStErRob
Copy link
Collaborator

I think the :library module is not a release artifact. What you download when you referece Glide in your build.gradle is the glide module's output, which is a .jar. This is why I said the following in #646:

I guess an AAR distribution could be made to include this, since it will help some people (using @aar), but it won't make AAR the required form of consuming Glide.

There's probably some Gradle magic needed to produce both .jar and .aar from module :glide or a separate module for AAR build. Also see scripts/upload.gradle.

@vanniktech
Copy link
Contributor Author

I cleaned scripts/upload.gradle up with #762

@vanniktech
Copy link
Contributor Author

@TWiStErRob @sjudd
Why is the packaging for glide specified as jar?

I believe that if we change this one to aar this PR could be merged and the proguard rules would be shipped.

@TWiStErRob
Copy link
Collaborator

This is a library and not everyone uses Gradle. Since Glide has no resources it doesn't make sense to force AAR. Also for this little change (copying 2 lines into a proguard file) it's not worth to break compatibility with those users. Eclipse for example is horrible for consuming AARs (especially with Maven) last time I checked... that was when I started using IDEA as main IDE. For non-Gradle it's easier to just drop the glide jar into the libs folder and it just works, with aar you have to extract and import a separate project, set up dependencies... This is why I'm suggesting (see above in other comments) a double-distribution of aar/jar side by side. @sjudd's intentions may have been different originally, the above is just my view.

@vanniktech
Copy link
Contributor Author

I'd be up for distributing both aar & jar.

@sjudd
Copy link
Collaborator

sjudd commented Dec 8, 2015

Although things have vastly improved over the last couple of years for AARs, I still agree that having a fat jar available is helpful for some users.

I'm totally ok with AAR+Jar.

I think if we do release an AAR, we can upload gradle projects individually (ie library + third_party/disklrucache + ...) rather than trying to combine them into a single AAR before upload. It would be a little weird to have AAR users depend on glide:library and jar users depend on the existing glide:glide, but it's probably worth the simplification in our build rules.

@vanniktech
Copy link
Contributor Author

So basically we'll have the old way plus for each subproject it's on AAR package and then you will just include whatever you need?

@TWiStErRob
Copy link
Collaborator

I think glide:glide should be released as both AAR and JAR, there's no point in releasing each submodule because they're private to glide. There are many artifacts (e.g. intg libs) already in the maven repos, I don't think we need more. They are also not really useful by themselves, or already have official releases by other names.

@sjudd
Copy link
Collaborator

sjudd commented Dec 9, 2015

@TWiStErRob I see what you're saying, but since maven and gradle handle transitive dependencies, there's no reason not to take advantage of that and upload each subproject individually. It's fairly painful to combine subprojects into fat jars/aars. At the same time, subprojects encourage code re-use, and allow greater parallelism, which decreases build times.

I think the extra few largely useless artifacts is worth the simplification to our build process. Particularly since we can still easily point people to the single artifact they actually want. This strategy also seems to match the direction other libraries are moving in, where the library is broken down into multiple components.

@TWiStErRob
Copy link
Collaborator

This all assumes that you use Maven or Gradle. The point of having a fat jar is? to help people without those awesome systems. If we drop that support (of copying a single Glide jar to /libs) we might as well just change =jar to =aar in the properties file (they can extract classes.jar from aar too). Am I missing something?

@sjudd
Copy link
Collaborator

sjudd commented Dec 9, 2015

Oh sorry I was misleading. I totally agree we should continue to release a single JAR as well as the AARs. I just don't think we need to create a single fat AAR, but can instead release multiple smaller AARs that match the internal project structure.

@TWiStErRob
Copy link
Collaborator

Ah, ok. Good then. We don't have to have all smaller libs as AARs, e.g. gifdecoder wouldn't gain much from being an AAR.

A note on build times: doing that you said will really help us, but also think about consumers. Consuming an AAR is not cheap. You know that the Android Gradle plugin extracts the contents every time (clean) just to be able to reference the jar inside (and merge resources). Compare this to adding a path to classpath, the path of the downloaded/cached jar file in the .m2|gradle cache directory. So releasing aar pushes some build time from the libs to all of the consumers...

A side note: While I'm arguing for people who don't use Gradle/Maven I think anyone who is developing Android apps and not using AS(IDEA)/Gradle are shooting themselves in the foot, but we should let them suffer if that's the path they choose :) or forced to choose :(

@TWiStErRob
Copy link
Collaborator

@vanniktech do you need more clarification to work on it?

@vanniktech
Copy link
Contributor Author

@TWiStErRob nope I don't think so. I'll try to find some time in the next days

@TWiStErRob
Copy link
Collaborator

@sjudd, as pointed out in #855 (comment) this should be ready to merge.

@sjudd
Copy link
Collaborator

sjudd commented Jan 11, 2016

LGTM, thanks all!

sjudd added a commit that referenced this pull request Jan 11, 2016
Add consumerProguardFiles configuration. Fixes #646
@sjudd sjudd merged commit 04b8515 into bumptech:master Jan 11, 2016
@vanniktech
Copy link
Contributor Author

@TWiStErRob & @sjudd thanks for making this possible!

@vanniktech vanniktech deleted the master_add_consumerproguardfiles branch January 11, 2016 20:36
@TWiStErRob
Copy link
Collaborator

@vanniktech everyone wins if it works, but it's not fully magical yet: we won -5 lines of proguard config, but lost +2 lines because of adding @aar (see #882); it's still a -3 win :) I'm curious how it works for you.

@vanniktech
Copy link
Contributor Author

@TWiStErRob it's not about how many lives we saved. It's just that everyone who will use AAR has less trouble setting it up as usually some guys are forgetting to configure Proguard.

Also the moment the Proguard settings do change, you can just fix that right in the AAR and no one has the hassle to update it.

sjudd added a commit to sjudd/glide that referenced this pull request May 17, 2016
*** Reason for rollback ***

Broke []

*** Original change description ***

MOE automated commit.
-------------
Merge pull request bumptech#854 from luxiaoyu/master

fix bug of DiskCacheStrategy.RESOURCE

-------------
Merge pull request bumptech#856 from swankjesse/jwilson_0102_okhttp_3

Upgrade to OkHttp 3.

-------------
Merge pull request bumptech#874 from TWiStErRob/aar

Introduce AAR publish for :library and stop publishing :glide

-------------
Merge pull request bumptech#761 from vanniktech/master_add_consumerproguardfiles

Add consumerProguardFiles configuration. Fixes bumptech#646

-------------
Mer...

***
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=116167265
sjudd added a commit to sjudd/glide that referenced this pull request May 17, 2016
*** Reason for rollback ***

Broke []

*** Original change description ***

MOE automated commit.
-------------
Merge pull request bumptech#854 from luxiaoyu/master

fix bug of DiskCacheStrategy.RESOURCE

-------------
Merge pull request bumptech#856 from swankjesse/jwilson_0102_okhttp_3

Upgrade to OkHttp 3.

-------------
Merge pull request bumptech#874 from TWiStErRob/aar

Introduce AAR publish for :library and stop publishing :glide

-------------
Merge pull request bumptech#761 from vanniktech/master_add_consumerproguardfiles

Add consumerProguardFiles configuration. Fixes bumptech#646

-------------
Mer...

***
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=116167265
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

4 participants