Skip to content
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

Mark tasks as not compatible with the Gradle configuration cache #3070

Merged
merged 2 commits into from Aug 25, 2023

Conversation

BoD
Copy link
Contributor

@BoD BoD commented Jul 11, 2023

Since, as for now, Dokka is not compatible with the Gradle configuration cache (see #1217), using it in a project that has it enabled will fail the build.
Marking the tasks as incompatible avoids these fails.

@IgnatBeresnev IgnatBeresnev self-requested a review August 17, 2023 12:42
Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution, very much appreciated!

I'll start Gradle integration tests now, which should take a couple of hours, and if everything's alright - will merge it.

@@ -232,6 +232,7 @@ abstract class AbstractDokkaTask : DefaultTask() {

init {
group = JavaBasePlugin.DOCUMENTATION_GROUP
super.notCompatibleWithConfigurationCache("Dokka tasks are not yet compatible with the Gradle configuration cache. See https://github.com/Kotlin/dokka/issues/1217")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, the integration tests are failing :(

Caused by: java.lang.NoSuchMethodError: 'void org.gradle.api.DefaultTask.notCompatibleWithConfigurationCache(java.lang.String)

The used method (notCompatibleWithConfigurationCache) was introduced in Gradle 7.4, whereas we still have to keep compatibility with Gradle 6.9+ (for as long as Kotlin itself supports it).

We probably need to use something like plugin variants to fix this properly, but there's little point in investing time into the current Gradle plugin - it will be completely re-written anyway.

However, it would still be nice to have this change for the time being. I think we can add a dirty hack - check with reflection that this method exists, and only then call it. What do you think?

Something along the lines of

val containsNotCompatibleWithConfigurationCache = this::class.memberFunctions
    .any { it.name == "notCompatibleWithConfigurationCache" && it.parameters.firstOrNull()?.name == "reason" }

if (containsNotCompatibleWithConfigurationCache) {
    super.notCompatibleWithConfigurationCache("Dokka tasks are not yet compatible with the Gradle configuration cache. See https://github.com/Kotlin/dokka/issues/1217")
}

Copy link
Contributor Author

@BoD BoD Aug 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think using reflection is a fair workaround :) Feel free to amend the PR (or I can do it if you prefer). 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to rob you of the valuable contribution 😅 If you have the time and the desire, I'd be happy if you did it :) I also didn't test the suggestion - I mean, it should work if this function is returned in memberFunctions, but who knows

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've just pushed the change. It seems to work on my side but let's see if the CI agrees :)

@IgnatBeresnev IgnatBeresnev self-assigned this Aug 21, 2023
@IgnatBeresnev IgnatBeresnev self-requested a review August 21, 2023 14:38
Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍

I had to run the integration tests locally because this branch is quite old. All of the tests are green, so it should be good to go

Once again, thank you for addressing the issue!

@IgnatBeresnev IgnatBeresnev merged commit bec2cac into Kotlin:master Aug 25, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants