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

compilation errors are hidden when KSP is used #72

Open
yigit opened this issue Nov 10, 2020 · 27 comments
Open

compilation errors are hidden when KSP is used #72

yigit opened this issue Nov 10, 2020 · 27 comments

Comments

@yigit
Copy link
Contributor

yigit commented Nov 10, 2020

Not sure what happened yet but between 20201106 and 20201023 versions of KSP, it started having a side effect where no kotlin compilation error is reported.

@Test
    fun badCodeFailsCompilation() {
        val src = SourceFile.kotlin("Foo.kt", """
            class Foo {
                val x:NonExistingType = TODO()
            }
        """.trimIndent())
        val instance = mock<SymbolProcessor>()
        val result = KotlinCompilation().apply {
            sources = listOf(src)
            symbolProcessors = listOf(instance)
        }.compile()
        assertThat(result.exitCode).isEqualTo(ExitCode.COMPILATION_ERROR)
    }

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.

@yigit
Copy link
Contributor Author

yigit commented Nov 10, 2020

Discussed with KSP folks.
It has been changed to be a completely separate task and its output gets fed into the kotlin compilation as source inputs.

google/ksp@3287061#diff-a345032d6c965be6e81478437767cb750c83bd0a952014661e86d454ea5a342bR132

Seems like the "proper" fix here is to run two compilations if symbol processors are provided.

@tschuchortdev
Copy link
Owner

Thanks for the heads up. I will look into this in December when I have some free time again.

@yigit
Copy link
Contributor Author

yigit commented Nov 12, 2020

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.
The proper solution is to make it work like the plugin where if KSP options are provided, we run a separate compilation (but a clone of the current one). That is more challenging than it looks because KotlinCompilation class is very stateful, copying it wont be easy.
Alternatively, i might try to add a hook to it to run KSP anaysis first, copy its output into sources and continue compilation (so run Step 3 for KSP, then Step 1,2,3,4 as documented in the KotlinCompilation.compile method).
If I can make that version make, maybe we can add KSP dependency on the main artifact BUT annotate symbolprocessors property with @Experimental. Initially we kept it separate due to kotlin 1.4 dependency but that is no longer the case since 1.4 is stable.
Wdyt?

@yigit
Copy link
Contributor Author

yigit commented Nov 12, 2020

ok here is the prototype of the second option:
yigit@9b7b52e

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.
I'm leaning towards that option as well because it better mimics the KSP behavior (also fixes another issue google/ksp#119) .
We can mark all KSP related interfaces experimental and i can clean the implementation to pull ksp related fields into KotlinCompilation and properly register the plugin on demand.

wdyt?

@tschuchortdev
Copy link
Owner

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:

  1. KSP itself does not respect abstraction boundaries and meddles directly in the Kotlin compilation process (I wouldn't put it past Google)
  2. KCT is not composable enough and doesn't offer the right abstraction/interface to plug into. Perhaps we need a slightly higher level API, which would also help with [ksp] Add support for reading generated resource files #70 and How can I test multi-module annotation processing? #56.

    The proper solution is to make it work like the plugin where if KSP options are provided, we run a separate compilation (but a clone of the current one)

That is more challenging than it looks because KotlinCompilation class is very stateful

I know, it's pretty terrible right now. If you have some concrete suggestions how to improve it, let me know.

@yigit
Copy link
Contributor Author

yigit commented Nov 14, 2020

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.

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)

  1. KSP itself does not respect abstraction boundaries and meddles directly in the Kotlin compilation process (I wouldn't put it past Google)
  2. KCT is not composable enough and doesn't offer the right abstraction/interface to plug into. Perhaps we need a slightly higher level API, which would also help with [ksp] Add support for reading generated resource files #70 and How can I test multi-module annotation processing? #56.
    I don't this this is an abstraction problem in KSP. It does not break anything in Kotlin Compilation using a private API or anything like that. The plugin API of Kotlin Compiler already has the infra to say "i'm done, stop processing further) and they are using that API to stop compilation. So for KSP, it does actually make sense to be an isolated task to do its work though it will likely be a problem for interoperability with other plugins.

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.
In addition to these, it is not as simple as re-running compilation as there is only 1 step we want to run for KSP (3). So another option could be to leak this implementation detail of KSP to the clients of KCT where they need to configure and run a KSPCompilation task and feed its output to another KotlinCompilation task.

@yigit
Copy link
Contributor Author

yigit commented Nov 14, 2020

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 thought about creating another AbstractKotlinCompilation for KSP but i cannot because the class has internal constructor and it is clearly documented not to be inherited from outside the module so I don't think opening it up is an option.

I also thought about creating a KspCompilation that will have an internal delegate of KotlinCompilation but I cannot simply call compile on KotlinCompilation because I don't want java compilation (nor kapt) to run when processing KSP. (java compilation reports false negatives as the kotlin code is not compiled after KSP). We could add flags to turn them off but feels super hacky.
I could do it by adding KSP dependency to the main module but I know you don't want it (though it would not be adding to KotlinCompilation so maybe that is OK with you?)

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).

@tschuchortdev
Copy link
Owner

i cannot find an option w/o opening up KCT internals more

Which is fine as long as it makes sense and is not another hack.

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.

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.

@yigit
Copy link
Contributor Author

yigit commented Nov 14, 2020

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).
Let me give that a try where the steps can be a defined API in KCT. That should allow me to move KAPT related stuff into another class that implements that Step idea. It could also allow keeping KSP separate.
I'll prototype something to followup. Thanks for being flexible on this.

@yigit
Copy link
Contributor Author

yigit commented Nov 16, 2020

I'm still prototyping but i have a qq.
Is there a use case where compile can be called multiple times on a KotlinCompilation? In other words, they can right now but is it intentional or just undefined behavior?

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).
From there, we can have these additional steps which can receive an immutable model + compilation helpers and run their steps and only have limited API to contribute (currently thinking they can just provide an exit code + additional sources for next step).
The reason why i want to lock down the model is that, right now even if I abstract steps and give them KotlinCompilation as an argument, they might modify anything in it hence after each step, we may need to recalculate everything (e.g. sources).

@tschuchortdev
Copy link
Owner

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 compile can only be called once on a compilation configuration. It's probably a good idea to defer side-effecting operations and make clear to the user when side-effects will be executed. In a functional language like Haskell or Scala we could build up a tree that represents the compilation/task graph with an initial f-algebra, which can then be manipulated by higher-level tasks but in Kotlin that won't be possible without pulling in some very heavy functional programming libraries (or lots of syntactic annoyances).

they might modify anything in it hence after each step, we may need to recalculate everything (e.g. sources)

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.

@yigit
Copy link
Contributor Author

yigit commented Nov 17, 2020

It kind of depends where we want to put the abstraction.
That is, for each rule, the API contract might be less loosely defined such that:
runStep(compilation : KotlinCompilation) : ExitCode
Then we would let it modify KotlinCompilation the way it wants (e.g. kapt would call addSources for each file it generates).
If we do that, we would need to recalculate sources for each of them (or ask them to do it).
(e.g. not do this https://github.com/tschuchortdev/kotlin-compile-testing/blob/master/core/src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt#L606 or do it for every step, or ask every step to be responsible to run it)

If we let them modify KotlinCompilation and also let developer call compile multiple times, side effects become a bigger problem.
e.g. what happens when I prepare a KotlinCompilation, run it with kapt which would add more sources and then just call compile again on it. The second compilation will have generated sources from the first one which would be a very surprising behavior from API perspective. That is why, if we want to let user call compile multiple times, it is better if that operation never changes the KotlinCompilation object (even its internals).

The API i was thinking of was more like:

runStep(compilation : ImmutableKotlinCompilation, sources : List<File>) : IntermediateResult

where intermediate result would be something like:

data class IntermediateResult(val exitCode:ExitCode, generatedSources : List<File>, anything else)

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 Result class is an inner class and refers to KotlinCompilation fields. If we want compile to be callable multiple times, that should change as well (which is totally doable).
wdyt?

@tschuchortdev
Copy link
Owner

If we do that, we would need to recalculate sources for each of them (or ask them to do it).
(e.g. not do this https://github.com/tschuchortdev/kotlin-compile-testing/blob/master/core/src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt#L606 or do it for every step, or ask every step to be responsible to run it)

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 compile function to make it more clear. It might become annoying syntactically when many actions are deferred, but then again, we have used flatMap all over for years with RxJava.

That is why, if we want to let user call compile multiple times, it is better if that operation never changes the KotlinCompilation object (even its internals).

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.

The second compilation will have generated sources from the first one which would be a very surprising behavior from API perspective.

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.

data class IntermediateResult(val exitCode:ExitCode, generatedSources : List<File>, anything else)

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?

@yigit
Copy link
Contributor Author

yigit commented Nov 21, 2020

If we do that, we would need to recalculate sources for each of them (or ask them to do it).
(e.g. not do this https://github.com/tschuchortdev/kotlin-compile-testing/blob/master/core/src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt#L606 or do it for every step, or ask every step to be responsible to run it)

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 compile function to make it more clear. It might become annoying syntactically when many actions are deferred, but then again, we have used flatMap all over for years with RxJava.

That is why, if we want to let user call compile multiple times, it is better if that operation never changes the KotlinCompilation object (even its internals).

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.

The second compilation will have generated sources from the first one which would be a very surprising behavior from API perspective.

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.

Sorry, I took steps can edit KotlinCompile as they can add sources, which would have an impact on later steps / compile calls.
Restricting them to report such extra changes in a well defined interface avoids these problems (kind of scopes it).

data class IntermediateResult(val exitCode:ExitCode, generatedSources : List<File>, anything else)

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?

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).
I'll ignore the current API of KotlinCompile (I think i got too attached to it). I should be possible to keep that api based on the new infra, at the least. I'll try to prototype something and get back to you.

@yigit
Copy link
Contributor Author

yigit commented Nov 23, 2020

Here is where I am w/ the prototype.
It supports both the KSP case I need and also your request to change followup compilations more significantly (e.g. register plugins)
(i'll link some sources to explain but please don't read into them, it is a prototype).

I've moved all data of KotlinCompilation (and friends) into model interfaces. (src)
So there is:

CompilationModel
JvmCompilationModel : CompilationModel
JsCompilationModel: CompilationModel

These interfaces are immutable and we have internal implementations for each that are mutable.

Then there is compilation steps which simply receive the current model + utilities to compile.
When executed, they return an IntermediateResult which includes an exit code and a model for followup steps:
https://github.com/yigit/kotlin-compile-testing/blob/step-based-compilation/core/src/main/kotlin/com/tschuchort/compiletesting/step/CompilationStep.kt#L20

class IntermediateResult<Params: CompilationModel>(
        val exitCode: KotlinCompilation.ExitCode,
        val updatedModel: Params,
        // outputs for Result, must be a folder
        val outputFolders: List<File>
    ) 

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 Result (hence please ignore that parameter, see below for TODO)

The nice thing about this setup is that each step can create its own model, delegate everything to the original and just override whatever it wants.
For instance, this is what the KaptCompilationStep returns after compilation:

private class OutputCompilationModel(
        val delegate: JvmCompilationModel,
        val kotlinCompilationClassesDir: File
    ) : JvmCompilationModel by delegate {
        override val classpaths: List<File>
            get() = delegate.classpaths + listOf(kotlinCompilationClassesDir)
    }

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:) )
Moreover, with kotlin 1.4, properies can be delegated to other properties so keeping KotlinCompilation API the same is also feasible:

