Kotlin dsl support #262

marcoferrer · 2018-08-23T13:19:59Z

~~This is not ready to merge and is still a work in progress but I wanted to open the pr a little early to get some feedback on the proposed approach to resolving #219.~~

googlebot · 2018-08-23T13:20:12Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

marcoferrer · 2018-08-23T13:22:02Z

I signed it!

zpencer · 2018-08-23T19:29:13Z

dummy comment to try and unwedge CLA bot

zpencer · 2018-08-23T19:50:32Z

I like this idea of adding extensions functions, and then changing the groovy API only if extensions can not solve the problem. Taking a compile only dependency on kotlin is fine.

Would it be possible to add a test case that shows how a kotlin DSL build file looks? https://github.com/google/protobuf-gradle-plugin/pull/205/files may be a possible starting point.

marcoferrer · 2018-08-24T03:25:50Z

exampleProject/build.gradle.kts

+    // Extra proto source files besides the ones residing under
+    // "src/main".
+    "protobuf"(files("lib/protos.tar.gz"))
+    "protobuf"(fileTree("ext/"))


using string configuration access but i should probably look into adding an extension scoped to the dependency handler

marcoferrer · 2018-08-24T03:27:03Z

build.gradle

+// Temporary work around to get compilation working during development
+// Considering moving to a separate module
+// https://discuss.gradle.org/t/kotlin-groovy-and-java-compilation/14903/10
+compileGroovy.dependsOn = compileGroovy.taskDependencies.mutableValues - 'compileJava'


This is really ugly so Im hoping to move it to a seperate module. Im just not sure where it should live in the project.

marcoferrer · 2018-08-24T03:30:57Z

@zpencer I added an example of a kotlin dsl script based on the original example. It is still in the example directory while I try to figure out how to wire up a test project for this that would work with the build process. Im open to suggestions on how to approach it.

marcoferrer · 2018-08-24T03:40:05Z

Tested the current impl in a new idea project and everything seems to be working so far.
Type resolution and code generation are working as expected. Here’s a screenshot with type hints enabled.

gmale · 2018-10-10T16:30:18Z

This is a huge improvement over the currently required syntax!

I'm not sure if this question is out of scope (i.e. too complex of a change to the underlying plugin). I only bring it up since the PR requested feedback:

Is there a way to reduce the amount of boilerplate? All the developer is really trying to convey here is, "I want to use protobuf with grpc and javalite" which is probably an extremely common usecase for kotlin-dsl users. Any logic that could be moved out of the extension object and into the plugin would make life easier for many users. For example:

protobuf {
    plugins {
        id("grpc") option "lite"
    }
}

or maybe:

protobuf {
    plugins {
        id("javalite") version "3.0.0"
        id("grpc") version "1.8.0"
    }
}

marcoferrer · 2018-10-11T11:54:31Z

Performed some clean up and pushed the updated example project.

I think there might be a way to implement the shorter syntax for options in the task.plugin config block. ie

ofSourceSet("main").forEach{
            it.plugins {
                id("grpc") option "testing"
            }
}

Im just not sure having options configurable in the protobuf.plugins block is a good idea since the assumed config would be implicitly applied to all source sets.

googlebot · 2018-10-11T17:06:26Z

CLAs look good, thanks!

marcoferrer · 2018-10-11T17:49:07Z

@zpencer Im not sure if my build is failing in travis to due master failing?

zpencer

RE failing CI: it looks like this test is failing, but in master it is passing: ./gradlew test --tests com.google.protobuf.gradle.ProtobufAndroidPluginTest --stacktrace"

I'm still learning Kotlin, so please excuse my basic questions about syntax that is hard to learn about by Googling :)

zpencer · 2018-10-11T23:38:19Z

src/main/kotlin/com/google/protobuf/gradle/KotlinInteroperability.kt

+
+fun ProtobufConfigurator.plugins(closure: NamedDomainObjectContainerScope<ExecutableLocator>.() -> Unit) {
+    plugins(delegateClosureOf<NamedDomainObjectContainer<ExecutableLocator>> {
+        this{ this.closure() }


What does this { ... } mean in Kotlin?

Yeah looks gross and after doing some additional digging I updated with a cleaner usage. This implementation was a port from the existing example provided here #219 (comment)

Back to your original question. There are a couple of Kotlin features at work here that make up this statement.

Lambda with Receiver

The closureOf method takes a lambda and the this property in that lambda is the argument passed in.

Invoke Operator Overloading

The gradle kotlin dsl provides and extension function overloading the invoke operator for objects with type NamedDomainObjectContainer

This function allows us to apply our configuration to the receiver

Lambda as Last Param

If the last argument to a function is a lambda then the parenthesis can be omitted.

In the latest commit, this code has been refactored to something thats a little more readable, after looking into what exactly the method needed to achieve.

fun GenerateProtoTask.plugins(closure: NamedDomainObjectContainerScope<GenerateProtoTask.PluginOptions>.()-> Unit) { plugins(closureOf<NamedDomainObjectContainer<GenerateProtoTask.PluginOptions>> { this(closure) }) }

zpencer · 2018-10-12T00:11:34Z

src/main/kotlin/com/google/protobuf/gradle/KotlinInteroperability.kt

+/**
+ * The 'testProtobuf' configuration.
+ */
+val ConfigurationContainer.testProtobuf: Configuration


Can this be more general? It looks like this gives special treatment to the main sourceset (protobuf) and the test sourceset (testProtobuf) but won't handle user created sourcesets like this:

sourceSets { main { } foo { java { srcDirs 'src/foo/' } } } dependencies { fooProtobuf files("/tmp/protos-test.tar.gz") }

createConfiguration is where the plugin creates the xxxxProtobuf configurations in the project:
https://github.com/google/protobuf-gradle-plugin/blob/master/src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy#L154

Maybe there is some way this extension can plug into them in a general way.

So I cant dynamically provide the dependency configuration methods, but I can try providing a nicer mechanism.

You can try it out in the latest commit.

dependencies{ protobuf["sample"]("com.google.protobuf:protobuf-java:3.6.1") }

This solution has some pros and cons so I wanted to discuss them with you when possible. Ill add a full explanation in a few.

zpencer · 2018-10-12T00:27:57Z

src/main/kotlin/com/google/protobuf/gradle/KotlinInteroperability.kt

+}
+
+fun ProtobufConfigurator.protoc(closure: ExecutableLocator.() -> Unit) {
+    protoc(delegateClosureOf(closure))


I am still trying to wrap my head around delegateClosureOf vs closureOf (https://github.com/gradle/kotlin-dsl/blob/master/doc/getting-started/Closures.md). Is delegateClosureOf meant for closures that make use of the closure delegate from groovy ? As far as I can tell this plugin does not use this feature.

So is a by-product of my ignorance of groovy. I updated to use closureOf

zpencer · 2018-10-12T00:31:01Z

src/main/kotlin/com/google/protobuf/gradle/KotlinInteroperability.kt

+    })
+}
+
+fun ProtobufConfigurator.GenerateProtoTaskCollection.ofSourceSet(sourceSet: String): Collection<GenerateProtoTask> =


Why can't the groovy version of these functions be called directly? Maybe a comment would be helpful.

I added a comment to the source but Ill post it here as well.

The method generatorProtoTasks applies the supplied closure to an
instance of GenerateProtoTaskCollection.

Since JavaGenerateProtoTaskCollection and AndroidGenerateProtoTaskCollection
each have unique methods, and only one instance in allocated per project, we need a way to statically resolve the available methods. This is a necessity since Kotlin does not have any dynamic method resolution capabilities.

Ah, so the problem is the dynamic typing of GenerateProtoTaskCollection, since we don't know whether it is JavaGenerateProtoTaskCollection or AndroidGenerateProtoTaskCollection, kotlin's static typing means we can call neither's methods.

What do you think about instead adding two extension methods to GenerateProtoTaskCollection like asJavaTasks returning the same object casted as JavaGenerateProtoTaskCollection (or an empty collection if not applicable), and asAndroidTasks casting as AndroidGenerateProtoTaskCollection or an emtpy collection. That way, we minimize the amount of extension code that must be kept in sync with the groovy DSL. Since we expect kotlin DSL users to be in an IDE, this should be relatively discoverable code auto completion.

marcoferrer · 2018-10-15T18:49:09Z

Awesome Ill clean up ProtobufDependencyConfiguration.kt.

In regards to writing up tests, Im still trying to get a test project working with the existing test classes. There isnt much available in terms of resources for testing kotlin dsl projects using the gradle runner so its been a slow process. Just wanted to give a heads up.

marcoferrer · 2018-10-15T19:05:27Z

Also there are extensions built into the kotlin gradle dsl that clean up the custom source set syntax a little further

dependencies {
    "fooProtobuf"(files("/tmp/protos-test.tar.gz"))
}

marcoferrer · 2018-10-16T02:08:44Z

testProjectKotlinDslBase/build.gradle.kts

+
+// This extension is not auto generated when we apply the plugin using
+// apply(plugin = "com.google.protobuf")
+val Project.protobuf: ProtobufConvention get() =


Since this is another extension that relies on the plugin being applied in the plugins block, should we add it to ProtobufConfiguratorExts.kt as well? It would normalize different plugin usages.

Can you clarify what this block is doing? Also it looks like HasConvention is not a part of the public gradle API

This extension is used to get the plugin convention from the project, which was needed for Line 106. I didnt realize that HasConvention isnt part of the public api. The code itself I copied from the dynamically generated code file that gradle creates when you use the plugins block. I can see if there's a way around casting to HasConvention

Cleaned up the usage to the following

val Project.protobuf: ProtobufConvention get() = this.convention.getPlugin(ProtobufConvention::class)

nit: Can this workaround be moved nearer to apply(plugin = "com.google.protobuf") ?

marcoferrer · 2018-10-16T02:11:46Z

Finally got tests added. PR is ready for a full review whenever you get a chance.

examples/exampleKotlinDslProject/settings.gradle.kts

zpencer · 2018-10-18T01:59:39Z

testProjectKotlinDslBase/build.gradle.kts

+
+// This extension is not auto generated when we apply the plugin using
+// apply(plugin = "com.google.protobuf")
+val Project.protobuf: ProtobufConvention get() =


Can you clarify what this block is doing? Also it looks like HasConvention is not a part of the public gradle API

zpencer

Thanks a lot for contributing this PR! I added a few small remaining concerns.

zpencer · 2018-10-18T17:26:04Z

gradle/wrapper/gradle-wrapper.properties

@@ -1,5 +1,5 @@
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-4.8-bin.zip


undo this change

examples/exampleKotlinDslProject/settings.gradle.kts

zpencer · 2018-10-18T17:43:26Z

testProjectKotlinDslBase/build.gradle.kts

+
+// This extension is not auto generated when we apply the plugin using
+// apply(plugin = "com.google.protobuf")
+val Project.protobuf: ProtobufConvention get() =


nit: Can this workaround be moved nearer to apply(plugin = "com.google.protobuf") ?

zpencer · 2018-10-24T00:18:16Z

@zhangkun83 would you like to take a look before I merge?

zhangkun83 · 2018-10-24T00:48:12Z

I took a glance and the DSL looks pretty good. I trust @zpencer's judgement. Good job guys.

zpencer · 2018-10-24T00:49:16Z

Great! Let's merge it so our users can start using this experimental feature.

marcoferrer · 2018-10-24T16:14:20Z

Really happy to see this get merged. @zpencer Thanks again for all your help. When I get some free time Im hoping to add more documentation to the new code and possibly some additional tests.

This is an experimental feature. See `examples/exampleKotlinDslProject` for an example.

zhangkun83 · 2018-11-08T23:12:47Z

0.8.7 is released! It's already live on plugins.gradle.org. It may take a few hours for it to show up in Maven Central.

marcoferrer added 4 commits August 23, 2018 00:08

fix error in ide highlighting

0e47250

add kotlin dsl dependency to build

2865946

add initial interop extensions

9600995

added exts for different task collection types

32b3d53

added kotlin dsl example

4de07b1

marcoferrer commented Aug 24, 2018

View reviewed changes

added dependency configuration exts

995569c

zhangkun83 requested a review from zpencer October 10, 2018 16:38

marcoferrer added 6 commits October 11, 2018 07:45

apply kotlin style guide linting

32836af

move default example project

3e5fc62

add example kotlin dsl project

26549e1

bump kotlin version

d4002f7

bump gradle wrapper version 4.10

c63bcc7

remove old settings.gradle for example

7430894

zhangkun83 mentioned this pull request Oct 11, 2018

Prepare for 0.8.7 release #267

Closed

marcoferrer added 2 commits October 11, 2018 13:05

align kotlin version with version used kotlin gradle dsl

c71d47a

revert project gradle wrapper to 4.8

e76a6fc

zpencer reviewed Oct 12, 2018

View reviewed changes

add support for custom configuration dependency management

f39687f

marcoferrer added 6 commits October 15, 2018 21:47

remove custom source set dependency helper

5ee5d3b

update sample ktdsl project config

89fd766

add kotlin dsl test project

5a96a1b

add kotlin dsl tests

ae2e090

clean up build script

de27e56

add explicit imports

96c0635

marcoferrer commented Oct 16, 2018

View reviewed changes

marcoferrer added 2 commits October 15, 2018 22:31

remove redundant kotlin dsl import

00d0a0b

move verifyProjectDirHelper to ProtobufPluginTestHelper

92da1d1

zpencer reviewed Oct 18, 2018

View reviewed changes

marcoferrer added 2 commits October 17, 2018 23:35

remove usage of non public api in test build script

61c8829

add gradle wrapper with version 4.10 to example project

0fc32d8

zpencer reviewed Oct 18, 2018

View reviewed changes

marcoferrer and others added 4 commits October 18, 2018 16:22

update example links in readme

35c7ae1

revert gradle wrapper and clean up test ktdsl build script

c98ece1

Update readme

8b7668c

fix line endings in gradlew files

d6c0635

zpencer approved these changes Oct 24, 2018

View reviewed changes

zpencer merged commit 548171d into google:master Oct 24, 2018

marcoferrer deleted the kotlin-dsl-support branch October 24, 2018 16:14

zhangkun83 pushed a commit to zhangkun83/protobuf-gradle-plugin-1 that referenced this pull request Nov 7, 2018

Kotlin dsl support (google#262)

c63ce80

This is an experimental feature. See `examples/exampleKotlinDslProject` for an example.

ejona86 mentioned this pull request Oct 8, 2022

Kotlin dsl example #205

Closed

Kotlin dsl support #262

Kotlin dsl support #262

Conversation

marcoferrer commented Aug 23, 2018 • edited Loading

googlebot commented Aug 23, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

marcoferrer commented Aug 23, 2018

zpencer commented Aug 23, 2018

zpencer commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoferrer commented Aug 24, 2018

marcoferrer commented Aug 24, 2018 • edited Loading

gmale commented Oct 10, 2018 • edited Loading

marcoferrer commented Oct 11, 2018

googlebot commented Oct 11, 2018

marcoferrer commented Oct 11, 2018 • edited Loading

zpencer left a 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

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

zpencer Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

marcoferrer commented Oct 15, 2018

marcoferrer commented Oct 15, 2018

marcoferrer Oct 16, 2018 • 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

marcoferrer commented Oct 16, 2018

Choose a reason for hiding this comment

zpencer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zpencer commented Oct 24, 2018

zhangkun83 commented Oct 24, 2018

zpencer commented Oct 24, 2018

marcoferrer commented Oct 24, 2018

zhangkun83 commented Nov 8, 2018 • edited Loading

marcoferrer commented Aug 23, 2018 •

edited

Loading

marcoferrer commented Aug 24, 2018 •

edited

Loading

gmale commented Oct 10, 2018 •

edited

Loading

marcoferrer commented Oct 11, 2018 •

edited

Loading

zpencer Oct 15, 2018 •

edited

Loading

marcoferrer Oct 16, 2018 •

edited

Loading

zhangkun83 commented Nov 8, 2018 •

edited

Loading