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
Kotlin DSL assignment plugin integration #22240
Conversation
cb085ca
to
e8c8512
Compare
subprojects/base-services/src/main/java/org/gradle/api/SupportsKotlinAssignment.java
Outdated
Show resolved
Hide resolved
subprojects/base-services/src/main/java/org/gradle/api/SupportsKotlinAssignment.java
Outdated
Show resolved
Hide resolved
...in-dsl-plugins/src/main/kotlin/org/gradle/kotlin/dsl/plugins/dsl/KotlinDslCompilerPlugins.kt
Outdated
Show resolved
Hide resolved
746b359
to
47617ac
Compare
58329b3
to
d31f817
Compare
00eac71
to
7344c4a
Compare
subprojects/kotlin-dsl/src/main/kotlin/org/gradle/kotlin/dsl/KotlinBuildScript.kt
Outdated
Show resolved
Hide resolved
subprojects/kotlin-dsl/src/main/kotlin/org/gradle/kotlin/dsl/KotlinInitScript.kt
Outdated
Show resolved
Hide resolved
subprojects/kotlin-dsl/src/main/kotlin/org/gradle/kotlin/dsl/KotlinSettingsScript.kt
Outdated
Show resolved
Hide resolved
87ca3ad
to
3348b46
Compare
2c01741
to
46acdf5
Compare
...in-dsl-plugins/src/main/kotlin/org/gradle/kotlin/dsl/plugins/dsl/KotlinDslCompilerPlugins.kt
Show resolved
Hide resolved
subprojects/kotlin-dsl/src/main/kotlin/org/gradle/kotlin/dsl/KotlinAssignmentExtensions.kt
Outdated
Show resolved
Hide resolved
@@ -73,7 +74,8 @@ import kotlin.script.templates.ScriptTemplateDefinition | |||
"-Xjsr305=strict", | |||
"-XXLanguage:+DisableCompatibilityModeForNewInference", | |||
"-XXLanguage:-TypeEnhancementImprovementsInStrictMode", | |||
] | |||
], | |||
provider = KotlinBuildScriptTemplateAdditionalCompilerArgumentsProvider::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.
❔ I guess there's no way to extract these annotations to a super-class, right?
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.
Forgot to answer this one: I tried it, but it seems it doesn't work with a super-class.
.../kotlin-dsl/src/main/kotlin/org/gradle/kotlin/dsl/assignment/internal/KotlinDslAssignment.kt
Outdated
Show resolved
Hide resolved
...sl/src/main/kotlin/org/gradle/kotlin/dsl/precompile/PrecompiledScriptDependenciesResolver.kt
Show resolved
Hide resolved
@@ -376,7 +379,7 @@ class CompileKotlinScript( | |||
identityFileInputs: MutableMap<String, CurrentFileCollectionFingerprint> | |||
): UnitOfWork.Identity { | |||
val identityHash = newHasher().let { hasher -> | |||
listOf("templateId", "sourceHash", "compilationClassPath", "accessorsClassPath").forEach { | |||
listOf("assignmentOverloadEnabled", "templateId", "sourceHash", "compilationClassPath", "accessorsClassPath").forEach { |
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.
🤔 Would be nice to move these strings out into constants to avoid typos.
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.
Moved to constants. Also I see identity hash doesn't contain jvmTarget, I guess we should add it? /cc @eskatos
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.
It might be we missed to add it to the identity hash
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.
Maybe it would be worth extracting this listOf()
as a constant close to the declaration of the other constants to avoid missing another property later?
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.
Moved the list to constants
* limitations under the License. | ||
*/ | ||
|
||
@file:Suppress("DEPRECATION") |
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.
❓ Why is this necessary? If we need it, let's document why (and for how long).
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.
We implemented deprecated ScriptTemplateAdditionalCompilerArgumentsProvider since there is no good replacement yet. Added a comment.
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.
Thanks.
🧐 This should not be a Javadoc comment, but instead //
.
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
...src/integTest/kotlin/org/gradle/kotlin/dsl/integration/KotlinDslAssignmentIntegrationTest.kt
Outdated
Show resolved
Hide resolved
...src/integTest/kotlin/org/gradle/kotlin/dsl/integration/KotlinDslAssignmentIntegrationTest.kt
Outdated
Show resolved
Hide resolved
...in-dsl-plugins/src/integTest/kotlin/org/gradle/kotlin/dsl/plugins/dsl/KotlinDslPluginTest.kt
Outdated
Show resolved
Hide resolved
799f878
to
16de9c6
Compare
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.
LGTM, left one small comment, decide if it's worth addressing.
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.
We should emit a message saying that Kotlin DSL property assignment is an incubating feature
.
About adding more assignment extensions, did you look at what Groovy does for ConfigurableFileCollection
and ListProperty
& co?
// Kotlin 1.8.0 has wrong warning message for assign plugin, | ||
// replace with 'Assign Kotlin compiler plugin is an experimental feature' once we update to 1.8.20. | ||
// https://github.com/JetBrains/kotlin/commit/0eb34983cb38064684cfc76dacb8d4460fcc6573 | ||
"Lombok Kotlin compiler plugin is an experimental feature", |
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.
❔ We could consider adding both messages already.
When 1.8.20 will be out, early adopters will use it before we get a chance to upgrade.
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.
I planned to add other extension methods in follow-up PR, but I can do that now.
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.
More assignment extensions could go into another PR, me thinks. It was just me being curious and wanting to make sure we don't forget.
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.
+1 to keeping this small and instead creating issues to make sure we don't forget adding more stuff.
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.
@lptr are you referring to my original comment in this thread about the compiler warning or to the other extension methods subject?
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.
I meant the extension methods.
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.
Added additional message, also I added incubating message. Message is triggered from assign
methods.
…ubating log message
172f1af
to
be36048
Compare
@bot-gradle test this |
I've triggered the following builds for you: |
@bot-gradle test and merge |
I've triggered a build for you. |
🎉 |
So we can dogfood #22240. Co-authored-by: Anže Sodja <asodja@gradle.com>
This adds the Kotlin assignment plugin to the Gradle.
Part of #9268.
TODO:
kotlin-dsl
Gradle plugin with support for the Kotlin compiler assignment plugin: done as part of Update kotlin-dsl plugin version #23529