Add prototype for Gradle Kotlin script support #202

donat · 2017-01-27T14:20:30Z

No description provided.

oehme

This is a great start!

The biggest issue I see is the fact that we do an expensive computation synchronously in order to get information that is already there.

Basically the Kotlin plugin asks us for the template classpath in order to load our template class in order to read the annotation on it in order to find out the file regex. Instead, our TemplateProvider could tell the Kotlin plugin the regex directly (that's what the isApplicable method should be for). The Kotlin plugin should not even try to load the template up-front.

As long as we have this unnecessary up-front call, we'll pay the price of a synchronous TAPI operation, which might block the UI in order to download Gradle, which would be a really bad user experience.

The other comments are mostly cosmetic or out of curiosity. None of this should stop us from merging, but it would be great if you could create issues to follow up on these problems @donat

oehme · 2017-01-30T16:35:38Z

build.gradle

@@ -240,13 +240,14 @@ subprojects {
            targetCompatibility = 1.8
        }

-        tasks.withType(AbstractCompile).all {
+        tasks.matching { it instanceof JavaCompile || it instanceof GroovyCompile }.all {


Why no stick with AbstractCompile?

The kotlin plugin contributes KotlinCompile which extends AbstractCompile but doesn't have attributes like options.

oehme · 2017-01-30T16:36:21Z

build.gradle

-            if (targetPlatformVersion >= '46') {
-                options.compilerArgs << '-Werror'                
-            }
+// TODO (donat) kotlin compilation currently fails with bad path error


Kotlin uses the -Werror flag too?

I got the following Java compilation error which I wanted to deal with later:
warning: [path] bad path element "/Users/donat/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-reflect/1.1-M04/50e442efdd3eab1dcf2ddbf4b3ec5dabecf1b2b2/kotlin-runtime.jar": no such file or directory

oehme · 2017-01-30T16:37:39Z

org.eclipse.buildship.kotlin/build.gradle

+apply plugin: 'kotlin'
+
+dependencies {
+    compile 'org.gradle:gradle-core:3.3'


Should be parametrized, some as the TAPI version we use.

oehme · 2017-01-30T16:41:17Z

org.eclipse.buildship.kotlin/src/main/java/org/eclipse/buildship/kotlin/KotlinPlugin.java

+        try {
+            connection = GradleConnector.newConnector()
+                    .forProjectDirectory(projectDir)
+                    .useDistribution(new URI("https://repo.gradle.org/gradle/dist-snapshots/gradle-script-kotlin-3.3-20161205200654+0000-all.zip"))


Do we still need a special distribution to use Kotlin? I thought this worked now with the standard Gradle distros? We should use whatever is specified in the project settings.

When I first created the prototype, this is how it was done in gradle-script-kotlin. It's no longer necessary, I've changed that. (will push the changes later)

oehme · 2017-01-30T16:42:10Z

org.eclipse.buildship.kotlin/src/main/java/org/eclipse/buildship/kotlin/KotlinPlugin.java

+                    .forProjectDirectory(projectDir)
+                    .useDistribution(new URI("https://repo.gradle.org/gradle/dist-snapshots/gradle-script-kotlin-3.3-20161205200654+0000-all.zip"))
+                    .connect();
+            KotlinBuildScriptModel model = connection.getModel(KotlinBuildScriptModel.class);


Should respect offline mode, console listener etc. like our other model operations.

From the dependency resolver class we don't see the eclipse dependencies (because it runs separately). We should discuss this with JetBrains.

oehme · 2017-01-30T16:44:16Z

org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt

+@ScriptTemplateDefinition(
+    resolver = GradleKotlinScriptDependenciesResolver::class,
+    scriptFilePattern = ".*\\.gradle\\.kts")
+abstract class KotlinBuildScript(project: Project) : Project by project {


Why is this duplicated? This should already be in GSK

Here the resolver = GradleKotlinScriptDependenciesResolver::class, part is the interesting one: it references the class from the org.eclipse.buildship.kotlin plugin instead of from GSK.

Okay, same comment as above: This shows that loading the template to configure the IDE is wrong.

oehme · 2017-01-30T16:44:55Z

org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinModelQuery.kt

+	}
+
+	fun connectorFor(projectDir: File): GradleConnector =
+			GradleConnector.newConnector().forProjectDirectory(projectDir).useDistribution(URI("https://repo.gradle.org/gradle/dist-snapshots/gradle-script-kotlin-3.3-20161205200654+0000-all.zip"))


Hardcoded gradle-script-kotlin distro

oehme · 2017-01-30T16:47:11Z

org.eclipse.buildship.kotlin/src/main/java/org/eclipse/buildship/kotlin/KotlinPlugin.java

+    }
+
+    private static List<String> templateClasspathFor(File projectDir) {
+        ProjectConnection connection = null;


Duplicated code between here and KotlinModelQuery.

oehme · 2017-01-30T16:51:06Z

org.eclipse.buildship.kotlin/src/main/java/org/eclipse/buildship/kotlin/KotlinPlugin.java

+        return classpath;
+    }
+
+    private static List<String> templateClasspathFor(File projectDir) {


This should not even be necessary, we should talk to the JetBrains team about this. There absolutely no need to load the template class at this point. It introduces a synchronous call which may result in synchronously downloading Gradle etc. Our ScriptTemplateProvider can already tell the Kotlin plugin if it applies and what dependency resolver to use. So there is no need to load the template in order to look for the same information on some annotation.

I created https://github.com/gradle/buildship/issues/153 for that.

oehme · 2017-01-30T16:52:06Z

org.eclipse.buildship.kotlin/build.gradle

+
+dependencies {
+    compile 'org.gradle:gradle-core:3.3'
+    compile 'org.gradle:gradle-process-services:3.3'


Why do we need core and process-services?

Some random class was needed for the compilation.

Some random class? Can you be more specific? As far as I can see this should only be using the TAPI, not Gradle core.

Sorry for the sketchy answer. We need them to compile KotlinBuildScript.kt. The class was copied from gradle-script-kotlin which depends on those libraries. Without the dependencies we get the following compile errors:

:org.eclipse.buildship.kotlin:compileKotlin e: /Development/git/buildship/org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt: (11, 23): Unresolved reference: Project e: /Development/git/buildship/org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt: (12, 23): Unresolved reference: plugins e: /Development/git/buildship/org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt: (19, 43): Unresolved reference: Project e: /Development/git/buildship/org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt: (19, 54): Only interfaces can be delegated to e: /Development/git/buildship/org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt: (19, 54): Unresolved reference: Project e: /Development/git/buildship/org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt: (28, 49): Unresolved reference: ObjectConfigurationAction e: /Development/git/buildship/org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt: (29, 9): Unresolved reference: project e: /Development/git/buildship/org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt: (29, 17): Public-API inline function cannot access non-public-API 'internal open fun <ERROR FUNCTION>(): [ERROR : <ERROR FUNCTION RETURN TYPE>] defined in root package' e: /Development/git/buildship/org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt: (29, 25): Unresolved reference: it e: /Development/git/buildship/org.eclipse.buildship.kotlin/src/main/kotlin/org/eclipse/buildship/kotlin/KotlinBuildScript.kt: (29, 28): Public-API inline function cannot access non-public-API 'internal open fun <ERROR FUNCTION>(): [ERROR : <ERROR FUNCTION RETURN TYPE>] defined in root package'

I had to copy the class to reference my altered version of GradleKotlinScriptDependenciesResolver in the annotation. That, I needed to change to leverage the differences in Eclipse (I had to pass different attributes to just to highlight one). We should discuss with Rodrigo how to share the implementation between Buildship and gradle-script-kotlin.

This just shows that defining this stuff in the annotation doesn't make sense for the IDE. It should be provided by the ScriptTemplateProvider. Same as with the isApplicable check. We should not need to load the template at all at this point.

Yes, I agree with you.

donat · 2017-01-31T14:15:28Z

I've created new issues for all your concerns @oehme. Once I manage to get rid of the ip-validation failure I'll merge the branch to master.

donat mentioned this pull request Jan 27, 2017

Review Buildship support for Kotlin build scripts gradle/kotlin-dsl-samples#258

Closed

Add prototype for Gradle Kotlin script support

b903847

donat force-pushed the dc-kotlin-script branch 2 times, most recently from ce0bfc3 to 6d2256a Compare January 30, 2017 09:58

Add Kotlin Eclipse to all target platforms

51c72f6

donat force-pushed the dc-kotlin-script branch from 6d2256a to 51c72f6 Compare January 30, 2017 10:21

oehme approved these changes Jan 30, 2017

View reviewed changes

Merge branch 'master' into dc-kotlin-script

9414966

donat added 3 commits January 31, 2017 15:29

Use toolingApiVersion variable for kotlin plugin dependency versions

c9646f5

Remove KotlinModel query duplication

f6264d9

Use Gradle distribution from import properties

3ffc60e

donat merged commit acdfc01 into master Jan 31, 2017

donat deleted the dc-kotlin-script branch January 31, 2017 14:42

This was referenced Feb 4, 2017

Integrate existing Kotlin support to Buildship and let people try it out #222

Closed

Start discussion with JetBrains how to get rid of the need for ScriptTemplateProviderEx.getTemplateClassClasspath() #225

Closed

[deleted] #295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prototype for Gradle Kotlin script support #202

Add prototype for Gradle Kotlin script support #202

donat commented Jan 27, 2017

oehme left a comment •

edited

Loading

oehme Jan 30, 2017

donat Jan 31, 2017

oehme Jan 30, 2017

donat Jan 31, 2017

oehme Jan 30, 2017

donat Jan 31, 2017 •

edited

Loading

oehme Jan 30, 2017

donat Jan 31, 2017

oehme Jan 30, 2017

donat Jan 31, 2017 •

edited

Loading

oehme Jan 30, 2017

donat Jan 31, 2017

oehme Jan 31, 2017

oehme Jan 30, 2017

donat Jan 31, 2017 •

edited

Loading

oehme Jan 30, 2017

donat Jan 31, 2017 •

edited

Loading

oehme Jan 30, 2017

donat Jan 31, 2017

oehme Jan 30, 2017

donat Jan 31, 2017

oehme Jan 31, 2017

donat Jan 31, 2017 •

edited

Loading

oehme Jan 31, 2017 •

edited

Loading

donat Jan 31, 2017

donat commented Jan 31, 2017

Add prototype for Gradle Kotlin script support #202

Add prototype for Gradle Kotlin script support #202

Conversation

donat commented Jan 27, 2017

oehme left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donat Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donat Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donat Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donat Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donat Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

oehme Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donat commented Jan 31, 2017

oehme left a comment •

edited

Loading

donat Jan 31, 2017 •

edited

Loading

donat Jan 31, 2017 •

edited

Loading

donat Jan 31, 2017 •

edited

Loading

donat Jan 31, 2017 •

edited

Loading

donat Jan 31, 2017 •

edited

Loading

oehme Jan 31, 2017 •

edited

Loading