yahoo / squidb

SquiDB is a SQLite database library for Android and iOS
https://github.com/yahoo/squidb/wiki
Apache License 2.0
1.31k stars 132 forks source link

Move plugins to root #175

Closed jaredsburrows closed 8 years ago

jaredsburrows commented 8 years ago

@sbosley Moved plugins to root to remove duplicated code

jdkoren commented 8 years ago

@sbosley Is anything adversely affected by removing the gradle version check? I don't recall what it was for.

jaredsburrows commented 8 years ago

@jdkoren Yes, it used an older plugin for older gradle versions. Since we should all be using the wrapper and the latest plugin, I just removed the old one.

sbosley commented 8 years ago

@jaredsburrows it's hypothetically possible that there are other users of this project who have not yet updated their gradle version and are building from source. Since this would technically be a backwards-incompatible change for such people, we might want to keep the version check on master and only remove it on our dev_3.0 branch instead.

jaredsburrows commented 8 years ago

@sbosley It doesn't make sense has the IDE naturally uses gradlew and uses of any gradle project should use gradlew.

sbosley commented 8 years ago

@jaredsburrows hmm I'm pretty sure that we had a problem once where this would break depending on the version of gradlew used by the root project (since I think the root project gradle is the one that gets used for building, not whatever version in the source dependencies happen to have). Now that I'm thinking of it, have you confirmed that this PR doesn't cause any problems when using squidb as a source dependency in some other project? We should probably test that before merging.

jaredsburrows commented 8 years ago

@sbosley Why would you ever use as a source dependency? That defeats the entire purpose of the android-maven-gradle-plugin plugin. All you have to do is use gradlew install when making changes.

sbosley commented 8 years ago

@jaredsburrows the android-maven-gradle-plugin is for us; it supports building the artifacts that we deploy to jcenter. However, some projects import squidb as a dependency from source (either for easier debugging, because they're using a slightly altered fork or a non-master branch, or for any number of other reasons). For these projects, the maven plugin does nothing useful, but it's still there and needs to be resolved since it's a part of the source code. I still think we need to test to make sure this change doesn't break projects for people using squidb in that way, and I'm not sure that I understand your objection -- do you see some way of removing that plugin for people building from source without breaking our ability to build artifacts for jcenter?

jaredsburrows commented 8 years ago

@sbosley Don't remove the plugin. It is great for publishing to mavenLocal() for testing. People should not be importing the library from source. They should always be using binary dependencies.

See: https://docs.gradle.org/current/userguide/maven_plugin.html#N13830

sbosley commented 8 years ago

Since the gradle version check is for a very niche use case that's probably not relevant to anyone anymore, I think after much discussion that it is probably ok to remove. If anyone complains, we can add it back in. @jaredsburrows thanks for taking the time to do the cleanup here!