Fix compatibility with Gradle's configuration cache #2499
Conversation
| @@ -76,10 +76,10 @@ abstract class AbstractDokkaTask : DefaultTask() { | |||
| } | |||
|
|
|||
| @Classpath | |||
| val plugins: Configuration = project.maybeCreateDokkaPluginConfiguration(name) | |||
| val plugins: ConfigurableFileCollection = project.objects.fileCollection() | |||
There was a problem hiding this comment.
You can drop explicit initialization, just make this
abstract val plugins: ConfigurableFileCollection and Gradle will initialize this for you.
There was a problem hiding this comment.
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.
|
|
||
| @Classpath | ||
| val runtime: Configuration = project.maybeCreateDokkaRuntimeConfiguration(name) | ||
| val runtime: ConfigurableFileCollection = project.objects.fileCollection() |
| @@ -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 { | |||
There was a problem hiding this comment.
What's the context for this change? I'd leave a comment why we are adding mavenLocal here.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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'
|
@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? |
|
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. |
|
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 |
|
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. So that's a third option. |
IgnatBeresnev
added
the
runner: Gradle plugin
|
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. |
|
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. |
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.
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) |
|
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? |
|
@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 |
|
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 |
|
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. |
|
(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 |
|
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 |
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? |
|
To confirm, does that mean the updated Gradle plugin will be:
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. |
|
(replying with my official JB hat since this is more about the future work rather than in my of-the-clock Dokkatoo work)
The idea is: written from scratch, but copy-pasting a lot from Dokkatoo. So a mix of 3 and 2.
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? |
Fixes #1217
Fixes #2231
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.