kotlin icon indicating copy to clipboard operation
kotlin copied to clipboard

Support logging file accessed in Kotlin annotation processing

Open acejingbo opened this issue 3 years ago • 3 comments

Context:

Youtrack report: https://youtrack.jetbrains.com/issue/KT-52853

This change expanded KAPT to support a new param fileAccessHistoryReportFile, which report all the classed used during annotation processing into a file, in the form of a list of URIs.

This is useful for build speed improvements described in https://engineering.fb.com/2017/11/09/android/rethinking-android-app-compilation-with-buck/ . Essentially, using the list of used classes, we can compile only the dependencies that are really affected by the developer's code changes, and improve kotlin build speed

Test plan:

Tested in real life CLI based build tool Tested in unit test ./gradlew :kotlin-annotation-processing-base:test --tests "org.jetbrains.kotlin.kapt.base.test.JavaKaptContextTest". Test passed.

Use the test as an example, you can see the following line in log:

[INFO] Dumping KAPT file access history to /var/folders/x9/f8cztkfn3b9gpdth_4v2nyx00000gn/T/kotlin-annotation-processing-baseProject_test_2971344083190170958/kapt_access_history8753395934352559134.txt

Locally print the report file's content, you can see the following text content:

file:/private/var/folders/x9/f8cztkfn3b9gpdth_4v2nyx00000gn/T/kotlin-annotation-processing-baseProject_test_2971344083190170958/kaptRunner3607328635994579172/generated/MyMethodMyAnnotation.java
jar:file:/Library/Java/JavaVirtualMachines/jdk1.8.0_181.jdk/Contents/Home/lib/ct.sym!/META-INF/sym/rt.jar/java/io/Serializable.class
jar:file:/Library/Java/JavaVirtualMachines/jdk1.8.0_181.jdk/Contents/Home/lib/ct.sym!/META-INF/sym/rt.jar/java/lang/Comparable.class
jar:file:/Library/Java/JavaVirtualMachines/jdk1.8.0_181.jdk/Contents/Home/lib/ct.sym!/META-INF/sym/rt.jar/java/lang/Enum.class
jar:file:/Library/Java/JavaVirtualMachines/jdk1.8.0_181.jdk/Contents/Home/lib/ct.sym!/META-INF/sym/rt.jar/java/lang/Object.class
jar:file:/Library/Java/JavaVirtualMachines/jdk1.8.0_181.jdk/Contents/Home/lib/ct.sym!/META-INF/sym/rt.jar/java/lang/String.class
jar:file:/Library/Java/JavaVirtualMachines/jdk1.8.0_181.jdk/Contents/Home/lib/ct.sym!/META-INF/sym/rt.jar/java/lang/annotation/Annotation.class

acejingbo avatar Jun 17 '22 21:06 acejingbo

Minor cleanup

acejingbo avatar Jul 06 '22 19:07 acejingbo

Hi @ausatiy @abelkov I see the PR and Youtrack are both assigned. Any comments on the code or questions I can help answer?

acejingbo avatar Jul 06 '22 19:07 acejingbo

Hi @ausatiy @abelkov This is useful for build speed improvements described in https://engineering.fb.com/2017/11/09/android/rethinking-android-app-compilation-with-buck/, so really hope to achieve this... If you have any concern for the current approach, please comment and we can always adjust :D

acejingbo avatar Aug 03 '22 19:08 acejingbo

Hi @ausatiy @abelkov ! Could you please help check this out and give it a review? As a reminder, this is a fork for KAPT to report file usage, as we discussed in the meeting offline, this should be on the right track, and other alternative may not cover all file-access from processors. Feel free to lemme know if there's anything to adjust.

acejingbo avatar Nov 21 '22 21:11 acejingbo

@ausatiy @abelkov Is there any chance we can move with PR? We already have an offline conversation, but unfortunately things didn't move on

rybalkinsd avatar Feb 16 '23 17:02 rybalkinsd

Overall OK, but I would expand the commit message, adding the context from the PR, since it is not clear from the commit message alone, why fileAccessHistoryReportFile could be useful.

ilmirus avatar Jul 29 '23 20:07 ilmirus

@Tapchicoma @ilmirus Thanks for the review! Sorry i was offline last month. Now that i am back i hope to ask

(1) I am happy to fix the nit and commit msg. But I saw in https://youtrack.jetbrains.com/issue/KT-52853 it recently changed to "Remote Run". Should i still do nit fix suggested above, or should i avoid change to not disturb the remote run.

(2) I see "approved", but "Merging is blocked. The base branch restricts merging to authorized users". I suppose it means you will need to click the "merge" button, not me?

acejingbo avatar Aug 04 '23 21:08 acejingbo

Cherry-picked into master as 7a13173e6a5faab9d491c79f776250f9bad7b62e commit with additional changes.

Tapchicoma avatar Aug 23 '23 15:08 Tapchicoma