-
Notifications
You must be signed in to change notification settings - Fork 0
Introduce CommonKspSettingsPlugin
#58
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors KSP plugin configuration by extracting common settings into a dedicated CommonKspSettingsPlugin. Previously, common KSP settings were applied using a synchronized mechanism with build services that proved fragile in multi-project builds with separate ClassLoaders. The new approach leverages Gradle's built-in plugin application semantics to ensure settings are applied exactly once per project.
Key changes:
- Introduced
CommonKspSettingsPluginto encapsulate all common KSP configuration logic - Simplified
KspBasedPluginto delegate settings application to the new plugin - Updated project version from
2.0.0-SNAPSHOT.029to2.0.0-SNAPSHOT.030
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
version.gradle.kts |
Bumped version to 2.0.0-SNAPSHOT.030 |
pom.xml |
Updated version and protoc dependency to 4.33.1 |
ksp/src/main/kotlin/io/spine/tools/core/jvm/ksp/gradle/KspBasedPlugin.kt |
Removed synchronization logic and delegated common settings to new plugin |
ksp/src/main/kotlin/io/spine/tools/core/jvm/ksp/gradle/CommonKspSettingsPlugin.kt |
New plugin containing extracted common KSP settings logic |
dependencies.md |
Updated to reflect new version and protoc dependency changes |
config |
Updated subproject commit reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /** | ||
| * The function replaces default destination directory defied by |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'defied' to 'defined'.
| * The function replaces default destination directory defied by | |
| * The function replaces default destination directory defined by |
| sourceSet.run { | ||
| // KSP Gradle Plugin already added its output to source sets. | ||
| // We need to add the replacement manually because we filtered | ||
| // it before in `Project.makeKspIgnoreGeneratedSourceProtoDir()`. |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The referenced function name Project.makeKspIgnoreGeneratedSourceProtoDir() doesn't match the actual function name makeKspIgnoreProtocOutputDir() in the codebase. Update the comment to reference the correct function name.
| // it before in `Project.makeKspIgnoreGeneratedSourceProtoDir()`. | |
| // it before in `Project.makeKspIgnoreProtocOutputDir()`. |
armiol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexander-yevsyukov LGTM in general. Please see my comments.
| import org.gradle.api.file.DirectoryProperty | ||
| import org.gradle.kotlin.dsl.findByType | ||
|
|
||
| public class CommonKspSettingsPlugin : Plugin<Project> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document this type.
| val path = protobufExtension?.generatedFilesBaseDir | ||
| return path?.let { File(it) } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is redundant.
| makeKspTasksDependOnSpineCompiler() | ||
| makeCompileKotlinTasksDependOnKspTasks() | ||
| replaceKspOutputDirs() | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is also redundant.
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this one is also redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR simplifies applying common KSP plugin settings. KSP plugins working in the Spine environment need to apply common settings once. Previously, the status of applying the settings was checked using the Build services feature. It proved to be fragile in the environments where plugins are applied in
subprojectsblock where these blocks are not related to each other and do not have commonClassLoader.This PR aims to solve this issue, pulling out the common settings into a plugin, which
KspBasedPluginapplies via the class name. This makes Gradle to apply the plugin once, which was the intention of the previous arrangement with build services.