-
Notifications
You must be signed in to change notification settings - Fork 79
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
Only clear the output dir the first round #720
Conversation
Hopeefully resolves square#693
?.forEach { | ||
check(it.deleteRecursively()) { | ||
"Could not clean file: $it" | ||
if (compilationRound == 1) { |
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.
If I'm reading correctly, wouldn't this still behave the same as the existing behavior? Because we already have the didRecompile
flag that should prevent us getting to this point after the first invocation of analysisCompleted
@@ -59,6 +60,7 @@ internal class CodeGenerationExtension( | |||
bindingTrace: BindingTrace, | |||
files: Collection<KtFile> | |||
): AnalysisResult? { | |||
compilationRound++ | |||
if (didRecompile) return null | |||
didRecompile = true |
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.
I'm now confused. This boolean should make sure that we never delete files in the 2nd round.
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.
I think this was doing the inverse - it would always wipe the 2nd+ round when this seeks to only wipe the first round it processes. Should we remove the didRecompile? I wasn't quite sure how it worked
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.
Let me clarify the use case. We need to wipe the directory to avoid any stale files. The compiler APIs require a directory as additional root, it would be easier if we could just return a set of files.
What I've seen in the past is that analysisCompleted
gets called multiple time with the same inputs. That's why I've added the didCompile
boolean flag. It's false
by default, which will make all the code run during the first call, but not any further calls. Therefore we only wipe the directory per compilation once and then not again. This counter isn't necessary.
What I understood from the ticket is that the inputs analysisCompleted
are changing now and we therefore should run our code, but not wipe the directory again. Is this correct?
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.
I'm not sure now. @hungvietnguyen could you offer insight?
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.
I mentioned this in #693 (comment), but clarifying it a bit more here:
- The Kotlin incremental compiler (IC) runs in multiple rounds (this while loop).
- In each round, the IC invokes the Kotlin compiler, which will instantiate new instances of compiler plugins, so keeping a state such as
didRecompile
only works within a round and doesn't work across rounds. - To avoid confusion: Here, a round = a Kotlin compiler invocation. In each round, it may call com.squareup.anvil.compiler.codegen.CodeGenerationExtension#analysisCompleted multiple times.
In the bug, Anvil was generating outputs correctly in the first round, but then deleting them in the second round.
It might be easier if you use the sample project and attach a debugger to Anvil to see 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.
Looking at the Kotlin compiler invocation in the IC while loop, it seems that each call is indepedent and they don't track compilation rounds.
Alternatively, should anvil clear its output dir at the Gradle level before kotlincompile runs?
In a non-incremental run, output directory cleanup is usually already handled by Gradle / AGP / KGP. It's not necessary (but also doesn't hurt) if Anvil does the cleanup.
In an incremental run, output directory should not be cleaned up because the essence of an incremental run is that parts of the output are replaced while the other parts remain intact.
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.
Hmm, what if it worked like this:
- Anvil always clears the output dir first before the KotlinCompile task runs (via doFirst)
- Anvil creates the output dir the first time it runs, never clears it during the KotlinCompile task. If the output dir exists, assume that anvil has already run a previous round.
That sort of solves the "has a previous round run" problem by never trying to intligently determine rounds
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.
- Anvil always clears the output dir first before the KotlinCompile task runs (via doFirst)
It's okay to clean up the output in a non-incremental run, but in an incremental run, the output cannot be cleaned up. The key idea of incrementality is that we partially re-generate only some of the output. If we delete all of the output, then that means we'll need to re-generate all of them, which means we don't have incrementality.
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.
Hmm. Does anvil need to clear outputs at all? If kotlincompile Will do that for us? Otherwise I kind of would expect anything generating code during AnalydisHandlerExtension would be incompatible with IC
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.
It depends:
- In a non-incremental run, Gradle / KGP should clean the outputs, it's optional for Anvil to do it.
- In an incremental run, no one should clean the outputs.
Otherwise I kind of would expect anything generating code during AnalydisHandlerExtension would be incompatible with IC
If Anvil's output files have 1-1 correspondence with Kotlin source files, then I think it can be made compatible (similar to isolating annotation processors). Otherwise, it needs more investigation.
Hopeefully resolves #693