class KotlinCompilation ... {
   private val model: JvmCompilationModelImpl() // mutable model, internal
   var includeRuntime: Boolean by model::includeRuntime
}

One API change I'm making is removing kapt parameters from KotlinCompilation. Instead, KotlinCompilationStep adds an extension to KotlinCompilation. This will be a breaking API change but i think it is good:
https://github.com/yigit/kotlin-compile-testing/blob/step-based-compilation/core/src/main/kotlin/com/tschuchort/compiletesting/step/KaptCompilationStep.kt#L204 (but we can still keep them in KotlinCompilation by delegating and deprecate those properties?)

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.

  • I tried to divide the model further (e.g. CompilationEnvironment for all host related stuff). Not very happy with it so will probably merge them all back to model an try again later.
  • Moved all kotlin compilation utilites into a util class that is passed into each step. This gives them infra to compile w/o accessing KotlinCompilation (so steps cannot change user parameters). Will probably keep it.
  • How we'll build result is still not resolved. Right now, Result is an inner class but I'll probably change it because compile can be called multiple times hence the contents of a previously returned result might change, we don't want that.
  • Output properties in KotlinCompilation, e.g. classesDir should move to result. That would allow us to isolate different runs (it is immutable anyways so I believe it should be fine).
  • Because each step returns a new model, things like generatedStubFiles in Result is not very clear. I'll probably have an API for steps to contribute to the final Result API. It should also be theoretically possible to find generated sources by diffing initial model (user input) and final model (after last compilation) but that might flake. Will followup there.

wdyt?

@yigit
Copy link
Contributor Author

yigit commented Nov 23, 2020

here is a prototype of KSP support w/ steps:

yigit@59abc25

@yigit
Copy link
Contributor Author

yigit commented Nov 23, 2020

btw i noticed while doing the KSP where for proper isolation for multiple compile calls, we need to move all output directories into result (they can still be extension functions).

Wdyt about that?
e.g. the generatedStubsDirectory could be contributed by the KspCompilationStep into the Result object as an extension property.

@yigit
Copy link
Contributor Author

yigit commented Nov 23, 2020

and here is a prototype of moving step specific data (e.g. kapt stubs) into result via extensions:
yigit@df07cdb

I'll stop here until you get a chance to review this. (no rush, room does have a workaround for now)
I don't particularly think this is an amazing solution overall but it checks all boxes and i think it is good enough.

  • KotlinCompilation API can stay the same
  • No need to add KSP dependency on main project
  • Kapt parts are isolated
  • Steps can affect followup steps w/o messing w/ user parameters.
  • Multiple compile calls are isolated

@yigit
Copy link
Contributor Author

yigit commented Dec 6, 2020

Hi,
I'm guessing you don't have time to look into this.
Should I just send a PR that does the quick fix (but proper) by adding the KSP dependency?
I can mark all KSP APIs experimental

@tschuchortdev
Copy link
Owner

@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.

@mvdbos
Copy link

mvdbos commented Dec 14, 2020

@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 compile, although we do create a new KotlinCompile instance for it, without a symbolProcessor set, and with all kspSourcesDir files from the first pass added as sources.

Works fine, but still would be nice if this is fixed. Let me know if I can help.

@vnermolaev
Copy link

@tschuchortdev @yigit
Cannot say whether it is helpful, but here is the (extremely suboptimal) code for the approach @mvdbos mentioned

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()
}

@yigit
Copy link
Contributor Author

yigit commented Dec 27, 2020

that is what we are doing right now in room:
https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:room/compiler-processing-testing/src/main/java/androidx/room/compiler/processing/util/runner/KspCompilationTestRunner.kt;l=67

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.

@yigit
Copy link
Contributor Author

yigit commented Jan 12, 2021

@tschuchortdev circling back on this. Did you have any time to look into this or do you anticipate if you will?

