Refactor gradle plugin #263
Conversation
|
I see you did not touch bindings logic but we have a failure on |
chippmann
force-pushed
the
feature/cleanup_gradle_plugin
branch
from
2b0e1a9 to
df84299
Compare
There was a problem hiding this comment.
It's a bit of an anti-pattern for a task to declare its own dependencies. Normally it's the responsibility of the plugin to setup that. If a task requires some input from another task then it can be passed to the task as an input property. https://docs.gradle.org/current/userguide/custom_tasks.html
open class TaskA: AbstractTask() {
@Output
public val output = objects.stringProperty()
}
open class TaskB: AbstractTask() {
@Input
val input = objects.stringProperty()
}
// somewhere in the plugin
taskB.input.set(taskA.output)
The advantage of doing it this way is we can leverage gradle incremental build support. So if taskB's output didn't change then taskA will not be executed.
So a task should only do one thing, it doesn't know anything about the other tasks.
…nto individual files
| ) | ||
|
|
||
| // START: android specific tasks | ||
| val checkD8ToolAccessibleTask = checkD8ToolAccessibleTask() |
There was a problem hiding this comment.
I was expecting an if (isAndroidExportConfigured()) statement here, what happens if the user doesnt' want to build for android?
There was a problem hiding this comment.
This is currently taking care of in the setup of the build task:
if (godotJvmExtension.isAndroidExportEnabled.get()) {
finalizedBy(packageBootstrapDexJarTask, packageMainDexJarTask)
}There was a problem hiding this comment.
Ahh, weird way of doing it - we don't really need to configure the android/native image tasks when they are disabled?
There was a problem hiding this comment.
Anyway, not a big issue. This will change once I'm done with the refactor.
| // END: android specific tasks | ||
|
|
||
| // START: graal native image specific tasks | ||
| val checkNativeImageToolAccessibleTask = checkNativeImageToolAccessibleTask() |
| import org.gradle.api.Task | ||
|
|
||
| fun Project.checkAndroidJarAccessibleTask(): Task { | ||
| return with(tasks.create("checkAndroidJarAccessible")) { |
There was a problem hiding this comment.
Not a big issue, but have you tried using register instead of create? With register a task is only configured/resolved when it will participate in the execution.
There was a problem hiding this comment.
Ah nice to know. I'll change them
Resolves #259
by introducing a task registry with which the individual tasks are properly grouped and accessible and configurable in a typesafe way.by splitting the task registration/creation into individual extension functions residing in their own files.It also splits some functions up into individual parts and removes some parameter passing by introducing extension functions/properties for gradles
Projectclass so we can access extensions directly for example.This could further be improved though by using sealed classes or interfaces for the tasks and use them inside theTaskRegistryas in it's current state, theTaskRegistrycould become unreadable if even more tasks would be added in the future. But to keep this more simple until we actually need it, i opted to just keep them in an enum for now.Edit: updated implementation after: #263 (review)