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

Update to stable kotlin dsl #275

Merged
merged 6 commits into from
Dec 17, 2018

Conversation

marcoferrer
Copy link
Contributor

@marcoferrer marcoferrer commented Nov 17, 2018

This PR updates the Kotlin DSL version to stable 1.0.3 1.0.4 (version included in gradle 5.0) and resolves #274

Unfortunately it breaks compatibility with gradle 4.10.2 and ktdsl 1.0.0-rc6, due to changes in constructor visibility in NamedDomainObjectContainerScope. The release version exposes a static factory method for object instantiation that is not present in rc6.

Im still investigating the possibility of maintaining compatibility.


This change is Reviewable

@@ -100,7 +100,7 @@ tasks {
from(sourceSet.output)

val compileTaskName = sourceSet.getCompileTaskName("java")
dependsOn(tasks.getByName(compileTaskName))
dependsOn(project.tasks.getByName(compileTaskName))
Copy link
Contributor Author

@marcoferrer marcoferrer Nov 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An additional extension was added to the task container in 1.0.3. So in order to deal with ambiguity in method resolution I had to explicitly qualify the tasks property.

Copy link
Collaborator

@zhangkun83 zhangkun83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is failing. There seems to be two errors:

com.google.protobuf.gradle.ProtobufAndroidPluginTest > testProjectAndroidKotlin should be successfully executed (kotlin only) STANDARD_ERROR
    Failed to notify ProjectEvaluationListener.afterEvaluate(), but primary configuration failure takes precedence.
    java.lang.IllegalStateException: buildToolsVersion is not specified.
    	at com.google.common.base.Preconditions.checkState(Preconditions.java:173)
    	at com.android.build.gradle.BasePlugin.createAndroidTasks(BasePlugin.java:645)
    	at com.android.build.gradle.BasePlugin$10.call(BasePlugin.java:608)
    	at com.android.build.gradle.BasePlugin$10.call(BasePlugin.java:605)
    	at com.android.builder.profile.ThreadRecorder.record(ThreadRecorder.java:156)
    	at com.android.builder.profile.ThreadRecorder.record(ThreadRecorder.java:120)
    	at com.android.build.gradle.BasePlugin.lambda$createTasks$1(BasePlugin.java:603)
    	at org.gradle.internal.event.BroadcastDispatch$ActionInvocationHandler.dispatch(BroadcastDispatch.java:93)
    org.gradle.api.GradleScriptException: A problem occurred evaluating project ':testProjectAndroid'.
    	at org.gradle.groovy.scripts.internal.DefaultScriptRunnerFactory$ScriptRunnerImpl.run(DefaultScriptRunnerFactory.java:93)
    	at org.gradle.configuration.DefaultScriptPluginFactory$ScriptPluginImpl$2.run(DefaultScriptPluginFactory.java:177)
    	at org.gradle.configuration.ProjectScriptTarget.addConfiguration(ProjectScriptTarget.java:77)
    	at org.gradle.configuration.DefaultScriptPluginFactory$ScriptPluginImpl.apply(DefaultScriptPluginFactory.java:182)
    	at org.gradle.configuration.project.BuildScriptProcessor.execute(BuildScriptProcessor.java:38)
    	at org.gradle.configuration.project.BuildScriptProcessor.execute(BuildScriptProcessor.java:25)
    	at org.gradle.configuration.project.ConfigureActionsProjectEvaluator.evaluate(ConfigureActionsProjectEvaluator.java:34)
    	at org.gradle.configuration.project.LifecycleProjectEvaluator.evaluate(LifecycleProjectEvaluator.java:55)
    	at org.gradle.api.internal.project.AbstractProject.evaluate(AbstractProject.java:540)
    	at org.gradle.api.internal.project.AbstractProject.evaluate(AbstractProject.java:93)
    	at org.gradle.execution.TaskPathProjectEvaluator.configureHierarchy(TaskPathProjectEvaluator.java:47)
    	at org.gradle.configuration.DefaultBuildConfigurer.configure(DefaultBuildConfigurer.java:35)
    	at org.gradle.initialization.DefaultGradleLauncher$2.run(DefaultGradleLauncher.java:124)
    	at org.gradle.internal.Factories$1.create(Factories.java:22)
    	at org.gradle.internal.progress.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:91)
    	at org.gradle.internal.progress.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:53)
    	at org.gradle.initialization.DefaultGradleLauncher.doBuildStages(DefaultGradleLauncher.java:121)
    	at org.gradle.initialization.DefaultGradleLauncher.access$200(DefaultGradleLauncher.java:32)
    	at org.gradle.initialization.DefaultGradleLauncher$1.create(DefaultGradleLauncher.java:98)
    	at org.gradle.initialization.DefaultGradleLauncher$1.create(DefaultGradleLauncher.java:92)
    	at org.gradle.internal.progress.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:91)
    	at org.gradle.internal.progress.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:63)
    	at org.gradle.initialization.DefaultGradleLauncher.doBuild(DefaultGradleLauncher.java:92)
    	at org.gradle.initialization.DefaultGradleLauncher.run(DefaultGradleLauncher.java:83)
    	at org.gradle.launcher.exec.InProcessBuildActionExecuter$DefaultBuildController.run(InProcessBuildActionExecuter.java:99)
    	at org.gradle.tooling.internal.provider.runner.BuildModelActionRunner.run(BuildModelActionRunner.java:46)
    	at org.gradle.launcher.exec.ChainingBuildActionRunner.run(ChainingBuildActionRunner.java:35)
    	at org.gradle.tooling.internal.provider.runner.SubscribableBuildActionRunner.run(SubscribableBuildActionRunner.java:58)
    	at org.gradle.launcher.exec.ChainingBuildActionRunner.run(ChainingBuildActionRunner.java:35)
    	at org.gradle.launcher.exec.InProcessBuildActionExecuter.execute(InProcessBuildActionExecuter.java:48)
    	at org.gradle.launcher.exec.InProcessBuildActionExecuter.execute(InProcessBuildActionExecuter.java:30)
    	at org.gradle.launcher.exec.ContinuousBuildActionExecuter.execute(ContinuousBuildActionExecuter.java:81)
    	at org.gradle.launcher.exec.ContinuousBuildActionExecuter.execute(ContinuousBuildActionExecuter.java:46)
    	at org.gradle.tooling.internal.provider.DaemonBuildActionExecuter.execute(DaemonBuildActionExecuter.java:77)
    	at org.gradle.tooling.internal.provider.DaemonBuildActionExecuter.execute(DaemonBuildActionExecuter.java:46)
    	at org.gradle.tooling.internal.provider.LoggingBridgingBuildActionExecuter.execute(LoggingBridgingBuildActionExecuter.java:60)
    	at org.gradle.tooling.internal.provider.LoggingBridgingBuildActionExecuter.execute(LoggingBridgingBuildActionExecuter.java:34)
    	at org.gradle.tooling.internal.provider.ProviderConnection.run(ProviderConnection.java:150)
    	at org.gradle.tooling.internal.provider.ProviderConnection.run(ProviderConnection.java:113)
    	at org.gradle.tooling.internal.provider.DefaultConnection.getModel(DefaultConnection.java:172)
    	at org.gradle.tooling.internal.consumer.connection.CancellableModelBuilderBackedModelProducer.produceModel(CancellableModelBuilderBackedModelProducer.java:53)
    	at org.gradle.tooling.internal.consumer.connection.PluginClasspathInjectionSupportedCheckModelProducer.produceModel(PluginClasspathInjectionSupportedCheckModelProducer.java:41)
    	at org.gradle.tooling.internal.consumer.connection.AbstractConsumerConnection.run(AbstractConsumerConnection.java:59)
    	at org.gradle.tooling.internal.consumer.connection.ParameterValidatingConsumerConnection.run(ParameterValidatingConsumerConnection.java:47)
    	at org.gradle.tooling.internal.consumer.DefaultBuildLauncher$1.run(DefaultBuildLauncher.java:89)
    	at org.gradle.tooling.internal.consumer.DefaultBuildLauncher$1.run(DefaultBuildLauncher.java:83)
    	at org.gradle.tooling.internal.consumer.connection.LazyConsumerActionExecutor.run(LazyConsumerActionExecutor.java:84)
    	at org.gradle.tooling.internal.consumer.connection.CancellableConsumerActionExecutor.run(CancellableConsumerActionExecutor.java:45)
    	at org.gradle.tooling.internal.consumer.connection.ProgressLoggingConsumerActionExecutor.run(ProgressLoggingConsumerActionExecutor.java:58)
    	at org.gradle.tooling.internal.consumer.connection.RethrowingErrorsConsumerActionExecutor.run(RethrowingErrorsConsumerActionExecutor.java:38)
    	at org.gradle.tooling.internal.consumer.async.DefaultAsyncConsumerActionExecutor$1$1.run(DefaultAsyncConsumerActionExecutor.java:55)
    	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:63)
    	at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:46)
    	at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:55)
    Caused by: java.lang.NoSuchMethodError: org.gradle.api.artifacts.dsl.DependencyHandler.getAttributesSchema()Lorg/gradle/api/attributes/AttributesSchema;
    	at org.jetbrains.kotlin.gradle.plugin.KotlinBasePluginWrapper.setupAttributeMatchingStrategy(KotlinPluginWrapper.kt:80)
    	at org.jetbrains.kotlin.gradle.plugin.KotlinBasePluginWrapper.apply(KotlinPluginWrapper.kt:75)
    	at org.jetbrains.kotlin.gradle.plugin.KotlinBasePluginWrapper.apply(KotlinPluginWrapper.kt:39)
    	at org.gradle.api.internal.plugins.ImperativeOnlyPluginApplicator.applyImperative(ImperativeOnlyPluginApplicator.java:35)
    	at org.gradle.api.internal.plugins.RuleBasedPluginApplicator.applyImperative(RuleBasedPluginApplicator.java:43)
    	at org.gradle.api.internal.plugins.DefaultPluginManager.doApply(DefaultPluginManager.java:137)
    	at org.gradle.api.internal.plugins.DefaultPluginManager.apply(DefaultPluginManager.java:112)
    	at org.gradle.api.internal.plugins.DefaultObjectConfigurationAction.applyType(DefaultObjectConfigurationAction.java:113)
    	at org.gradle.api.internal.plugins.DefaultObjectConfigurationAction.access$200(DefaultObjectConfigurationAction.java:36)
    	at org.gradle.api.internal.plugins.DefaultObjectConfigurationAction$3.run(DefaultObjectConfigurationAction.java:80)
    	at org.gradle.api.internal.plugins.DefaultObjectConfigurationAction.execute(DefaultObjectConfigurationAction.java:136)
    	at org.gradle.api.internal.project.AbstractPluginAware.apply(AbstractPluginAware.java:46)
    	at org.gradle.api.internal.project.ProjectScript.apply(ProjectScript.java:34)
    	at org.gradle.api.Script$apply$0.callCurrent(Unknown Source)
    	at build_62gdcmp0wcqcliehr5ivkhqhm.run(/home/travis/build/google/protobuf-gradle-plugin/build/tests/testProjectAndroidMain/testProjectAndroid/build.gradle:7)
    	at org.gradle.groovy.scripts.internal.DefaultScriptRunnerFactory$ScriptRunnerImpl.run(DefaultScriptRunnerFactory.java:91)
    	... 53 more