@tschuchortdev
Copy link
Owner

tschuchortdev commented Jan 12, 2021

@yigit I've reviewed it over the holidays and have been thinking about ways to improve it since then. Here are some thoughts:

  • HasExtensionData and StepRegistry make for a nice DSL and easy way to add compilation steps
  • If I'm not mistaken, the execution order of extensions is non-deterministic in the sense that it depends on which extension was called and thus registered first in the DSL. This could be quite confusing for users. For example, both KAPT and KSP can consume and produce Kotlin files, so a different execution order will yield different results. I don't know how this is handled in Gradle.
  • There are a few things that need to be cleaned up (for example findHostClasspath)
  • HostEnvironment.getResourcesPath has knowledge of MainComponentRegistrar, which is not ideal but I don't think it's worth changing now. MainComponentRegistrar should really be private to one of the KotlinCompilation classes but can't be because of the service locator.
  • There is still a lot of KAPT related stuff in the KotlinCompilation class. If KAPT is becoming optional, it would make sense to separate those things.
  • KotlinCompilation will always include KAPT only because the kapt extension property happens to be referenced in makeResult (but nowhere else). This should either be removed or made clear that KotlinCompilation will also include KAPT step
  • The many different Result and Model classes make it hard to follow the code. I'm trying to make this a bit more straight forward
  • I was thinking that it would be nice if, instead of using KotlinCompilationUtils, extensions which need to call the Kotlin compiler could "inspect" the compilation, find the KotlinCompilationStep/JsCompilationStep and simply wrap this existing step with altered parameters, without having to know about K2JVMCompiler and so on. But I don't know yet if it's possible and gives the required flexibility.
  • It is still the case that files and directories are created all over the place (both directories of the steps and inline source files that are written lazily). What do you think about making "write source files to disk" its own step and only giving paths to following steps? I think this is how Gradle does it

@yigit
Copy link
Contributor Author

yigit commented Jan 20, 2021

@yigit I've reviewed it over the holidays and have been thinking about ways to improve it since then. Here are some thoughts:

  • HasExtensionData and StepRegistry make for a nice DSL and easy way to add compilation steps
  • If I'm not mistaken, the execution order of extensions is non-deterministic in the sense that it depends on which extension was called and thus registered first in the DSL. This could be quite confusing for users. For example, both KAPT and KSP can consume and produce Kotlin files, so a different execution order will yield different results. I don't know how this is handled in Gradle.

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.

  • There are a few things that need to be cleaned up (for example findHostClasspath)
  • HostEnvironment.getResourcesPath has knowledge of MainComponentRegistrar, which is not ideal but I don't think it's worth changing now. MainComponentRegistrar should really be private to one of the KotlinCompilation classes but can't be because of the service locator.
  • There is still a lot of KAPT related stuff in the KotlinCompilation class. If KAPT is becoming optional, it would make sense to separate those things.

+1

  • KotlinCompilation will always include KAPT only because the kapt extension property happens to be referenced in makeResult (but nowhere else). This should either be removed or made clear that KotlinCompilation will also include KAPT step

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 :)

  • The many different Result and Model classes make it hard to follow the code. I'm trying to make this a bit more straight forward
  • I was thinking that it would be nice if, instead of using KotlinCompilationUtils, extensions which need to call the Kotlin compiler could "inspect" the compilation, find the KotlinCompilationStep/JsCompilationStep and simply wrap this existing step with altered parameters, without having to know about K2JVMCompiler and so on. But I don't know yet if it's possible and gives the required flexibility.

I thought giving a compilation util would allow more customizations and better isolation.

  • It is still the case that files and directories are created all over the place (both directories of the steps and inline source files that are written lazily). What do you think about making "write source files to disk" its own step and only giving paths to following steps? I think this is how Gradle does it

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.

@F43nd1r
Copy link

F43nd1r commented May 1, 2021

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?

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

No branches or pull requests

5 participants