-
Notifications
You must be signed in to change notification settings - Fork 88
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
Compiler doesn't pick incremental changes to multibindings in some circumstances #693
Comments
I've seen this under similar circumstances too, but only for code removals like you pointed out. In some cases Anvil's generated code stayed around and referenced deleted code, which caused compilation failures. |
New incremental compilation is now enabled by default in Kotlin 1.8.20: https://kotlinlang.org/docs/whatsnew1820.html#new-jvm-incremental-compilation-by-default-in-gradle And since many Android projects include Jetpack Compose compiler, I suspect this issue will now get much more widespread. |
The sample project was very useful! I found that when Because of this, the However, Anvil seems to rely on the I guess we'll need to figure out [1] first. From there, it will be clear whether the bug is in the new IC or in Anvil. |
I've filed https://youtrack.jetbrains.com/issue/KT-57919/Kotlin-class-metadata-doesnt-contain-info-about-class-annotations for the Kotlin team to take a look. |
Thanks! |
Hey team is there any workaround for this issue? We found that every time we touch any class with Also is it the root cause because of this https://kotlinlang.org/docs/whatsnew1820.html#new-jvm-incremental-compilation-by-default-in-gradle? If yes does it mean turning it somehow off should address the issue? Btw we are on 1.8.20 so this issue persist not only in 1.8.10. Update: disabling incremental compilation |
Can you try turning new incremental compilation off with |
The old IC uses metadata too iirc, don't think changing the IC will help |
|
For us, issue only occurs when new IC is turned on (on 1.8.10, we did not update to 1.8.20 yet).
Did you try setting it to false? |
I wanted to provide a bit more context on why only the new IC has this issue. Both the new IC and the old IC rely on Kotlin class metadata to detect a change. Because the Kotlin class metadata currently doesn't contain info about class annotations (https://youtrack.jetbrains.com/issue/KT-57919), if we add/remove a class annotation, both the new and old IC will not detect a change. The difference is that the new IC supports compile avoidance, which means that the task (in this case, As long as the Therefore, the new IC make its easier for this bug to show up, especially when used with Anvil. |
This bug should only happen if you add/remove the Btw, the workaround is to set If you don't find the above to be the case, then that is probably a different bug. |
Would the workaround be to just set |
Yes, I think that's another workaround. However, there will be some performance impact: Even if the task can run fast, it runs for all builds even when we don't make any changes. |
Sorry folks for confusion I meant we tried this option
I bet we saw this issue even when we add a new argument (dependency) to the inject constructor, its not just adding / removing annotation but even changing the dependencies. |
That sounds like a different bug. It would be great if you could file another bug with a sample project. |
@hungvietnguyen we're still seeing this issue fail in Kotlin 1.9.0-Beta with the fix in IC |
There seem to be 2 issues here: [Fixed] Issue 1 -
|
Hopeefully resolves square#693
Thank you @hungvietnguyen!! I've taken a shot at a possible fix in #720, would be curious for your thoughts. Re: this
Anvil currently does this due to this.
I'd be curious if there's a way we can get around that limitation in a way that is IC friendly? |
Sure, I've left a comment on #720. Regarding an IC-compatible solution, I have no idea, but I think we can start by refining what "incremental compilation tricked us" means. Once we've tracked that down, usually it's either a bug in the IC or some missing features for compiler plugins that the Kotlin compiler should provide. In both cases, it will be a bug/request for Kotlin team, and we can go from there? |
![]() https://github.com/JetBrains/kotlin/releases/tag/v1.9.0 Well, now Kotlin fix issue 1, what need for issue 2? #693 (comment) |
Quick update on where we're at with this: Currently there doesn't appear to be a feasible way for us to completely fix this issue without a new IC API for compiler plugins. That new API is being tracked in KT-51733. Separately, we've also begun working on KSP support for Anvil. KSP has the type of IC API that we need in-place already, which should help to resolve this issue. However, I can't give an estimate on when KSP support will be ready since we are just starting and it will also depend on when Dagger finishes adding its KSP support. In the meantime, the best known work-around is still using the old IC by setting You may also notice slightly fewer instances of incremental compilation issues with Kotlin 1.9.0 and Anvil 2.4.7+ thanks to KT-58289 being fixed. |
Thanks for the update. Are the IC regressions in the recent versions (such as #710) also caused by the same issue? |
I believe the workaround is to set I wouldn't recommend setting
|
Yes, as far as I can tell. I retested the use-case from #710 and observed the same behavior outlined in issue 2 around the difficulties with managing the generated output files.
Thanks for expanding on my comment @hungvietnguyen, I could have been clearer about it being situationally helpful for reducing the incremental issue occurrences (as opposed to a complete work-around).
Also adding a quick clarification on this ^: although full Anvil KSP support will depend on Dagger KSP, adding KSP support for factory generation is something we can do independently. So barring any major gotchas, that piece should some a bit sooner 🤞. |
Hi @JoelWilcox, just wanted to follow up if Also - as Dagger has just brought KSP support are there any gaps in Anvil that could be filled in by external contributors to help solving this issue? |
👋 No updates to share on this currently.
Thanks for asking! We always appreciate contributions from the community, but for now I think it's easier to keep it as an internal effort, at least until we're farther along in the initial support. I'll continue to provide updates here if that changes, as well as when there's other major milestones to report. |
Does anyone found a workaround that allows updating Kotlin version without disabling incremental compilation? |
Yes. TL;DR fixTo solve it for yourself immediately, put this in your Anvil project(s) or in a convention plugin: Kotlin DSL (build.gradle.kts)tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>().configureEach {
// Kapt tasks are also KotlinCompile tasks, but we don't care about them
if (this !is org.jetbrains.kotlin.gradle.tasks.KaptGenerateStubs) {
val anvilSrcGenDir = layout.buildDirectory.dir(sourceSetName.map{ "anvil/src-gen-$it/anvil" })
// adds the Anvil directory to the task's outputs
this.outputs.dir(anvilSrcGenDir)
}
} Groovy DSL (build.gradle)tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinCompile).configureEach {
// Kapt tasks are also KotlinCompile tasks, but we don't care about them
if (!(this instanceof org.jetbrains.kotlin.gradle.tasks.KaptGenerateStubs)) {
def anvilSrcGenDir = layout.buildDirectory.dir(sourceSetName.map { "anvil/src-gen-$it/anvil" })
// adds the Anvil directory to the task's outputs
this.outputs.dir(anvilSrcGenDir)
}
} Why?Anvil's output directory isn't being added to the Assume you have a single package com.foo
import com.squareup.anvil.annotations.ContributesMultibinding
@ContributesMultibinding(String::class)
class MyClass : MyInterface
interface MyInterface Handling Deletions and Build CacheThis script reproduces running a build on an Anvil module where the sources aren't in the build directory, but they are cached (locally, in this case). ./gradlew compileKotlin
rm -rf lib/build
./gradlew compileKotlin -i The second
Note that there are only three outputs listed, and none of them have anything to do with Anvil:
Those three outputs were restored from cache, but the Anvil-generated code was not. That means no hints, no factories, and somewhere downstream, no Dagger bindings for Now if you apply the fix above run the same three commands, the console output will be:
Notice that the Anvil-generated code is now listed as an output, and it has now been restored. That means compilation can continue as intended. Handling Incremental ChangesIncremental builds in Gradle and Kotlin both monitor outputs. Without the fix from above, if you comment out the annotation in (full output)
Here's the important part:
Kotlin is not watching the Anvil-generated code as part of its incremental compilation logic. That's why we're here. Now if I:
The output is now: (full output)
And here's that same important part:
KGP is populating its incremental compilation outputs based upon the outputs of the Permanent Fix?I'll do a quick update to This issue highlights a lack of test coverage around the Gradle plugin itself. There's currently nothing in the way of Gradle Test Kit tests. I'll be adding the scaffolding and tests themselves within the next few days. Hopefully we can |
Thanks Rick, I'll give this a try! I want to give some background why the output directory with generated files never was added. The idea is that the main input files are the source of truth. Based on the main input Anvil generates source files, which then get compiled to class files. There's the mapping of the main source code to this modified set of class files. Anvil's generated source code is only a by-product that doesn't need to be restored from cache. The important piece that needs to be restored in the output is the class files and not the generated source code. I'm afraid that this will only mask the fundamental gaps with incremental compilation, but I may be the wrong. I can also see a scenario happening where after making an incremental change kotlinc will have to run twice before the output is cached. Test cases for that will be a big help, so I'm looking forward to that. |
Thanks for the workaround! After some testing, it appears the workaround breaks UP-TO-DATE checks for ksp tasks. All ksp tasks in modules where anvil is seem to always execute, even without changes. Info from build scan points to this workaround: It seems that ksp task extends tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>().configureEach {
if (this !is KspTaskJvm && this !is KaptGenerateStubsTask) {
val ssName = sourceSetName.get()
val anvilSrcGenDir = layout.buildDirectory.dir("anvil/src-gen-$ssName/anvil")
// adds the Anvil directory to the task's outputs
outputs.dir(anvilSrcGenDir)
}
} |
The workaround does not need to be applied to the KSP task. Anvil doesn't run in the KSP task. Your solution seems valid. |
I'm using // ....
val ssName = when {
sourceSetName.isPresent -> sourceSetName.get()
name == "compileTestFixturesKotlin" -> "testFixtures"
else -> error("Cannot determine sourceSetName for task $name")
}
// .... EDIT. After I brought up my project to working state, the workaround doesn't work for me. The |
@svilen-ivanov can confirm, I received exactly the same issue when tried to launch :sample:app:assembleDebug. Steps to reproduce:
Error will be returned during
Project |
I think the problem might be deeper than that as the KspTask doesn't extend KotlinCompile, so I'm not sure why it's triggering here 🤔 |
I think the problem lies with how Anvil always clears the generated code (link). Even with @RBusarow solution, there just isn't any Anvil code to reference after incremental compilation. I ran into a similar issue #754 and the following patch fixes it #755. I don't have a strong understanding of how compiler plugins work, so it would be good for someone with more context to evaluate this workaround. |
|
lurker here; so we're unable to fix this & are waiting for dagger ksp+anvil support of that? |
If you use custom Anvil code generators under your own namespace, make sure to remove the |
adding the code above to add it as an output + custom code generator adjustment worked like a charm! |
@RBusarow thanks for the workaround, I see when we add the new output in the task we are producing overlapping outputs: https://ge.solutions-team.gradle.com/s/pk6mtthejsday/timeline?details=j7bq5uuamihla |
This seems fixed by |
I can confirm the beta version works for my team after opting in to Updating my custom code generator to use Thanks to everyone who helped figure this out and resolved it. 👏 |
When:
@ContributesMultibinding
annotation in a library modulekotlin.incremental.useClasspathSnapshot=true
ingradle.properties
)Changes on those annotations may not get picked up when making incremental builds.
Steps to reproduce:
@ContributesMultibinding
annotationThe text was updated successfully, but these errors were encountered: