dokka icon indicating copy to clipboard operation
dokka copied to clipboard

Use buildSrc convention plugins to configure the Dokka subprojects

Open aSemy opened this issue 3 years ago • 5 comments

Basic groundwork for #2703

aSemy avatar Oct 11 '22 17:10 aSemy

I made some good progress but then I got stuck when I reached the integration test config, because it's a bit of a beast.

https://github.com/Kotlin/dokka/blob/83174361becb2af227159834cdf6e14db9300c53/integration-tests/build.gradle.kts

I think most of the config in here should be converted to a specific 'integration test' buildSrc plugin.

aSemy avatar Oct 11 '22 17:10 aSemy

@IgnatBeresnev I see you've added the 'Gradle Plugin' label, and while this PR would help with that, it's more about the improving the general Gradle build config of the entire project :)

aSemy avatar Oct 12 '22 20:10 aSemy

Yeah, true. I don't want to create too many labels, but I definitely want to mark all Gradle-related PRs in case we ask for anyone's help with review and whatnot

If it makes anyone uneasy, we can certainly create a separate one or rename the existing. We'll discuss it with the team this week, just in case

IgnatBeresnev avatar Oct 12 '22 20:10 IgnatBeresnev

With regards to labels, what about a 'build config' or 'ci/cd' label that would cover the build scripts, and also GitHub Actions, Qodana, TeamCity etc?

aSemy avatar Oct 13 '22 11:10 aSemy

I've made some good progress! All shared build-config now comes from buildSrc convention plugins, not from subprojects{} or allprojects{} blocks. This makes the build much more modular, because it's not restricted by inheritance, and easier to configure because a build script is more self contained - it doesn't have config applied from some arbitrary parent.

aSemy avatar Oct 13 '22 20:10 aSemy

After researching #2652, I think I now finally understand why the proposed changes are good and what they do!

However, if we first merge #2652 - it would introduce conflicts. @aSemy would you be OK with that? Looks like you would be able to just move the files over into build-logic.

IgnatBeresnev avatar Jan 30 '23 22:01 IgnatBeresnev

Either merging this PR or #2652 first would work, but since there are fewer changes in #2652, and converting buildSrc to an included build is a more 'logical' upgrade, I think it would be best if this PR were merged first, and either #2652 is updated, or we create a new similar PR

aSemy avatar Jan 30 '23 22:01 aSemy

@aSemy Rebasing?

Goooler avatar Feb 22 '23 05:02 Goooler

@Goooler thanks for the review! I'm also taking a look now

IgnatBeresnev avatar Feb 23 '23 02:02 IgnatBeresnev

Found a minor problem when comparing two versions:

The configuration for subprojects {} was extracted into the kotlin-jvm convention. Previously, it was applied to the gradle plugin as well, whereas now it's not. This leads to:

  • KotlinCompile configuration is not applied for the Gradle plugin, so if you compare the .module files for the plugin you'll see that "org.gradle.jvm.version": 8 was changed to 11
  • The sources for the gradle plugin are not published. I'll leave a comment on the line that I think causes it

The rest seems to work fine :)

[Optional] Dokka used for publishing used to be 1.7.20, now it's 1.7.10 - got a bit confused when comparing published javadocs :) I'll update the versions after the release anyway, so it's ok to skip it

IgnatBeresnev avatar Feb 27 '23 17:02 IgnatBeresnev

Hi! Not sure if you noticed that the integration tests for coroutines are unable to be run because of the toolchain being set to 1.8 globally. This refactoring has uncovered quite a number of problems already, which is good, I guess :)

A problem occurred evaluating root project 'kotlinx.coroutines'.
> Failed to apply plugin 'jdk-convention'.
> Project required JDK 11+, but found 1.8

Can you please fix it when you have the time, so that the checks pass? I believe that's the last thing holding up the merge

Not sure what the best solution for the fix is, but I'd say any that builds the same bytecode as before and makes the tests pass. We can revisit our requirements for the used java/kotlin versions later and separately

IgnatBeresnev avatar Mar 07 '23 15:03 IgnatBeresnev

thanks for checking @IgnatBeresnev, I didn't realise that was the problem.

I set the toolchain for Dokka to use Java 8 because that matches the existing jvmTarget

https://github.com/Kotlin/dokka/blob/ded804e5772399f1495016d598573cb20b673b58/build.gradle.kts#L33

If I set the toolchain to 11, then the :runners:maven-plugin build fails:

> Task :runners:maven-plugin:helpMojo
[INFO] Error stacktraces are turned on.
[INFO] Scanning for projects...
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] Building dokka-maven-plugin 1.8.20-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- maven-plugin-plugin:3.5.2:helpmojo (default-cli) @ dokka-maven-plugin ---
> Task :plugins:base:frontend:nodeSetup
> Task :plugins:base:frontend:npmSetup SKIPPED
> Task :runners:maven-plugin:helpMojo
[WARNING] Using platform encoding (UTF-8 actually) to read mojo source files, i.e. build is platform dependent!
[INFO] java-javadoc mojo extractor found 0 mojo descriptor.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 2.425 s
[INFO] Finished at: 2023-03-07T16:44:34+01:00
[INFO] Final Memory: 12M/68M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-plugin-plugin:3.5.2:helpmojo (default-cli) on project dokka-maven-plugin: Execution default-cli of goal org.apache.maven.plugins:maven-plugin-plugin:3.5.2:helpmojo failed: Unsupported class file major version 55 -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-plugin-plugin:3.5.2:helpmojo (default-cli) on project dokka-maven-plugin: Execution default-cli of goal org.apache.maven.plugins:maven-plugin-plugin:3.5.2:helpmojo failed: Unsupported class file major version 55

