Remove the dependency on internal Gradle API

[IgnatBeresnev]

Dokka depends on classes from org.gradle.api.internal for which there are no guarantees about backward compatibility and proper deprecation as demonstrated by #2816

It looks like TaskDependencyInternalWithAdditions can be replaced with Gradle's dependsOn for setting up task dependencies.

[atyrin]

For DokkaMultiModuleTask and DokkaCollectorTask tasks Dokka has API methods for adding and removing child tasks (addChildTask(s)/removeChildTask(s), addSubprojectChildTasks/removeSubprojectChildTasks ). These methods may accept path/task/project as a parameter. The list of such child tasks is stored as a Set<String> of paths in the parent task (childDokkaTaskPaths in AbstractDokkaParentTask).
Child task == task on which the parent task depends.

This internal storage of child tasks is used in cases when the list of all dependency tasks is requested with the getTaskDependencies(): TaskDependencyInternal. That's the place where internal packages are used -- to override the method and access to TaskDependencyInternal. Also, it is used as a list of tasks that are confidently Dokka-related.

We can eliminate the logic and replace the internal storage with dependsOnfrom DefaultTask. That will remove the requirement to override getTaskDependencies.
But there are disadvantages:

1. This is a Set<Object> that is not safe.
2. It is not allowed to use remove method on the set. Only the full re-write with setDependsOn.
3. dependsOn is a container that accepts any user input, so we have to expect anything inside.
   Together, they lead to the necessity of writing a little of extra code for checking types/filtering.

[aSemy]

This is related to #2700

Currently the Dokka Gradle plugin uses task references to access generated docs between subprojects, which is not recommended by Gradle

Don’t reference other project tasks directly

A frequent anti-pattern to declare cross-project dependencies is:

dependencies {
   // this is unsafe!
   implementation project(":other").tasks.someOtherJar
}

This publication model is unsafe and can lead to non-reproducible and hard to parallelize builds. This section explains how to properly create cross-project boundaries by defining "exchanges" between projects by using variants.

Directly referencing other tasks from other projects causes problems with the Gradle Configuration Cache #2231 and prevents docs aggregation #2612.

To migrate to the recommended Gradle method of sharing output between subprojects, the Dokka Gradle Plugin should

1. For each Dokka-enabled subproject create appropriate Configurations for providing and consuming generated documentation
2. The providing Configuration will be filled using the docs generation task
3. The consuming Configuration will add dependencies on other subprojects.

Then, the aggregation task can use the consuming configuration as a task input. Gradle will automatically retrieve the docs, and trigger the docs generation tasks as needed.

There is a practical example, based on aggregating JaCoCo test reports, in the Gradle docs: https://docs.gradle.org/7.3/samples/sample_jvm_multi_project_with_code_coverage.html

[IgnatBeresnev]

@aSemy indeed, what you're proposing sounds very reasonable, and would help with the mentioned issues 👍

As you've probably understood by now, we are not Gradle experts and there are many issues with the plugin, so implementing and reviewing it will take time.

At the moment I'm concerned by the fact that the internal API can be removed/changed in any release, which can limit our compatibility with the previous/future versions of Gradle. So I'd like to first try to get rid of the internal API by using the stable dependsOn alternative.

After that, the more general issue of task reference/dependency can be addressed. I believe there's some discussion about it in #1752

Do you see any problems with this approach or maybe you have some concerns that we don't see? I don't think any of your current PRs cover this problem, right?

[aSemy]

Do you see any problems with this approach or maybe you have some concerns that we don't see?

I'm not saying "don't do it", because pragmatically trying to patch the existing setup is less work than trying overhaul it. The critique I have is that replacing an internal API with a misuse of the task dependency API doesn't really help the Dokka Gradle plugin - it's still incompatible with projects that follow the Gradle spec. I want to use it, but I can't.

I don't think any of your current PRs cover this problem, right?

All of the Gradle Plugin PRs have been building up to fixing this :)

[IgnatBeresnev]

The critique I have is that replacing an internal API with a misuse of the task dependency API doesn't really help the Dokka Gradle plugin - it's still incompatible with projects that follow the Gradle spec

For sure, it's not meant to be a replacement for the proper fix. Replacing TaskDependencyInternal with dependsOn doesn't improve things from the outside, the plugin is still incompatible, but at least it will be stable and a new Gradle release will not break it. I want to roll it out in the nearest release, because if things do break 2 days after the Dokka release - it can be very long until the next Dokka release, and Dokka cannot publish minor bugfix releases on its own, since we follow Kotlin's versioning.

[IgnatBeresnev]

@aSemy we have a question, maybe you can help us and explain what the proper way of doing it is

AbstractDokkaParentTask has a lot of methods like addChildTask, removeChildTask, removeSubprojectChildTasks, etc. Right now they all work because the parent task has a list of child tasks inside of it, so we can add to it and remove from it.

