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

Remove usage of internal gradle classes in task dependencies #2835

Closed
wants to merge 4 commits into from

Conversation

atyrin
Copy link
Contributor

@atyrin atyrin commented Jan 30, 2023

Intermediate fix #2822

Switch in using storage of child tasks from an internal set of paths to Gradle set dependsOn.

Changes:

  • add check for a type inside addChildTask(path: String)
    • it will produce a warning in the import process if we can resolve the task by the path and it is not an AbstractDokkaTask. That part is changed compared with 1.7.20 (previously reported only on executing of :dokkaHtmlMultiModule).
    • it will not report anything during the import if we cannot resolve the task by the path. Otherwise, we will report the errors during the import if some submodules don't have applied the Dokka plugin.
  • fix formatting for an error message if there is no submodule with applied Dokka.

@atyrin
Copy link
Contributor Author

atyrin commented Jan 30, 2023

Also, I noticed that addSubprojectChildTasks, removeSubprojectChildTasks, removeChildTasks(projects: Iterable<Project>, childTasksName: String), addChildTasks(projects: Iterable<Project>, childTasksName: String) don't work in runtime (in project build files) but works during configuration (during pairing subproject files and parent)
(in 1.7.20 the same)

tasks.dokkaHtmlMultiModule.configure {
    addChildTask(":kn-moodule:dokkasub") // works
    addChildTasks(setOf(project(":kn-moodule")), ":dokkasub") // doesn't work
}

image

@atyrin
Copy link
Contributor Author

atyrin commented Jan 30, 2023

We have one more reference for internal package: import org.gradle.api.internal.plugins.DslObject inside /dokka/runners/gradle-plugin/src/main/kotlin/org/jetbrains/dokka/gradle/AbstractDokkaLeafTask.kt

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.

Overall LGTM 👍

Copy link
Member

@vmishenev vmishenev left a comment

Choose a reason for hiding this comment

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

Can you rebase onto master?

@atyrin
Copy link
Contributor Author

atyrin commented Feb 13, 2023

In the current approach, there is an issue for the scenario when there is a task that depends on AbstractDokkaParentTask, e.g.:

tasks.create("X"){
    dependsOn(tasks.getByName("dokkaHtmlMultiModule"))
}

In such configuration, Gradle throws an exception for calls like this project.tasks.findByPath(path).

Caused by: org.gradle.api.internal.AbstractMutationGuard$IllegalMutationException: Gradle#projectsEvaluated(Action) on build 'dokka-sandbox' cannot be executed in the current context.
	at org.gradle.api.internal.AbstractMutationGuard.createIllegalStateException(AbstractMutationGuard.java:39)
	at org.gradle.api.internal.AbstractMutationGuard.assertMutationAllowed(AbstractMutationGuard.java:34)
	at org.gradle.invocation.DefaultGradle.assertProjectMutatingMethodAllowed(DefaultGradle.java:287)
	at org.gradle.invocation.DefaultGradle.projectsEvaluated(DefaultGradle.java:354)
	at org.jetbrains.kotlin.gradle.plugin.DefaultKotlinBasePlugin.apply(KotlinPluginWrapper.kt:79)
	at org.jetbrains.kotlin.gradle.plugin.KotlinBasePluginWrapper.apply(KotlinPluginWrapper.kt:184)
	at org.jetbrains.kotlin.gradle.plugin.AbstractKotlinMultiplatformPluginWrapper.apply(KotlinPluginWrapper.kt:311)
	at org.jetbrains.kotlin.gradle.plugin.KotlinMultiplatformPluginWrapper.apply(PluginWrappers.kt:74)
	at org.jetbrains.kotlin.gradle.plugin.KotlinMultiplatformPluginWrapper.apply

So, the idea to put in dependsOn only resolved tasks is not working.

@IgnatBeresnev
Copy link
Member

Well, at least now we know! @atyrin thanks for the research

Proposing to do the following:

  1. Since we're working on project isolation and we're aiming to include it in the stable release, I don't think spending any more time trying to make the dependsOn change work makes sense. We tried it as an easy drop-in solution, but it turned out to be more difficult than I imagined. Hopefully, the internal Gradle API will not change dramatically until the stable release. So proposing to revert the dependsOn part.
  2. We're going to remove public API methods such as addChildTasks and removeChildTasks anyway, so I'm proposing to keep the deprecation
  3. This research was very valuable, maybe it was the whole reason why it was implemented through the internal API and not via dependsOn, so it makes sense to leave a short comment with a link to this PR

@atyrin
Copy link
Contributor Author

atyrin commented Feb 13, 2023

I've created a separate PR for further updates regarding deprecations only: #2857

@atyrin atyrin closed this Feb 13, 2023
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.

Remove the dependency on internal Gradle API
3 participants