Improve Multiplatform Support With Source Set-Based Configuration #1021
Conversation
| */ | ||
| private fun decorateKotlinTarget(target: KotlinTarget) { | ||
| // TODO: Check whether special AGP handling is still necessary. |
There was a problem hiding this comment.
IIRC this is still needed as KMP creates additional sources sets that do not map to the AGP ones.
There was a problem hiding this comment.
The motivation for this comment is this: Previously, KSP could only see a subset of source sets configured. This has now changed, so it might see AGP-created source sets where it did not before, possibly enabling a unified KMP/Android handling. Would you suggest dropping this comment without further investigation?
| val parentKspTaskName = lowerCamelCased("ksp", sourceSetName, "KotlinMetadata") | ||
|
|
||
| project.locateTask<KspTask>(parentKspTaskName)?.let { parentKspTask -> | ||
| dependsOn(parentKspTask) |
There was a problem hiding this comment.
Can we please avoid manual task wiring e.g using dependsOn?
Instead, inputs added from the parentKspTask should already contain correct dependency information.
There was a problem hiding this comment.
Leaving out dependencies on parent KSP tasks will make the integration test KMPWithHmppIT#testCustomSourceSetHierarchyBuild fail:
> Task :workload:compileKotlinJvm FAILED
Execution optimizations have been disabled for task ':workload:compileKotlinJvm' to ensure correctness due to the following reasons:
- Gradle detected a problem with the following location: '/tmp/junit389805203877564473/workload/build/generated/ksp/metadata/clientMain/kotlin'. Reason: Task ':workload:compileKotlinJvm' uses this output of task ':workload:kspClientMainKotlinMetadata' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.
- Gradle detected a problem with the following location: '/tmp/junit389805203877564473/workload/build/generated/ksp/metadata/commonMain/kotlin'. Reason: Task ':workload:compileKotlinJvm' uses this output of task ':workload:kspCommonMainKotlinMetadata' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.
e: /tmp/junit389805203877564473/workload/src/clientMain/kotlin/com/example/ClientMainAnnotated.kt: (5, 20): Unresolved reference: ClientMainAnnotatedForClientMain
The above dependencies seem analogous to this kotlinCompile.dependsOn(kspTaskProvider), which has been present before.
What's your suggestion to do instead?
There was a problem hiding this comment.
I guess 29f416f is what you were looking for.
| // Some options have a global and a source set-specific variant. The latter, if specified, overrides the former. | ||
| // The following is necessary due to KotlinSourceSet not being extension-aware: | ||
| // - A KotlinSourceSet receiver addresses the source set-specific variant if the `ksp { ... }` block is invoked | ||
| // inside a source set block. | ||
| // - A Project receiver addresses the corresponding global variant. (This is required as the compiler's name | ||
| // resolution would always prefer a receiver-less member and never invoke an extension receiver variant.) | ||
| // - A corresponding private property provides the global variant's backing field. |
There was a problem hiding this comment.
Does this also work for Groovy?
There was a problem hiding this comment.
Most probably not (as we need to have the upstream change of KotlinSourceSet being extension-aware), but I still have this on my radar and will check. Is Groovy support a KSP goal? (As KSP is for Kotlin and there is not a single test based on Groovy build scripts, I'm wondering.)
There was a problem hiding this comment.
We use kotlin script in KSP project itself, but there are lots of end users still uses Groovy, so I would prefer a groovy support, if possible.
There was a problem hiding this comment.
Actually, with the current PR, Groovy accessors do not work. In fact, it breaks the global ksp { ... } block.
A GitHub code search finds
- 40 files using Groovy build scripts – search term
content:/ksp *\{/ path:/\.gradle$/ NOT is:archived - 91 files using Kotlin build scripts – search term
content:/ksp *\{/ path:/\.gradle.kts$/ NOT is:archived
To avoid breaking Groovy support, we could either
- enable the new functionality dependent on a Gradle property, say
ksp.multiplatform.enabled, or - register a second extension, which is then used exclusively for source sets, e.g.
kspSourceSet { ... }.
The first alternative would have the advantage of not requiring changes once KotlinSourceSet becomes extension aware, so I'd suggest using the property to switch. OK for you?
There was a problem hiding this comment.
Commit 9cf4738 will stop breaking Groovy build scripts, making the source set configuration dependent on a Gradle property setting ksp.multiplatform.enabled=true. Configuring source sets va Groovy will be supported later, once KotlinSourceSet becomes extension-aware.
|
Quick status update: In
Other tasks can also depend on KSP-generated sources. For example, there is Generally, task dependencies cannot fix problems completely:
As @gavra0 pointed out, manual task wiring should generally be avoided in favor of outputs carrying |
These can be enabled via setting the Gradle property 'ksp.sourceSetDependencies.enabled=true'. They are enabled by default for 'ksp.multiplatform.enabled=true'.
|
This is now ready for review. By default, KSP still behaves as before. The idea is not to break existing code, even if it relies on buggy behavior.
Current caveats:
Users could be encouraged to enable these properties via release notes. We could also add recommendations Alternative: Enabling source set dependencies by defaultIf the default of
These tests all fail with: The cause is that KSP, when processing the
|
|
A correction regarding failing The root cause of the redeclaration errors is not KSP ignoring generated files from the parent
So the old behavior with task dependencies hides generated |
|
The simplification over the current implementation (
This is intended. In a target compilation, all the source sets (target + transitive deps) form a compilation unit, instead of just the target source set + libraries/binaries from dependent source sets. In other words, while source sets are shared, compilations are not. If thinking of annotation processing a kind of compilation, the old behavior is more in line with compilation and less surprising. Not saying this is right or wrong, but I'm also worried that something isn't working with Groovy. |
Yes, that's also my understanding with the current model. (It is also my understanding that klibs will at some point be used to avoid the need to recompile shared sources.)
Yes, from a producer's perspective, that looks consistent. Of course, the difference between compilation and KSP processing is the type of artifact generated, "binary" (metadata, klib, target code) vs. sources. Consumers of sources (IDE, Gradle plugins, compiler, source JAR packaging) currently assume an "integrated source set" model, which does not differentiate between generated and original sources. If, in spite of this, we treat generated sources different from original sources, the consumer's model becomes inconsistent, creating the problems a number of users are currently having:
Does the above help?
This is only temporary, and will not hurt unless explicitly enabled via |
|
IMHO, adding generated sources to gradle source set as a workaround to inform IDE is probably not right in the beginning. Currently, it works within the same source set only because KSP filters out its output folder from its inputs. For multiple compilation units, this workaround is the source of all the issues. We are exploring a set of new APIs to inform IDEs (maybe through Gradle) about the (to be) generated sources. Does that make sense? |
|
Yes, if it were a workaround just to inform the IDE, it would certainly make sense to find another solution. Instead of a workaround just to please the IDE, I see the addition of generated files to their respective source set as a way to inform all downstream tools about KSP outputs in a source set-specific way. (This seems consistent with KSP applying the principle to itself via its multi-round processing.) Let's assume KSP outputs are not part of their respective source set. What would be the way to inform consumers of source set inputs about generated files? (Task dependencies apparently do not work correctly as Kotlin MPP's |
|
Beyond feature discussions and toolchain internals, I'd like to add some thoughts about developer expectations and creating a working mental model. So here's an attempt to step back from implementation details and put on my KSP user hat. As a multiplatform developer, I want to generate code from annotations. I already know about multiplatform source sets, targets and expect/actual. I'd like to generate code as if I wrote it manually. Generated code would be part of the source set of my choice, the compiler and other tools would process it just like code I wrote manually. I might have heard about compilations, but prefer not to think too much about those, expecting them to just work and generate the target code from my source hierarchy. Assuming the above with KSP leads to surprises: Identical code is generated multiple times in different places, generated code appears in a source set, but is then hidden from dependent source sets. It's almost like Schrödinger's generated code, somehow it exists, but then it doesn't. Without knowing implementation details, I'd have a hard time understanding how everything plays together in the tool chain. If I'd want to integrate something, which does not work out of the box, I'd probably be in trouble. For example, I wonder if generating expect/actual code from a single annotation would ever be possible. For me to be productive, I'd like KSP to operate in a way that can be understood by just applying previous knowledge about Kotlin multiplatform source code organization. |
|
I need some more time to think about and discuss this with @gavra0. Will update next week. |
|
Trying to double-check if the approach I am suggesting has possible downsides I might have overlooked, I could not find any:
Hope this helps. Not meant to push anything, just providing additional info. |
|
Hello @OliverO2, we really like the idea of source set based configuration and truly appreciate the discussion / explorations on the processing models. For Groovy support, I've talked with a few folks who have more context (than me) about the usage of Groovy and Kotlin Gradle scripts in the field. We'd really like to avoid having a different level of support between Kotlin and Groovy, even if it is protected by an experimental flag. I'm wondering if it is possible to have another approach (than extension) to achieve the same goal? As for the processing model, the behavior of this patch is different from #1037. In this PR, processors still see all upstream source sets, plus generated codes from upstream. The proposal in #1037 seems to suggest processing each source set individually. Is that correct? Either way, I think the PR has gone too far. Can we leave the processing model changes out and focus on configuration for this PR? |
|
To summarize, this PR addresses the following issue complexes:
Supporting (1.) in a Groovy-compatible way would require upstream to make Fully supporting (2.) and (3.) means changing the current processing model, committing KSP to generate actual sources (not quasi-binaries) and communicating this to the toolchain. As far as I am aware, this is the only approach which plays well with all Gradle plugins encountering KSP-generated code. Other approaches led to playing Whac-A-Mole with workarounds. #1037 complements this PR: In both cases, processors would see parent source sets (including their generated files) – otherwise symbols would not resolve. #1037 enables generating code for the special case of intermediate source sets, which are not shared between multiple targets. It is probably not that important currently, but at least documents that the case exists and what could be done to achieve a consistent KSP experience. More important would probably be the complementary suggestion in #965, which has even been requested as the default mode of operation: Filtering input files by the current (output) source set to avoid generating duplicate declarations. For me, reverting to KSP's current processing model would break things. Focusing on Groovy-compatible source set configuration would require extra work without providing tangible benefits, so nothing I could justify. For now, I'll stay with KSP plus this PR's changes and will probably switch to a regular compiler plugin over time. Then it's all binary (IR) code and the difficulties of having generated "sources, but not sources" vanish. If someone finds another viable path which addresses all issues encountered, I'd appreciate it. If you'd like to reuse parts of this PR, you're of course free to cherry-pick and/or copy-paste whatever you see fit. Thanks for considering! |
|
For Groovy support, I agree that writing per-source-set configurations in the global ksp extension is ugly so let's not go for it if possible. Unfortunately, I don't have a better plan either. As for the model, there are many details to be defined, like what you mentioned in #965. For example, in this PR, processors see target + parents + generated_from_parents when processing a target source set. This can be redundant compared to the current model, depending on processors. Also, because there is no API to distinguish from those source sets currently, the change will only make things more difficult: how does a processor know which class is processed and which is not? Workarounds will be needed. Or there can be a future model change again. Given the complexity of the model change, I'd really appreciate it if it can be carried out in separate PRs (let's keep discussing in #965 or #1037), as they (configuration for each source set / making outputs observable to downstreams) don't seem to be closely coupled. |
That's true, we need the source set detection inside the processor for that. Ideally, we'd have incremental processing even across source sets, but it's not there (yet). However, in the use cases I was considering, the processor can easily differentiate when given the names of input and output source set. I wonder what an alternative plan might look like that would not contradict the expectations of the Gradle infrastructure. What would we do about downstream tasks (beyond the compiler) processing sources from source sets, if these downstream tasks expect input file dependencies and break things if given a task dependency instead (such as Gradle I agree that a model change should not be taken lightly and we should find a solution that avoids any further model change. |
|
Is anything from this PR going to be integrated into KSP? It would solve all of the complications I have in my mutliplatform codebase when it comes to working with KSP. |
|
Is there any update on this? It would be very nice to get support like this for KMP. |
|
how do i create a processor |
This PR addresses multiplatform issues with a new source set-based configuration.
It retains backward compatibility with dependency-based configuration. There is no need to change existing builds, which have been successfully using KSP before. However, fixing multiplatform issues mentioned below requires switching to the new source set-based configuration.
Details
sourceSets { val commonMain by getting { ksp { processor(project(":test-processor")) } } }examples/multiplatform/workload/build.gradle.kts, the above declaration replaces:dependencies { add("kspCommonMainMetadata", project(":test-processor")) add("kspJvm", project(":test-processor")) add("kspJs", project(":test-processor")) add("kspAndroidNativeX64", project(":test-processor")) add("kspAndroidNativeArm64", project(":test-processor")) add("kspLinuxX64", project(":test-processor")) add("kspMingwX64", project(":test-processor")) }enabled,processor,argandallWarningsAsErrors(other options are global only):sourceSets { val commonMain by getting { ksp { processor(project(":test-processor")) arg("option1", "value1") allWarningsAsErrors = true } } }inheritable = false. Theinheritableoption is not inheritable andtrueunless set otherwise.clientMain).Versions