When it's refactored the proper way with Configurations, I understand that it will be impossible to support these methods (addChildTask, removeChildTask, etc), so we will have to delete them. Am I correct?

If we'll have to delete them, it might break some user projects, for instance in this case.

So we want to deprecate these methods now with warning, to give people time to migrate. But we don't know what solution to propose instead. Or is there none? How is this situation usually handled the idiomatic way?

[aSemy]

For sure, it's not meant to be a replacement for the proper fix. Replacing TaskDependencyInternal with dependsOn doesn't improve things from the outside, the plugin is still incompatible, but at least it will be stable and a new Gradle release will not break it. I want to roll it out in the nearest release, because if things do break 2 days after the Dokka release - it can be very long until the next Dokka release, and Dokka cannot publish minor bugfix releases on its own, since we follow Kotlin's versioning.

Yes, That makes sense, so if a quick-fix works for now then go for it. Although the Dokka Gradle plugin shouldn't be so closely tied to the Dokka generator version.

Currently the Dokka Gradle plugin is in lock-step with the Dokka project, but really the Dokka Gradle plugin basically a fancy wrapper to help create the configuration that is supplied to the actual Dokka generator. It's not even like it's a wrapper for the Dokka CLI (which embeds the Dokka generator) - it's a step removed.

That's why #2740 is important - it breaks the link between Dokka Gradle Plugin and Dokka Generator. If that's done, then there shouldn't be any problem with quickly releasing a new Dokka Plugin, even if Dokka Generator isn't changed.

When it's refactored the proper way with Configurations, I understand that it will be impossible to support these methods (addChildTask, removeChildTask, etc), so we will have to delete them. Am I correct?

Correct... although some overlap might be possible

So we want to deprecate these methods now with warning, to give people time to migrate. But we don't know what solution to propose instead. Or is there none? How is this situation usually handled the idiomatic way?

How it will work is

1. A subproject applies the Dokka plugin

   // ./subproject-alpha/build.gradle.kts
   // this will _produce_ Dokka configuration
   
   plugins {
     kotlin("jvm")
     id("org.jetbrains.dokka2")
   }

2. Any other project can consume the Dokka config from other subprojects. A sensible default is the root project

   // ./build.gradle.kts
   // this will _consume_ Dokka configuration from other subprojects
   
   plugins {
     id("org.jetbrains.dokka2")
     // (this project doesn't need the Kotlin plugin)
   }
   
   dependencies {
     // dependencies can be set manually
     dokka(projects(":subproject-alpha"))
   
     // or automatically (this could be configured using Gradle's 'default dependencies'
     allprojects.forEach { subProj ->
       dokka(project(subProj.path))
     }
   
     // plugins can also be set
     dokkaPlugin("org.jetbrains.dokka:markdown:1.2.3")
   
   
     // or the default Dokka Generator version could be overridden
     dokkaGenerator("org.jetbrains.dokka:dokka-core:1.7.20")
   }

3. The root project can now run the dokkaGenerate task which will retrieve the Dokka config from registered subproject dependencies via the dokka Gradle Configuration, and compute new settings for the Dokka Generate task

A similar example for JaCoCo aggregation is available here: https://docs.gradle.org/7.3/samples/sample_jvm_multi_project_with_code_coverage.html

I've been trying to develop a new Dokka plugin that works in 'new way'.

Here's an example integration test: https://github.com/aSemy/dokka/blob/feat/gradle-plugin-2/runners/gradle-plugin-2/src/testFunctional/kotlin/MultiModuleFunctionalTest.kt. It runs, but there's some problem with the Dokka Generator runtime classpath...

The old and new way might be able to work in parallel, and it might be possible to have some compatibility utils.

For example, files are added to the dokka Gradle Configuration from tasks:

val dokkaConfigurationTask by tasks.registering {}

val dokkaConfigurationElements by configurations.registering {
  // this configuration is...
  isCanBeResolved = false // NOT resolved by the source-project
  isCanBeConsumed = true  // IS consumed by other projects

  // This will cause the dokkaGeneratorTask task to run if the dokkaConfigurationTask requested by the aggregation task
  outgoing.artifact(dokkaConfigurationTask.map { task ->
    task.dokkaConfigurationFile
  })
}

This could be wrapped in a function... maybe - I'm unsure about how it will work in practice

abstract DokkaConfig {
  val dokkaConfiguration: Configuration

  fun addChildTask(task: DokkaTask) = dokkaConfiguration.configure { outgoing.artifact(task.map {...}) }
}

I don't think it's possible to handle the remove... functions reasonably though, it's not really idiomatic Gradle because of the lazy nature of evaluation - really this should be excludeTask, because the full list of tasks shouldn't be computed until necessary (in the execution phase)