-
Notifications
You must be signed in to change notification settings - Fork 67
Split signature-generator and Gradle plugin into separate projects #124
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
Conversation
30b7e24 to
4366602
Compare
- bump Gradle Plugin Publish Plugin version - build script updates & improvements - use Java Toolchains to set the Java version - use jvm-test-fixtures plugin for functionalTest sources - update build config, remove redundant Maven publication
b5e0510 to
3bec20a
Compare
- bump Gradle Plugin Publish Plugin version - build script updates & improvements - use Java Toolchains to set Java version - use jvm-test-fixtures plugin for functionalTest sources
- create convention plugins for common and java-specific config - add Gradle version catalog - simplify some build config
…ects - create new convention plugins - kotlin-gradle-plugin, for building Gradle plugins written in Kotlin - kotlin-jvm, for building Kotlin/JVM projects - maven-publishing, for sharing common maven-publish settings - update gitignore, because `build/` dirs might be nested, and to include common IDE/OS property/language files
# Conflicts: # build.gradle.kts
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.
The configuration in this file was moved to buildSrc convention plugins, so that it could be shared between the two new subprojects as appropriate.
| groovy.setSrcDirs(emptyList<String>()) | ||
| implementation("org.gradle.kotlin:gradle-kotlin-dsl-plugins:$expectedKotlinDslPluginsVersion") { | ||
| because("allows applying the `kotlin-dsl` / `embedded-kotlin` plugins in buildSrc convention plugins") | ||
| } |
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.
Sorry, I know you like removing the unnecessary src/main dirs, but in this case I thought it was extra configuration that didn't really bring much. Given how Gradle can be quite fragile, and how IntelliJ has tenuous support of buildSrc as it is, I thought it was better remove anything not strictly necessary.
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 brings a flat folder structure that we would like to embrace everywhere and get rid of the legacy nested format. Dogfooding and reporting all the problems in the process in an important part here.
I'm okay with this change and can tweak it later after all the merges though
- create convention plugins for common and java-specific config - add Gradle version catalog - simplify some build config
* The ABI shape will be reduced in the future, this is a technical change
|
Sorry for the delay here, it's a planning time in Kotlin, so I fail to spare a decent chunk of continuous time to get back on it. |
|
No probs. This is a big change so take your time 👍 |
|
I've asked my colleague to help with the Gradle part, he mentioned the following potential caveats:
|
Gradle Test Kit has been set to use Gradle 7.4.2 and the tests seems to work fine (although I don't really understand how or why...). binary-compatibility-validator/src/functionalTest/kotlin/kotlinx/validation/api/TestDsl.kt Line 34 in 14bd77c
Ah okay, interesting, thanks! And also, now that I think about it, it has to be the case. Lots of Gradle plugins depend on non-plugin dependencies, and they work fine. BCV depends on the Kotlin stdlib already, and that must come from Maven Central.
Yes, good point. I expect that any problems would be picked up by the Gradle Test Kit test. Also, any further incompatibility problems would be reduced in the future by my plan to use a Gradle Worker to run |
14bd77c to
3d46759
Compare
qwwdfsad
left 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.
Thank you, and sorry for the delay!
I gave it one more look, and it seems good in general
I have a few questions and an issue with non-conventional Gradle DSL and Maven Coordinates issue and I think it will be good to go
| groovy.setSrcDirs(emptyList<String>()) | ||
| implementation("org.gradle.kotlin:gradle-kotlin-dsl-plugins:$expectedKotlinDslPluginsVersion") { | ||
| because("allows applying the `kotlin-dsl` / `embedded-kotlin` plugins in buildSrc convention plugins") | ||
| } |
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 brings a flat folder structure that we would like to embrace everywhere and get rid of the legacy nested format. Dogfooding and reporting all the problems in the process in an important part here.
I'm okay with this change and can tweak it later after all the merges though
| } | ||
|
|
||
| tasks.validatePlugins { | ||
| //enableStricterValidation.set(true) // TODO enable validation |
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.
Could you please elaborate on when it's okay to enable it to what the blockers are?
|
|
||
|
|
||
| // the signing plugin must be configured *after* publications are created | ||
| afterEvaluate { |
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.
Could you please elaborate on why we have to rely on afterEvaluate even though previously it was not the case?
I'm a bit concerned here as the afterEvaluate block always causes troubles in the long run, as more and more workarounds start piling there (esp. with AGP present)
|
|
||
|
|
||
| @Suppress("UnstableApiUsage") | ||
| gradlePlugin { |
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 gave local publication a go:
-
On conventionally-configured Gradle projects with Kotlin DSL and
pluginseverything is really smooth -
On groovy DSL old-way (
kotlinx.coroutines😄 ) I failed to depend on the plugin:
- The plugin is published to
~/.m2/repository/org/jetbrains/kotlinx/binary-compatibility-validator/org.jetbrains.kotlinx.binary-compatibility-validator.gradle.plugin/which cannot be dependent on as is - I've checked out the resulting
POM, and its coordinates are the following:
<groupId>org.jetbrains.kotlinx</groupId>
<artifactId>gradle-plugin</artifactId>
<version>0.14.0-qtest</version>
Two points here:
gradle-plugin is not really a good name inkotlinxnamespace, I suggest renaming it to something more specific, e.g.bcv-gradle-pluginorbinary-compatibility-validator-gradle-plugin`- Figuring this out is hard and (we already had this in Kover) will lead to a lot of questions "how to upgrade with
classpathdependency"; let's publish maven relocation information
|
Also, if it works for you, could you please update Kotlin to 1.9.0 alongside this change? |
As part of preparatory refactoring for #88, this PR splits up the signature-generator project and Gradle-plugin into two separate projects.
Description
Because Gradle plugins must use Gradle's embedded version of Kotlin, splitting up the projects means the BCV Gradle plugin can be built using/targetting Gradle's embedded Kotlin, while the signature-generator can be built using another version of Kotlin.
It also means that the BCV Gradle plugin can use the
kotlin-dslplugin, which adds some Gradle specific configuration to help plugin development, but is not necessary in the signature-generator.Splitting up the projects also helps prepare for using the Gradle Worker API, so when the BCV Gradle plugin runs the signature-generator it can specifically control the dependencies used, and potentially default to
kotlinx-metadatadependency provided by KGP (if present).Summary of changes
./modulesdir. It's a personal convention that I use a lot of my projects, because it's nice that the built libraries are together under one directory and not mixed up with other build/gradle/IDE/git directories.Breaking changes
There should be no functional changes from this PR - the plugin and library should work exactly as before, however
plugins {}DSL just need to bump the version.gradlePluginPortal(), the BCV Gradle plugin won't be able to fetch the signature-generator by defaultTODO
UPDATE: not an issue - GPP proxies Maven Central
Because the signature-generator can only be published to Maven Central, and the default repository for plugins is
gradlePluginPortal(), the BCV Gradle plugin won't be able to fetch the signature-generator by default. There are a few ways around this:mavenCentral()as a plugin repoThere are now two new artifacts, and the old one will continue to exist. Do you want to publish Maven relocation information?
Dependencies
This PR depends on