Fix compatibility with Gradle's configuration cache #2499

3flex · 2022-05-15T04:12:27Z

I've tested locally with success.

The changes are fairly simple, but because the configurations are now resolved when executing certain tests, those tests fail if the dependencies aren't available (this includes project dependencies as well as external dependencies).

The way to properly achieve this would be to publish the required artifacts to either mavenLocal() or a custom local file repository and adding that repository so they're made available to the tests.

Publishing for review first though in case there's another approach to follow.

liutikas · 2022-08-08T03:25:45Z

runners/gradle-plugin/src/main/kotlin/org/jetbrains/dokka/gradle/AbstractDokkaTask.kt

@@ -76,10 +76,10 @@ abstract class AbstractDokkaTask : DefaultTask() {
    }

    @Classpath
-    val plugins: Configuration = project.maybeCreateDokkaPluginConfiguration(name)
+    val plugins: ConfigurableFileCollection = project.objects.fileCollection()


You can drop explicit initialization, just make this
abstract val plugins: ConfigurableFileCollection and Gradle will initialize this for you.

Thanks, I'm aware, I just left it as it currently is to align with the rest of the parameters which still use manual initialisation.

liutikas · 2022-08-08T03:25:51Z

runners/gradle-plugin/src/main/kotlin/org/jetbrains/dokka/gradle/AbstractDokkaTask.kt


    @Classpath
-    val runtime: Configuration = project.maybeCreateDokkaRuntimeConfiguration(name)
+    val runtime: ConfigurableFileCollection = project.objects.fileCollection()


Same as above

liutikas · 2022-08-08T03:27:28Z

runners/gradle-plugin/src/test/kotlin/org/jetbrains/dokka/gradle/DokkaCollectorTaskTest.kt

