-
Notifications
You must be signed in to change notification settings - Fork 411
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
Gradle 8.0 #2860
Gradle 8.0 #2860
Conversation
https://github.com/Kotlin/dokka/actions/runs/4171267730/jobs/7221031783
This issue of Shadow has been filed at GradleUp/shadow#820. |
|
https://github.com/Kotlin/dokka/actions/runs/4171517091/jobs/7221535291
|
This failure is from the example project and requires the updated version of Dokka (1.8.10, not released yet) |
Thanks, I'll continue some work after 1.8.10 released. |
examples/gradle/dokka-customFormat-example/gradle/wrapper/gradle-wrapper.properties
Outdated
Show resolved
Hide resolved
…le-wrapper.properties Co-authored-by: Matthew Haughton <[email protected]>
@aSemy |
tasks.withType<KotlinCompile>().configureEach { | ||
compilerOptions { | ||
apiVersion.set(KotlinVersion.fromVersion("1.4")) | ||
languageVersion.set(KotlinVersion.fromVersion("1.4")) | ||
} | ||
} | ||
|
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.
hey @Goooler, thanks for the ping
Gradle 8 bumped the Kotlin language level to 1.8
Also, the kotlin-dsl
plugin will automatically configure the language level (as well as other stuff), so there's no need to set it manually.
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.
tasks.withType<KotlinCompile>().configureEach { | |
compilerOptions { | |
apiVersion.set(KotlinVersion.fromVersion("1.4")) | |
languageVersion.set(KotlinVersion.fromVersion("1.4")) | |
} | |
} |
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.
Cause there are some outdated APIs used, seems we can't elevate API level to 1.8 for now, that's why I declare the overriding here.
https://github.com/Kotlin/dokka/actions/runs/4344123199/jobs/7587028523
https://github.com/Kotlin/dokka/actions/runs/4344122470/jobs/7587025547
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.
Ah right, okay.
The deprecations look like minor things to fix
e: warnings found and -Werror specified
w: file:///home/runner/work/dokka/dokka/runners/gradle-plugin/src/main/kotlin/org/jetbrains/dokka/gradle/DokkaPlugin.kt:109:63 'decapitalize(): String' is deprecated. Use replaceFirstChar instead.
w: file:///home/runner/work/dokka/dokka/runners/gradle-plugin/src/main/kotlin/org/jetbrains/dokka/gradle/dokkaDefaultOutputDirectory.kt:11:59 'decapitalize(): String' is deprecated. Use replaceFirstChar instead.
w: file:///home/runner/work/dokka/dokka/runners/gradle-plugin/src/main/kotlin/org/jetbrains/dokka/gradle/kotlin/isMainSourceSet.kt:26:34 'toLowerCase(): String' is deprecated. Use lowercase() 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.
Yeah, I did, but more things in https://github.com/Kotlin/dokka/actions/runs/4343604835/jobs/7585857751
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.
Here detekt/detekt#1286
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've been bitten by using kotlin-dsl in a plugin before, because the plugin was being used in versions of Gradle that were older than the version used to compile the plugin. I'd suggest reverting 2702 and avoiding use of
kotlin-dsl
in the plugin itself.
The kotlin-dsl
plugin just adds some sensible defaults, removing it won't help - those defaults would have to be added back in and it would make the build config more complicated and brittle. So my guess is the problems you're referring to, that detekt/detekt#1286 solved, would be caused by mis-configuration (e.g. adding a runtime dependency on gradleApi()
), or it using the Gradle embedded Kotlin. (Both of those problems seem have been resolved in newer Dekect plugin versions.)
In any case, it's not relevant to this PR.
Do I understand correctly that the old version of AGP is holding us from updating? That is, AGP 4.0.1 is not compatible with Gradle 8?
It's unclear. That's what the test failure seemed to suggest, but it doesn't make sense to me. The Android dependency shouldn't be 'aware' of the Gradle version†, so why should there be a missing class just because the Gradle version is different?
I'll do some digging today. It might be that we make DGP variants that are aware of the Gradle version, which on one hand is cool but on the other it will be a pain.
† While it is possible to publish plugins variants that are aware of the Gradle version that say "I'm only compatible with Gradle 7.6", so Gradle won't select them. But that's not the error (it would be a dependency resolution error), and AGP is older than the Gradle version that introduced this 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.
Thanks for the context on kotlin-dsl
! We'll keep an eye on it. So far it's not causing problems and I hope the integration tests cover it well
It's unclear. #2860 (comment), but it doesn't make sense to me.
Yeah, that's what I can't understand as well, the error is quite strange, considering the changes :( Unless AGP was using some Gradle API whch was removed/reworked
Thank you for taking a look!
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 actual cause of the test failure was hidden in the GitHub logs. I reproduced it locally, and here's the key parts of the stacktrace:
org.gradle.api.ProjectConfigurationException: A problem occurred configuring root project 'android-auto-configuration-test'.
Caused by: org.gradle.internal.event.ListenerNotificationException: Failed to notify project evaluation listener.
Caused by: java.lang.NoClassDefFoundError: org/gradle/api/plugins/MavenPlugin
at com.android.build.gradle.internal.variant.VariantHelper.setupArchivesConfig(VariantHelper.java:47)
at com.android.build.gradle.internal.LibraryTaskManager.createBundleTask(LibraryTaskManager.java:396)
Caused by: java.lang.ClassNotFoundException: org.gradle.api.plugins.MavenPlugin
The org.gradle.api.plugins.MavenPlugin
class was moved and marked as deprecated in Gradle 7, and now it's removed in Gradle 8 gradle/gradle#22807
So yeah, Android-GP 4.0.1 isn't compatible with Gradle 8 (I also checked with 4.1.3 - same problem).
However, I think the test failure is caused by the test not creating a realistic scenario. It uses org.gradle.testfixtures.ProjectBuilder
, which is intrinsically linked to the Gradle version used to build Dokka.
Lines 13 to 20 in 14c05d7
private val project = ProjectBuilder.builder().build().also { project -> | |
project.plugins.apply("com.android.library") | |
project.plugins.apply("org.jetbrains.kotlin.android") | |
project.plugins.apply("org.jetbrains.dokka") | |
project.extensions.configure<LibraryExtension> { | |
compileSdkVersion(28) | |
} | |
} |
In real life that's not going to happen - Android-GP would have to use Gradle 7 or lower.
As I see it, the workaround is to re-write AndroidAutoConfigurationTest
to use Gradle TestKit, which Dokka already has set up, and to specify the Gradle version to be 7 or lower for AndroidAutoConfigurationTest
.
Lines 29 to 49 in 14c05d7
fun createGradleRunner( | |
vararg arguments: String, | |
jvmArgs: List<String> = listOf("-Xmx4G", "-XX:MaxMetaspaceSize=2G") | |
): GradleRunner { | |
return GradleRunner.create() | |
.withProjectDir(projectDir) | |
.forwardOutput() | |
.withJetBrainsCachedGradleVersion(versions.gradleVersion) | |
.withTestKitDir(File("build", "gradle-test-kit").absoluteFile) | |
.withArguments( | |
listOfNotNull( | |
"-Pkotlin_version=${versions.kotlinVersion}", | |
"-Pdokka_it_kotlin_version=${versions.kotlinVersion}", | |
versions.androidGradlePluginVersion?.let { androidVersion -> | |
"-Pdokka_it_android_gradle_plugin_version=$androidVersion" | |
}, | |
* arguments | |
) | |
).run { this as DefaultGradleRunner } | |
.withJvmArguments(jvmArgs) | |
} |
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.
Nice find! It indeed doesn't look like it's Dokka's problem in this case, since we do support AGP 4.X, but AGP 4.X itself is not compatible with Gradle 8 - not Dokka's fault
Refactoring the test to utilize TestKit would be good, preferably in a separate PR. Although I'm not sure if it'll be possible to check the same things as in AndroidAutoConfigurationTest
- it requires access to the configured Dokka tasks. Maybe it could be done as verify tasks inside of the test project? We actually already have an integration test for Android - Android0GradleIntegrationTest
)
An additional task for this PR: the tested Gradle versions should be updated to include Gradle 8, and possibly drop older versions? Lines 3 to 50 in 14c05d7
|
Indeed, Gradle 8 should be added for sure 👍 |
Addressing to #2960. |
https://docs.gradle.org/8.0/release-notes.html
https://docs.gradle.org/8.0/userguide/upgrading_version_7.html#changes_8.0