And there's a warning for :runners:gradle-plugin

> Task :runners:gradle-plugin:compileTestKotlin
'compileTestJava' task (current target is 11) and 'compileTestKotlin' task (current target is 1.8) jvm target compatibility should be set to the same Java version.
By default will become an error since Gradle 8.0+! Read more: https://kotl.in/gradle/jvm/target-validation
Consider using JVM toolchain: https://kotl.in/gradle/jvm/toolchain

It looks like kotlinx.coroutines has a custom check for the JDK version...

https://github.com/Kotlin/kotlinx.coroutines/blob/4116d4a178f32b1241db8247a38cd2823fb8b03e/buildSrc/src/main/kotlin/jdk-convention.gradle.kts#L7-L14

I'll look into it a little more. Perhaps the kotlinx.coroutines int. test requires a little more config.

aSemy avatar Mar 07 '23 15:03 aSemy

I set the toolchain for Dokka to use Java 8 because that matches the existing jvmTarget

Yeah, that's the most important part - we cannot break compatibility with Java 8 for now.

We can set jvmTarget to 8 in the kotlin convention + the gradle plugin, but run the whole project with java 11 - I don't mind that. Adding the toolchain is definitely a good idea, even if jvmTarget stays at 8

But whatever works is fine :) We can revisit the used versions later

Tell me once it's ready to be looked at, I'll trigger the teamcity build

IgnatBeresnev avatar Mar 07 '23 16:03 IgnatBeresnev

@IgnatBeresnev Do you know why the jvm-version is set to 12 in the example workflows?

https://github.com/Kotlin/dokka/blob/2e78b7b79579ee6ed502e2985cbfdf5bb701abee/.github/workflows/gh-actions-artifacts-snapshots.yml#L21

aSemy avatar Mar 07 '23 16:03 aSemy

We can set jvmTarget to 8 in the kotlin convention + the gradle plugin, but run the whole project with java 11 - I don't mind that. Adding the toolchain is definitely a good idea, even if jvmTarget stays at 8

Toolchains don't let you separately define a language version; the JVM target is set to that of the Java version of the Toolchain. So if the project needs to target Java 8, the Toolchain needs to be Java 8. Don't ask me why jvmTarget can't be set separately...

Fortunately Toolchains are very flexible, so I set the integrationTest task launcher to be Java 11.

https://github.com/Kotlin/dokka/pull/2704/commits/1bbbd9b67a86ec81106f4f64f9c3c5d4e082c663

@IgnatBeresnev Tests have passed, it's ready for the TeamCity build!

aSemy avatar Mar 07 '23 16:03 aSemy

@IgnatBeresnev Do you know why the jvm-version is set to 12 in the example workflows?

It was copied from s3-snapshots.yml, but why it's set there to 12 - zero clue :( I think it's fine to set it to 11 like you did

@IgnatBeresnev Tests have passed, it's ready for the TeamCity build!

Thank you once again! Triggered the builds, will merge once they pass :)

IgnatBeresnev avatar Mar 07 '23 17:03 IgnatBeresnev

@IgnatBeresnev By the way, good job on the GitHub labels! They look really smart, and they're helpful.

aSemy avatar Mar 08 '23 10:03 aSemy

@aSemy yeah, going through all of the issues :) Part of the reason is actually for the dokkatoo DSL. In particular, the configuration and the Gradle plugin tags could be of interest to go along with the re-write since we're breaking compatibility anyway

IgnatBeresnev avatar Mar 08 '23 13:03 IgnatBeresnev

Hi @aSemy. I've noticed that after this change the artifact dokka-analysis started publishing an additional shadowRuntimeElements variant with the following attributes:

    {
      "name": "shadowRuntimeElements",
      "attributes": {
        "org.gradle.category": "library",
        "org.gradle.dependency.bundling": "shadowed",
        "org.gradle.libraryelements": "jar",
        "org.gradle.usage": "java-runtime"
      },
      "files": [
        {
          "name": "dokka-analysis-1.8.20-local-1-all.jar",
          "url": "dokka-analysis-1.8.20-local-1-all.jar",
          "size": 100899229,
          ...
        }
      ]
    }

This is unfortunate, because it causes variant resolution ambiguity with the runtimeElements variant in ...Plugin configurations of manually created DokkaTaskPartial tasks in the stdlib docs build.

Is this an expected consequence of this change?

  • if no, could you investigate why it has happened and how to stop publishing this variant?
  • if yes, could you provide a guidance how to overcome this variant ambiguity in a project?

ilya-g avatar Apr 06 '23 19:04 ilya-g

I will take a look! I'm not very familiar with the Shadow plugin, but it looks like there are a few issues relating to shadowRuntimeElements https://github.com/johnrengelman/shadow/issues?q=shadowRuntimeElements

What might be a quick fix is disabling publication of this specific configuration, with a similar way to how Test Fixtures publication can be disabled.

val javaComponent = components["java"] as AdhocComponentWithVariants
javaComponent.withVariantsFromConfiguration(configurations["testFixturesApiElements"]) { skip() }
javaComponent.withVariantsFromConfiguration(configurations["testFixturesRuntimeElements"]) { skip() }

aSemy avatar Apr 06 '23 20:04 aSemy