Skip to content

Remove output deletion that breaks incremental compilation#758

Closed
r0adkll wants to merge 2 commits intosquare:mainfrom
r0adkll:dh/remove-output-deletion
Closed

Remove output deletion that breaks incremental compilation#758
r0adkll wants to merge 2 commits intosquare:mainfrom
r0adkll:dh/remove-output-deletion

Conversation

@r0adkll
Copy link

@r0adkll r0adkll commented Oct 25, 2023

With @RBusarow fix (here: #693 (comment)) adding the anvil src gen directory to the task outputs for KotlinCompile fixes the incremental compilation.

Now the compiler plugin doesn't need to wipe the output directory since this will end up deleting all of anvil's generated output. This happens because the anvil compiler is invoked first with the list of changed files. This would leave only the changed files with generated outputs. However, the plugin is then invoked again with an empty list of files and then wipes the output directory with no files generated. I'm not sure why the final invocation is getting passed an empty list as IC mechanisms/Compiler Plugins are new for me

Steps to reproduce

gw clean :sample:library:assemble

Observing that all anvil generated code is present

Then changing any file in :sample:library and compile again

gw :sample:library:assemble

Now there won't be any output in the anvil src-gen-{sourceSet}.

Solution

If we remove the deletion code here

codeGenDir.listFiles()
      ?.forEach {
        check(it.deleteRecursively()) {
          "Could not clean file: $it"
        }
      }

then rerun the steps above we'll see that all the outputs changed accordingly and aren't deleted. Additionally you could try deleting files, or converting RealFatherProvider from an object to an injectable constructor class, then vice versa. This should result in the output updating each time.

@CLAassistant
Copy link

CLAassistant commented Oct 25, 2023

CLA assistant check
All committers have signed the CLA.

@vRallev
Copy link
Collaborator

vRallev commented Oct 26, 2023

Why not add the workaround for the output directory to the Gradle plugin?

@r0adkll
Copy link
Author

r0adkll commented Oct 26, 2023

@vRallev Ah, good call! Let me update.

@JoelWilcox
Copy link
Contributor

I believe Rick is working on applying the fix as well but has had it pending while getting integration test coverage in place; cc @RBusarow would be good to get your eyes on this and any thoughts on coordinating the changes here with what you have planned.

@scana
Copy link

scana commented Nov 7, 2023

Thanks @r0adkll!

Tested your changes on company's project and initially it seemed to work great with:

  • changing constructors parameters (adding/removing)
  • changing bound implementations (@ContributeBinding)

However I've noticed that if on branch X we'll have some new code akin to this and compile it:

class SomeClass @AssistedInject constructor(id: String) {
  @ContributesTo(AppScope::class)
  interface Component {
    fun someFactory(): SomeFactory
  }
  
  @AssistedFactory
  interface SomeFactory {
    fun create(id: String): Some
  }
}

switching back to branch without those classes will result in the following error (names changed a bit, but pointing to those two above):

> Task :modules:profile:compileDebugKotlin
e: file://scana-project/modules/feature/build/anvil/src-gen-debug/anvil/hint/merge/com/scana/some-module/util/SomeClass_Component.kt:12:40 Unresolved reference: SomeClass
e: file://scana-project/modules/feature/build/anvil/src-gen-debug/com/scana/some-module/util/SomeClass_Factory.kt:25:40 Unresolved reference: SomeClass

Removing those two files manually seem to resolve the issue for the time being.
Perhaps this cleanup should happen eventually during the incremental process - or at least partial cleanup.

@r0adkll
Copy link
Author

r0adkll commented Nov 28, 2023

@scana Yah, we've been experiencing the same issue along with some other caching oddities that we've had to roll this fix back. I'm not entirely sure that this is the path to fixing the IC issue and think that KSP might be the closer term goal for that. I know Block is internally working on this issue and they might have some closer term resolution, but I don't really have more insight then that.

@JoelWilcox
Copy link
Contributor

Thanks again for the PR; closing now in favor of #836

@JoelWilcox JoelWilcox closed this Feb 6, 2024
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

Successfully merging this pull request may close these issues.

5 participants