-
Notifications
You must be signed in to change notification settings - Fork 63
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
compilation errors are hidden when KSP is used #72
Comments
Discussed with KSP folks. google/ksp@3287061#diff-a345032d6c965be6e81478437767cb750c83bd0a952014661e86d454ea5a342bR132 Seems like the "proper" fix here is to run two compilations if symbol processors are provided. |
Thanks for the heads up. I will look into this in December when I have some free time again. |
I have a very hacky fix for this (yigit@978174a) which reverts the behavior. I might send a PR for it but i'm not super convinced whether we should merge it or not. |
ok here is the prototype of the second option: I didn't clean it up to keep the change minimal so please do not pay attention to the implementation. The key part is the change in compile to run KSP first and collect its output for the rest. Lmk when you get a chance to evaluate these two options. The recommendation from KSP folks is the second implementation but that requires adding KSP dependency to the main project. wdyt? |
I haven't reviewed closely but out of these two options I would also lean towards number two. However, I think it would be better to use this opportunity and rewrite KCT internals a bit to make things cleaner. In general I still don't want to add the KSP dependency to the KCT core library because there's no reason why KCT should know about it. If there is no clean way to implement KSP support without meddling in KCT internals then that suggest to me one of two things:
I know, it's pretty terrible right now. If you have some concrete suggestions how to improve it, let me know. |
Not sure if I agree because KSP is analogous to KAPT and KCT already knows about KAPT (unless of course you dislike that part as well)
I'll play with some options on how KCT can become more extendable. Also, I don't yet have a clear idea on how to make KCT less stateful. A one off copy function wouldn't be hard to write but it would easily break in the future. I possibly hack could be to move all of the state into a data class to make compiler generate the copy but i'm not fully sold on that either. |
Looking at this a but more but i cannot find an option w/o opening up KCT internals more. To run KSP, I basically need the source files and compilation configuration. I also thought about creating a KspCompilation that will have an internal delegate of KotlinCompilation but I cannot simply call Besides these, another option is to add lifecycle callbacks to KotlinCompilation where I can run KSP before the main compilation starts. It doesn't feel like the right API and would still require opening up APIs to build compiler arguments & source then run kotlin compilation. So tl;dr; I'm stuck on here. I still believe KCT should treat KAPT and KSP the same and embedding KSP into the main will provide the best API for the end user (+ will be safer to handle any future changes in KSP since it is an actively developed project). |
Which is fine as long as it makes sense and is not another hack.
This is my feeling as well. I have thought about what you said about KAPT and while I don't think that KSP has the same official position as KAPT, given Google's influence this will likely change. From a library user's perspective they probably don't care about the KSP dependency even if they don't need it (or could there be some kind of dependency conflicts?). Consequently, I now believe that integrating KSP into KotlinCompilation the same way as KAPT is valid if the alternative would be needlessly complex, which is becoming apparent. However, this is clearly not scalable. Perhaps there is another abstraction needed here at a lower level than KotlinCompilation (kinda like Gradle tasks?) that allows us to remove KAPT and the Javac-step from their priviliged positions and make everything easily composable. I think we are at a stage of the development lifecycle where breaking API changes are justified. In summary: If you want to integrate KSP completely into KotlinCompilation, you have my go ahead (as long as this doesn't cause any problems with dependencies or breaking APIs due to KSP's experimental nature), but my preference is to build everything in a more composable manner from the ground up and would appreciate your help with this (and any other users who might be reading this). Though, I probably won't get to that before mid-December. |
I actually don't know the details of why KSP is moved outside the main repo but I don't think it has any degrading effect of KSP's importance. KSP team develops it from Kotlin's perspective (with close collaboration w/ the kotlin team) but current implementations are all JVM because that is where we have the initial use cases (in fact, any JVM specific functionality becomes a big debate, e.g. google/ksp#141) For KCT, integrating KSP shouldn't break any APIs as long as developer is not using KSP. But there is still the danger that if KSP ever wants to depend on an unstable Kotlin Compiler version, we would hit a dependency problem. We can workaround that by excluding KSP's kotlin dependency and ask user to depend on that kotlin version if they are using KSP. I like the task idea. I'm not sure how much of the java compilation and kotlin compilation arguments are shared so I'm not sure about the unbundling java (but totally makes sense in theory). |
I'm still prototyping but i have a qq. More specifically, i would like to lock down the public mutable variables in KotlinCompilation when compilation starts (probably by extracting a builder out of it). |
It is undefined behavior but should work fine because the compiler would just overwrite the files in the working directory. I'm not sure if it makes sense to prescribe that
Isn't that a good thing, to certain degree? It should be possible for some tasks to add source files. Right now I'm imagining that support for some rudimentary build-system responsibilities such as multi-module compilation and resources would also be implemented using these composable tasks, which would require that they can copy/add resource files and classpath entries. |
It kind of depends where we want to put the abstraction. If we let them modify KotlinCompilation and also let developer call The API i was thinking of was more like:
where intermediate result would be something like:
Any output that can be contributed to the compilation would be limited to the intermediate result which we would keep in a model that lives during compilation and disposed after result is produced. Currently, another issue is that the |
Is that necessary? I think it would be enough to keep a list of paths to source files and we could make the writing of inline source files an explicit step in the compilation that is deferred. We could even make the workingDir an extra parameter to the
We can also create copies of the KotlinCompilation object to "change" properties without changing the original object. I don't want to get too hung up on the KotlinCompilation class as it exists now. It does a lot of things, perhaps too much. If we want to keep a somewhat similar interface, then I imagine we will end up with a function that builds the "task graph" under the hood from different classes that each have their own parameters, in order to present a simple interface for the user, or maybe something with the decorator pattern and lots of interface delegation to keep down the boilerplate.
I don't think so. The kapt generated sources are placed in a different folder and if we keep a list of paths, then we can avoid this even if they are in the same directory. In any case, we shouldn't rely on the directory structure to know which files belong to a compilation. The user could have multiple source files belonging to different tests in a single resource folder.
Perhaps I am overengineering, but that doesn't look general enough to me to support all the use cases. For example, how would you add compiler plugins for following steps? |
Sorry, I took steps can edit KotlinCompile as they can add sources, which would have an impact on later steps / compile calls.
I'm not sure if we have that case but we can basically add it to the IntermediateResult as a parameter too then core compilation would merge them. I'll try to prototype the thing i've mentioned above. (separate user defined configuration/data from intermediate ones). |
Here is where I am w/ the prototype. I've moved all data of KotlinCompilation (and friends) into
These interfaces are Then there is compilation steps which simply receive the current model + utilities to compile.
I still haven't figured out how to report generated classes/sources. For now, I also have outputFolder there but will probably make it more explicit for The nice thing about this setup is that each step can create its own
A similar implementation could be done to add plugins to followup steps. This gives us best of the both worlds where original user parameters are not modified while there is a very easy way to modify them during compilation for each step (thanks kotlin delegates:) )
One API change I'm making is removing kapt parameters from KotlinCompilation. Instead, KotlinCompilationStep adds an extension to For now, steps have an id and can be registered on AbstractKotlinCompilation. While registering, they can specify dependencies and dependants. As identifiers, I'm using String for now, can be changed. I kind of liked this setup, if you like it too; i can try to clean it up. Here are the other parts that I have not figured out yet.
wdyt? |
here is a prototype of KSP support w/ steps: |
btw i noticed while doing the KSP where for proper isolation for multiple Wdyt about that? |
and here is a prototype of moving step specific data (e.g. kapt stubs) into result via extensions: I'll stop here until you get a chance to review this. (no rush, room does have a workaround for now)
|
Hi, |
@yigit As you can see, I'm pretty busy currently due to my thesis but my plan is to review your prototype and do the rewrite this month. I don't see any reason to waste your time even more with a quick fix that will be undone soon, unless you desperately need it right now. |
@tschuchortdev @yigit We just encountered the issue that .class files are not generated anymore when using the latest version of KSP. This seems to have the same root cause as this issue. The hack we use is indeed to do a second pass of Works fine, but still would be nice if this is fixed. Let me know if I can help. |
@tschuchortdev @yigit object KSPRuntimeCompiler {
fun compile(compilation: KotlinCompilation): KotlinCompilation.Result {
val pass1 = compilation.compile()
require(pass1.exitCode == KotlinCompilation.ExitCode.OK) {
"Cannot do the 1st pass"
}
val pass2 = KotlinCompilation().apply {
sources = compilation.sources + compilation.kspGeneratedSourceFiles
}.compile()
require(pass2.exitCode == KotlinCompilation.ExitCode.OK) {
"Cannot do the 2nd pass"
}
return pass2
}
private val KotlinCompilation.kspGeneratedSourceFiles: List<SourceFile>
get() = kspSourcesDir.resolve("kotlin")
.walk()
.filter { it.isFile }
.map { SourceFile.fromPath(it.absoluteFile) }
.toList()
} |
that is what we are doing right now in room: it is not ideal because it will always fail for java files accessing kotlin generated files, hence, we cannot use the result of the first compilation output. |
@tschuchortdev circling back on this. Did you have any time to look into this or do you anticipate if you will? |
@yigit I've reviewed it over the holidays and have been thinking about ways to improve it since then. Here are some thoughts:
|
This was a question I asked to the KSP team as well :) Right now, there is no ordering between these two tasks. And if a developer tries to use them in a way that order matters, they'll need to control the ordering in Gradle. So in my prototype, for ordering, i didn't put any guarantees between KAPT- KSP as it does not exist in the gradle setup right now.
+1
I was trying to avoid breaking the API as much as possible. Maybe we can re-thing those APIs and call it 2.0 ? You mentioned this before also, I think i'm just traumatized working on Android where breaking APIs is almost impossible :)
I thought giving a compilation util would allow more customizations and better isolation.
Yea, we can go w/ a more strict API. My concern was that, if someone wants to use existing files, we could avoid copying them if their task does not require copying them. |
Hi @yigit, @tschuchortdev. I'm interested in this rewrite. I think with the introduction of ksp the separation of kapt from the core components makes a lot of sense. What's the current state of things? Is anyone working on this? Can I help? |
I know this is an old post but hopefully this will help someone: import com.google.devtools.ksp.processing.SymbolProcessorProvider
import com.tschuchort.compiletesting.KotlinCompilation
import com.tschuchort.compiletesting.SourceFile
import com.tschuchort.compiletesting.kspWithCompilation
import com.tschuchort.compiletesting.symbolProcessorProviders
import org.jetbrains.kotlin.compiler.plugin.ExperimentalCompilerApi
import java.io.File
@OptIn(ExperimentalCompilerApi::class)
object KspTestUtil {
fun compile(
sourceFiles: List<SourceFile>,
symbolProcessorProviders: List<SymbolProcessorProvider>,
): KotlinCompilation.Result {
return KotlinCompilation().also { compiler ->
compiler.sources = sourceFiles
compiler.inheritClassPath = true
compiler.symbolProcessorProviders = symbolProcessorProviders
compiler.messageOutputStream = System.out
compiler.kspWithCompilation = true
}.compile()
}
fun getSourcesFromResult(result: KotlinCompilation.Result): List<File> {
val kspSourcesDir = result.outputDirectory.resolve("../ksp/sources")
return kspSourcesDir.walkTopDown()
.filter { it.isFile && it.extension == "kt" }
.toList()
}
} Specifically |
Not sure what happened yet but between
20201106
and20201023
versions of KSP, it started having a side effect where no kotlin compilation error is reported.Removing
symbolProcessors = listOf(instance)
line fixes the test.Recently, KSP re-implemented its plugin so this might be related to google/ksp@3287061.
Notice that the error is not from KSP processor, it simply drops the error in kotlin source code.
The text was updated successfully, but these errors were encountered: