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.

Sign up for GitHub

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

Jump to bottom

[KSP] Incremental symbol processing not working (or working poorly) #4549

Closed
mar3kk opened this issue Dec 20, 2024 · 6 comments
Closed

[KSP] Incremental symbol processing not working (or working poorly) #4549

mar3kk opened this issue Dec 20, 2024 · 6 comments

Comments

@mar3kk
Copy link

mar3kk commented Dec 20, 2024

Does dagger/hilt KSP actually support incremental symbol processing? It seems changing any java or kotlin file in our project, even one that seemingly shouldn't contribute to the output, causes the ksp to re-run completely.
Analyzing the KSP log files kspDirtySet.log and kspSourceToOutputs.log turns out most of the times the files considered dirty are only used to produce some proguard rules. There are a lot of entries ending _HiltModules_KeyModule_LazyClassKeys.pro. Feels like something that could be skipped when minification is disabled.
@mar3kk mar3kk changed the title [KSP] Incremental symbol processing not working (or working badly) [KSP] Incremental symbol processing not working (or working poorly) Dec 20, 2024
@bcorso
Copy link

bcorso commented Dec 26, 2024

Does dagger/hilt KSP actually support incremental symbol processing?

Hmm, yes, it should support incremental processing.

In KSP the incremantal processing is determined by the mode of each output (isolating/aggregating). For the LazyClassKey proguard files, this is setup here.

If you're seeing an issue specifically with the .pro files it could be a bug related specifically to resource file outputs (since most of Dagger's outputs are source files, it could explain why we didn't hit this before now). However, that would be something we'd need to get fixed in KSP rather than Dagger.

We're a little back logged right now, so if you could provide a minimal repro example that could help get this rolling.

@bcorso
Copy link

bcorso commented Dec 27, 2024 •
edited
Loading

Actually, nm. @Chang-Eric pointed out that we were forgetting the originating element for the resource file (https://github.com/google/dagger/blob/dagger-2.54/java/dagger/internal/codegen/processingstep/LazyClassKeyProcessingStep.java#L114).

I'll send out a fix for that soon.

copybara-service bot pushed a commit that referenced this issue Dec 27, 2024
@bcorso
Fix LazyClassKey proguard file issues.
40488e9 
  * Fixes the name of the proguard file to contain an underscore between the package and simple names (e.g. `test.FooModule` was `testFooModule_LazyClassKeys.pro` and is now `test_FooModule_LazyClassKeys.pro`).
  * Fixes incremental processing by adding the originating element to the `writeResource` call (#4549).
  * Fixes bug where `StringBuilder` was declared outside module for-loop, which could lead to duplicate entries across proguard rules for different modules.
  * Fixed `ClassName#toString()` usage to `ClassName#canonicalName()`, since `toString()` is ambiguous and will silently break things when we migrate to `XClassName`.
  * Added test coverage for the proguard file name and contents.

Fixes #4549

RELNOTES=Fixes #4549: Fixes incremental processing for LazyClassKey proguard files by adding the originating element to the `writeResource` call.
PiperOrigin-RevId: 709861817
@copybara-service copybara-service bot mentioned this issue Dec 27, 2024
Closed
@mar3kk
Copy link
Author

mar3kk commented Dec 27, 2024 •
edited
Loading

Thanks @bcorso for fixing it promptly, however I do get compilation error on the latest HEAD-SNAPSHOT.

e: [ksp] MultibindingAnnotationsProcessingStep was unable to process 'binds(com.opera.android.privatedownloads.customisation.PrivateFolderCustomisationViewModel)' because 'error.NonExistentClass' could not be resolved.

Update:
Oh, NVM, after applying resolution strategy described here https://dagger.dev/dev-guide/versions.html it compiled.

@bcorso
Copy link

bcorso commented Dec 27, 2024

Thanks @mar3kk for trying it out and verifying the fix!

We'll get the 2.54.1 release out next week, though it might be later in the week due to the holidays.

@bcorso
Copy link

bcorso commented Dec 27, 2024 •
edited
Loading

after applying resolution strategy described here https://dagger.dev/dev-guide/versions.html it compiled.

@mar3kk just to confirm (since I noticed you only said "it compiled"), did you verify if it fixed the incremental processing issues you were seeing?

@mar3kk
Copy link
Author

mar3kk commented Dec 27, 2024

Yes, I didn't verify the fix back then when I posted the message. I gave it a try and I think the issue reported here got fixed. I no longer see that many files contributing to the generated proguard rules. Thanks!

However I still see a hundreds of files marked dirty in the kspDirtySet.log after changing single .java file that is not listed as contributing to any output in the kspSourceToOutputs.log file. I am trying to figure it out but feels KSP cannot tell why those files are considered dirty. I don't think this is necessarily a dagger issue though, we have more ksp processors in the project and KSP doesn't help not producing some log files that could help finding the root cause which I reported here google/ksp#2277.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix LazyClassKey proguard file issues. google/dagger
2 participants
@bcorso @mar3kk