@@ -24,7 +24,7 @@ fun ProtobufConfigurator.protoc(closure: ExecutableLocator.() -> Unit) {

fun ProtobufConfigurator.plugins(closure: NamedDomainObjectContainerScope<ExecutableLocator>.() -> Unit) {
plugins(closureOf<NamedDomainObjectContainer<ExecutableLocator>> {
this(closure)
closure(NamedDomainObjectContainerScope.of(this))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New to kotlin and trying to decipher this syntax.
I can tell closure is a function parameter and Unit means no return value, but am not sure what NamedDomainObjectContainerScope<ExecutableLocator>.() means. And what does this refer to?

Seems NamedDomainObjectContainerScope<ExecutableLocator>.() -> Unit takes no parameter, but why closure is taking a parameter on this line?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Kotlin NamedDomainObjectContainerScope<ExecutableLocator> on the 2nd line is so-called receiver.

In closure: NamedDomainObjectContainerScope<ExecutableLocator>.() -> Unit closure parameter is a kind of 'extension' method, that would be executed on the appropriate receiver.

Back from bottom to top:

// extension method needs a receiver to be executed upon. The receiver is created
// by factory method NamedDomainObjectContainerScope.of(this)
closure(NamedDomainObjectContainerScope.of(this))
// here is what _this_ in the bottom line is. It is again a 'receiver' (now in groovy sense)
// of type NamedDomainObjectContainer<ExecutableLocator>
(closureOf<NamedDomainObjectContainer<ExecutableLocator>> {

and when we look inside 'plugins' definition:

  public void plugins(Closure configureClosure) {
    ConfigureUtil.configure(configureClosure, tools.plugins)
  }

indeed, tools.plugins parameter is of type NamedDomainObjectContainer.

So, back from top to bottom. The parameter tools.plugins of type NamedDomainObjectContainer would be configured with closureOf, it becomes this in the bottom line, where it is used as a factory method parameter. Factory method instantiates an object that would be configured by closure parameter of the top line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation!

Copy link
Collaborator

@zhangkun83 zhangkun83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcoferrer did you have a chance to look at the CI failures? I can reproduce them on my local machine.

@@ -24,7 +24,7 @@ fun ProtobufConfigurator.protoc(closure: ExecutableLocator.() -> Unit) {

fun ProtobufConfigurator.plugins(closure: NamedDomainObjectContainerScope<ExecutableLocator>.() -> Unit) {
plugins(closureOf<NamedDomainObjectContainer<ExecutableLocator>> {
this(closure)
closure(NamedDomainObjectContainerScope.of(this))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation!

@marcoferrer
Copy link
Contributor Author

@zhangkun83 Sorry about the delay. Im updating the gradle version and looking into the failures now.

@marcoferrer marcoferrer force-pushed the update-to-stable-kotlin-dsl branch from eb8962b to 1ec2c22 Compare December 7, 2018 23:36
@marcoferrer marcoferrer force-pushed the update-to-stable-kotlin-dsl branch from 4b31a34 to 9d1da25 Compare December 8, 2018 04:35
@@ -83,6 +86,8 @@ dependencies {
}
testCompile 'commons-io:commons-io:2.5'

// This kotlin version needs to be compatible with the
// android plugin being used in the test runtime.
testProjectRuntime "org.jetbrains.kotlin:kotlin-gradle-plugin:1.2.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumping this dependency version is what was causing the failures in the ci. The test runtime kotlin version needs to be compatible with the older version of the android plugin. I didnt need to update it in the first place so its fine being set to 1.2.0

*
* @return [Unit]
*/
fun <T : Any> NamedDomainObjectContainerScope<T>.id(id: String, action: (T.() -> Unit)? = null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two methods arent new. I just moved them down in the file and documented their usage

@marcoferrer
Copy link
Contributor Author

@zhangkun83 CI issues have been resolved. I also updated a few things for readability and added some documentation for what I can at the moment.

Copy link
Collaborator

@zhangkun83 zhangkun83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! One minor comment then you are good to go.
It's unfortunate that we can't make it compatible with older Gradle versions, but I guess if a user doesn't bother upgrading their Gradle version, they would probably be fine sticking to the older plugin version. So we should be fine.

README.md Outdated Show resolved Hide resolved
@marcoferrer marcoferrer force-pushed the update-to-stable-kotlin-dsl branch from f999f0b to 6fe9209 Compare December 11, 2018 16:52
@marcoferrer
Copy link
Contributor Author

So I think I found a work around for the compatibility problem between 4.10 and 5.0, but I did notice that master was failing with the same issue as my branch.

Caused by: org.gradle.internal.resolve.ModuleVersionNotFoundException: Could not find com.android.tools.build:gradle:2.2.0.
    Searched in the following locations:
        https://dl.google.com/dl/android/maven2/com/android/tools/build/gradle/2.2.0/gradle-2.2.0.pom
        https://dl.google.com/dl/android/maven2/com/android/tools/build/gradle/2.2.0/gradle-2.2.0.jar
        https://jcenter.bintray.com/com/android/tools/build/gradle/2.2.0/gradle-2.2.0.pom
        https://jcenter.bintray.com/com/android/tools/build/gradle/2.2.0/gradle-2.2.0.jar
        https://plugins.gradle.org/m2/com/android/tools/build/gradle/2.2.0/gradle-2.2.0.pom
        https://plugins.gradle.org/m2/com/android/tools/build/gradle/2.2.0/gradle-2.2.0.jar

@marcoferrer marcoferrer force-pushed the update-to-stable-kotlin-dsl branch from 0f60bb0 to 587c821 Compare December 12, 2018 14:09
@marcoferrer marcoferrer force-pushed the update-to-stable-kotlin-dsl branch from 587c821 to 6028363 Compare December 12, 2018 14:12
@marcoferrer
Copy link
Contributor Author

marcoferrer commented Dec 12, 2018

Didnt make any changes but CI is working now

Copy link
Collaborator

@zhangkun83 zhangkun83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcoferrer, thank you for the excellent work! I still struggle to comprehend the Kotlin tricks. A little more elaboration would be helpful.

*
* Since the kotlin compiler creates a static implementations of all extensions, we can create a
* delegating utility function that ensures we are not inline-ing our kotlin-dsl compilation
* target byte code.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I still don't understand why this additional layer of indirection would prevent the problem. Doesn't this method inline the invoke() call too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In-lining can only be performed by the Kotlin compiler, and this is file is compiled via the java compiler. The method is resolvable from java due to Kotlins interop features. Since inline methods dont exist in java, Kotlin will create a static implementation of the method for usage from java sources.

This work around solves the compatibility issue because the method contract didnt change between ktdsl versions, only its implementation.

Copy link
Collaborator

@zhangkun83 zhangkun83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you @marcoferrer!

@zhangkun83 zhangkun83 merged commit 1a64a64 into google:master Dec 17, 2018
@alexvas
Copy link

alexvas commented Mar 11, 2019

As of plugin version of 0.8.8 I still need a pair of auxiliary functions:

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

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

to make

protobuf {
    protoc {
        // The artifact spec for the Protobuf Compiler
        artifact = Libs.protoc
    }
    plugins {
        // Optional: an artifact spec for a protoc plugin, with "grpc" as
        // the identifier, which can be referred to in the "plugins"
        // container of the "generateProtoTasks" closure.
        id("grpc") {
            artifact = Libs.protoc_gen_grpc_java
        }
    }
   generateProtoTasks {
        ofSourceSet("main").forEach {
            it.plugins {
                // Apply the "grpc" plugin whose spec is defined above, without
                // options.  Note the braces cannot be omitted, otherwise the
                // plugin will not be added. This is because of the implicit way
                // NamedDomainObjectContainer binds the methods.
                id("grpc")
            }
        }
    }
}

work. Otherwise there is a gradle configuration error:

  Line 31:         id("grpc") {
                              ^ Too many arguments for public open fun id(id: String): PluginDependencySpec defined in org.gradle.kotlin.dsl.PluginDependenciesSpecScope

  Line 32:             artifact = Libs.protoc_gen_grpc_java
                       ^ Unresolved reference: artifact

2 errors

using Gradle 5.2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kotlin DSL does not compile with Gradle 5.0-rc-3
3 participants