@@ -21,8 +22,9 @@ class DokkaCollectorTaskTest {

        rootProject.allprojects { project ->
            project.plugins.apply("org.jetbrains.dokka")
-            project.tasks.withType<AbstractDokkaTask>().configureEach { task ->
-                task.plugins.withDependencies { dependencies -> dependencies.clear() }
+            project.repositories {


What's the context for this change? I'd leave a comment why we are adding mavenLocal here.

I'll add a comment once I have guidance on what's preferred in terms of setting up the test environment with access to the required dokka dependencies and put that in place - I tried to hint at this in the PR description:

The changes are fairly simple, but because the configurations are now resolved when executing certain tests, those tests fail if the dependencies aren't available (this includes project dependencies as well as external dependencies).

The way to properly achieve this would be to publish the required artifacts to either mavenLocal() or a custom local file repository and adding that repository so they're made available to the tests.

I'm not aware of any standard pattern for this.

I'm not aware of any standard pattern for this.

Maven Local can work, but isn't great because it's possible for other dependencies to get picked up as well, and it's not easy to clean out old dependencies.

A custom local file repository has significant drawback: when publishing a SNAPSHOT version, Gradle will always publish a new artifact. This is slow, and causes the local dir to grow in size when working locally, which is annoying.

Anyway, I'm working on a Gradle Plugin for solving this problem, if you'd like to take a look (though it's still awaiting approval in the Plugin Portal). It will publish to a local directory, but I added some caching to prevent SNAPSHOT spam, and utilities for depending on other subprojects.

@aSemy what if we start by publishing the artifacts to MavenLocal to unblock this PR and release it in 1.8.20? Once your plugin is ready, would you be able to substitute the MavenLocal publishing with it?

I'm personally fine with publishing it to Maven Local or to a custom local file repository, doesn't seem like we have a better alternative anyway, and having the configuration cache support is I think more important

So I think we can take this way for now 👍

Yeah for sure, Maven Local will work, just be aware that it might cause some unexpected problems and the solution is usually 'clear Dokka artifacts from Maven Local'

atyrin · 2022-09-07T14:36:27Z

@3flex hi, what is the state of the pull request? Are you waiting for a review from the team or is it still in development? Do you plan to finish your work?

3flex · 2022-09-08T03:37:12Z

I intend to finish it - but I have the open question re: testing, see #2499 (comment)

There are options but I'd like the maintainers to weigh in on what they'd prefer.

IgnatBeresnev · 2022-09-08T04:45:35Z

Hi! Yeah, sorry it's taking so long to get to: I haven't had the pleasure of dealing with this in any other project, so I also need time (and, more importantly, brain fuel) to research it.

Right off the bat I don't see any other solution apart from publishing it to mavenLocal() or some file repository :( It is strange though that in order to run unit tests you need to do something like this

3flex · 2022-09-08T06:32:31Z

I've also worked on an alternative for detekt, based on TestKit and the way it injects plugin classpaths to builds under test when testing Gradle plugins.

detekt/detekt#5149

So that's a third option.

3flex · 2022-12-04T07:23:30Z

Is there anything I can do to move this along? There's still more code to write and review once a direction is chosen for the testing side of things, so it would be good to get some direction.

Gradle is moving towards making configuration cache stable, possibly for Gradle 8 or soon after. It would be great to get this plugin ready before then.

IgnatBeresnev · 2023-02-02T00:04:04Z

Hi! We have not forgotten about this PR, but our resources have been spread thin for quite some time, and dealing with Gradle would occupy quite a bit of it, sorry.

I'll be making progress with Gradle-related PRs in the upcoming weeks (as part of #2700), and I'll have a closer look at this PR and your question after the first wave is complete.

@aSemy I don't think any of your PRs are related to or resolved this? If not, I'd be happy if you helped with brainstorming ideas for the question @3flex had - you definitely know better than me, and it'll take some time for me to research it all myself to propose an adequate solution.

aSemy · 2023-02-02T12:35:40Z

@aSemy I don't think any of your PRs are related to or resolved this? If not, I'd be happy if you helped with brainstorming ideas for the question @3flex had - you definitely know better than me, and it'll take some time for me to research it all myself to propose an adequate solution.

Hey thanks for asking.

So as I understand, this PR will updates the task properties to use types that Gradle can correctly serialize. However, my guess is that there will still be some recurring issues with config cache and/or build cache (because of the cross-project task dependencies #2822), and that requires a much more hefty fix.

So I think this PR is good and should be merged.... but the updates might become outdated soon. (What I'm hoping is that the refactored plugin #2839 will be an almost drop-in replacement for the current plugin, and it solves config cache/build cache problems.)

What I think would help is improving the tests.

updating Dokka's Gradle config (buildSrc, and then included-build plugins)
refactoring the integration tests (e.g. use Gradle TestKit and JVM Test Suite to structure them)
then enable the configuration cache (and build cache) in all test, running the tests twice, and making sure that the tests still work, and the logs indicate that the config cache/build cache were used
- https://docs.gradle.org/7.6/userguide/configuration_cache.html#config_cache:testing
- https://docs.gradle.org/7.6/userguide/test_kit.html#sub:test-kit-build-cache

Which are all reasonable, but they would take time (which is why I think this PR should be merged, because it will help a little)

3flex · 2023-03-20T21:18:22Z

Configuration cache is being promoted to stable in Gradle 8.1 which will be released within weeks.

What can I do to move this forward?

IgnatBeresnev · 2023-03-27T19:45:11Z

@3flex thanks for the reminder! I believe we've merged all of the major refactorings from #2700 (which introduced a lot of conflicts, sorry), and we still have a few weeks before Dokka 1.8.20, so we can try to squeeze it in

Could you please rebase this branch onto master? Or feel free to open a new PR if you find it easier than resolving conflicts

IgnatBeresnev · 2023-03-27T19:57:13Z

As for possible blockers, I think pretty much any solution that enables the use of Gradle's configuration cache should be fine, as we're aiming to eventually migrate to the re-written Gradle plugin from #2839, and it doesn't make much sense to spend a lot of time on making the current one beautiful - just have to make sure it works in the meantime.

The only thing to pay attention to are the tests - they should be runnable and should fail if breaking changes are introduced (like inserting a random throw IllegalStateException() in one of them or deleteing some important code inside dokka-base). I'll check this manually once all GitHub checks are green

3flex · 2023-03-28T02:22:44Z

I'm happy to rebase, but still need a response or approach to deal with this that's considered acceptable: #2499 (comment)

The simplest way to address it would be to publish the required artifacts to a local repository and then access them in the tests. Let me know if this is acceptable and I'll work on closing this out.

IgnatBeresnev · 2023-03-30T13:08:27Z

(discussed this in one of the comments, posting a summary here in case it was missed)

Publishing dependencies to Maven Local seems to be a good enough solution to unblock this PR 👍 We can revisit this part later once there's a better way to go about it

3flex · 2024-02-03T10:28:54Z

This isn't closed, so maybe there's still some value, but I'd like to know if dokkatoo will be used as the new Gradle plugin? I understand that already supports the configuration cache so I would close this if the plan is to use that or rewrite the plugin.

I have renewed interest in this since Gradle 8.6 supports encryption of the configuration cache and the setup-gradle action integrates nicely with it, meaning CI builds including doc builds will greatly benefit from CC support.

aSemy · 2024-02-05T20:55:33Z

This isn't closed, so maybe there's still some value, but I'd like to know if dokkatoo will be used as the new Gradle plugin?

hey @3flex, I can answer, because I work for JetBrains now!

Dokka Gradle Plugin will be updated to fully support Configuration Cache and Build Cache, and this will be based on Dokkatoo in some fashion. However, I can't say how long this will take. In the meantime I'm still actively maintaining and improving Dokkatoo, and I'd be happy to help out if anyone needs support.

I'd like to note as well that it's a good opportunity for anyone to get involved with the new Gradle plugin. For example, what should the new DSL look like? And what features should it have?

3flex · 2024-02-06T03:14:41Z

To confirm, does that mean the updated Gradle plugin will be:

Built using the existing plugin as the base, with learnings from dokkatoo
Built using dokkatoo as the base
Written from scratch?

It sounds like it's 2?

If it's 1 I'll update this PR (eventually!), if it's 2 or 3 then I'll close it.

If it's not clear yet then I'll hold off.

adam-enko · 2024-02-06T09:16:39Z

(replying with my official JB hat since this is more about the future work rather than in my of-the-clock Dokkatoo work)

To confirm, does that mean the updated Gradle plugin will be:

Built using the existing plugin as the base, with learnings from dokkatoo

Built using dokkatoo as the base

Written from scratch?

It sounds like it's 2?

The idea is: written from scratch, but copy-pasting a lot from Dokkatoo. So a mix of 3 and 2.

The DSL used to configure the plugin in build-logic is still being considered, but it will probably be closer to Dokkatoo (a DSL-based config) than DGP (task-based config).
The underlying plugin workings will need to be informed by the DSL, but again it will probably more similar to Dokkatoo.

But it's not been started yet and this could change.

In any case, I still see value in this PR because it does improve DGP, which is still widely used.

WDYT @IgnatBeresnev?

WIP

b27bc3f

IgnatBeresnev requested review from IgnatBeresnev and vmishenev May 16, 2022 13:41

liutikas reviewed Aug 8, 2022

View reviewed changes

IgnatBeresnev added the runner: Gradle plugin An issue/PR related to Dokka's Gradle plugin label Oct 12, 2022

Goooler mentioned this pull request Oct 13, 2022

Improve compatibility with the Gradle API, and follow best practices #2700

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compatibility with Gradle's configuration cache #2499

Fix compatibility with Gradle's configuration cache #2499

3flex commented May 15, 2022

liutikas Aug 8, 2022

3flex Sep 4, 2022

liutikas Aug 8, 2022

liutikas Aug 8, 2022

3flex Sep 4, 2022

aSemy Mar 28, 2023

IgnatBeresnev Mar 28, 2023

IgnatBeresnev Mar 28, 2023

aSemy Mar 29, 2023

atyrin commented Sep 7, 2022

3flex commented Sep 8, 2022

IgnatBeresnev commented Sep 8, 2022

3flex commented Sep 8, 2022

3flex commented Dec 4, 2022

IgnatBeresnev commented Feb 2, 2023

aSemy commented Feb 2, 2023

3flex commented Mar 20, 2023

IgnatBeresnev commented Mar 27, 2023

IgnatBeresnev commented Mar 27, 2023 •

edited

3flex commented Mar 28, 2023

IgnatBeresnev commented Mar 30, 2023 •

edited

3flex commented Feb 3, 2024

aSemy commented Feb 5, 2024 •

edited

3flex commented Feb 6, 2024 •

edited

adam-enko commented Feb 6, 2024 •

edited

Fix compatibility with Gradle's configuration cache #2499

Are you sure you want to change the base?

Fix compatibility with Gradle's configuration cache #2499

Conversation

3flex commented May 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atyrin commented Sep 7, 2022

3flex commented Sep 8, 2022

IgnatBeresnev commented Sep 8, 2022

3flex commented Sep 8, 2022

3flex commented Dec 4, 2022

IgnatBeresnev commented Feb 2, 2023

aSemy commented Feb 2, 2023

3flex commented Mar 20, 2023

IgnatBeresnev commented Mar 27, 2023

IgnatBeresnev commented Mar 27, 2023 • edited

3flex commented Mar 28, 2023

IgnatBeresnev commented Mar 30, 2023 • edited

3flex commented Feb 3, 2024

aSemy commented Feb 5, 2024 • edited

3flex commented Feb 6, 2024 • edited

adam-enko commented Feb 6, 2024 • edited

IgnatBeresnev commented Mar 27, 2023 •

edited

IgnatBeresnev commented Mar 30, 2023 •

edited

aSemy commented Feb 5, 2024 •

edited

3flex commented Feb 6, 2024 •

edited

adam-enko commented Feb 6, 2024 •

edited