-
Notifications
You must be signed in to change notification settings - Fork 165
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 support for ADT 3.0 and ktlin tasks for multidimension projects #32
Conversation
…nsion builds. new google repo is added since this is now a maven repo where google hosts all of its tools from now on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't always use LibraryPlugin to get the variantManager
val variantManager = when { | ||
target.plugins.hasPlugin(AppPlugin::class.java) -> target.plugins.findPlugin(AppPlugin::class.java)?.variantManager | ||
target.plugins.hasPlugin(LibraryPlugin::class.java) -> target.plugins.findPlugin(LibraryPlugin::class.java)?.variantManager | ||
target.plugins.hasPlugin(InstantAppPlugin::class.java) -> target.plugins.findPlugin(LibraryPlugin::class.java)?.variantManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @originx, is it wanted to always use the LibraryPlugin? Probably it will be better to use the specific Plugin, in this case InstantAppPlugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep thanks, rush of duplicating lines, fixed now :) thx
build.gradle.kts
Outdated
} | ||
|
||
dependencies { | ||
compileOnly(gradleApi()) | ||
compileOnly(kotlin("gradle-plugin", "1.1.1")) | ||
compileOnly("com.android.tools.build:gradle:2.3.3") | ||
compileOnly("com.android.tools.build:gradle:3.0.0-rc2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.0.0
is already available, please update to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
@originx can you also update CHANGELOG.md? |
@Tapchicoma what about kotlin version? Any reason why the project is using 1.1.1 instead of 1.1.51? |
@leinardi I have the same question, can't answer it by now. But it is not the scope of this PR. |
…nsion builds. new google repo is added since this is now a maven repo where google hosts all of its tools from now on.
review comments done, please recheck, |
I want to test this changes later, when I will at home. Overall looks good. |
@@ -1,6 +1,6 @@ | |||
package org.jlleitschuh.gradle.ktlint | |||
|
|||
import com.android.build.gradle.BaseExtension | |||
import com.android.build.gradle.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use star imports please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ktlint-gradle should run ktlint on itself 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 7ddc8b0
Checked, works ok. @JLLeitschuh this PR breaks support for older android plugin (2.3.3). I propose release current master with fix for newer ktlint versions and then make a new release with this PR. |
This fix is going to require a major version bump of the plugin because one of our dependencies has incremented its major version number. |
This now has merge conflicts (sorry). We may want to merge #36 first. |
I just made your merge conflicts much worse. Sorry. |
Signed-off-by: Yahor Berdnikau <[email protected]>
CHANGELOG.md
Outdated
@@ -19,7 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). | |||
### Changed | |||
- Define a different output file for each sourceSet | |||
### Fixed | |||
- Fixed plugin doesn't apply custom reporter for ktlint versions >0.10.x (#28) | |||
- Fixed plugin doesn't apply custom reporter for ktlint versions >0.10.x (#28) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary whitespace change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, damn, will update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check this commit d26f377
target.plugins.hasPlugin(InstantAppPlugin::class.java) -> target.plugins.findPlugin(InstantAppPlugin::class.java)?.variantManager | ||
target.plugins.hasPlugin(FeaturePlugin::class.java) -> target.plugins.findPlugin(FeaturePlugin::class.java)?.variantManager | ||
target.plugins.hasPlugin(TestPlugin::class.java) -> target.plugins.findPlugin(TestPlugin::class.java)?.variantManager | ||
else -> throw StopExecutionException("Must be applied with 'android' or 'android-library' plugin.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all of these types guaranteed to be on the classpath when either the android
or the android-library
plugin is applied?
If only the android
plugin is applied will you get a ClassDefNotFoundException
for the AppPlugin
class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, everything is ok. I've tested on my local test project.
Signed-off-by: Yahor Berdnikau <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Unless you want to add the android sample project too?
No, definetely I will add android project sample but in a separate PR. |
Add support for Android build tools 3.0 and ktlin tasks for multidimension builds.
new google repo is added since this is now a maven repo where google
hosts all of its tools from now on.
related to:
#29