From 929fca03fd99dbf6893dc5e2c5ed44af05243e86 Mon Sep 17 00:00:00 2001 From: Ivan Gavrilovic Date: Wed, 10 Apr 2019 16:49:29 +0100 Subject: [PATCH] Fixes to KAPT classpath change detection 1) Fix tests to use canonical path for comparison because Mac was failing 2) Update current classpath snapshot only with the missing entries from the previous one 3) Clean-up code and style --- .../KaptIncrementalWithAggregatingApt.kt | 32 +++++----- .../gradle/KaptIncrementalWithIsolatingApt.kt | 6 +- .../kotlin/gradle/internal/kapt/KaptTask.kt | 63 +++++++++---------- .../internal/kapt/KaptWithKotlincTask.kt | 13 ++-- .../internal/kapt/KaptWithoutKotlincTask.kt | 4 +- .../kapt/incremental/ClasspathSnapshot.kt | 12 ++-- .../kotlin/kapt3/base/KaptContext.kt | 2 +- .../kotlin/kapt3/base/KaptOptions.kt | 9 ++- .../base/incremental/classStructureCache.kt | 4 +- .../base/incremental/IncrementalKaptTest.kt | 5 +- .../org/jetbrains/kotlin/kapt3/Kapt3Plugin.kt | 2 +- 11 files changed, 69 insertions(+), 83 deletions(-) diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithAggregatingApt.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithAggregatingApt.kt index 4dfa55e18db..41064a011a1 100644 --- a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithAggregatingApt.kt +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithAggregatingApt.kt @@ -45,10 +45,10 @@ class KaptIncrementalWithAggregatingApt : KaptIncrementalIT() { assertEquals( setOf( - fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/UseBKt.java").absolutePath, - fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/B.java").absolutePath, - fileInWorkingDir("build/tmp/kapt3/stubs/main/baz/UtilKt.java").absolutePath, - fileInWorkingDir("build/tmp/kapt3/stubs/main/foo/A.java").absolutePath + fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/UseBKt.java").canonicalPath, + fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/B.java").canonicalPath, + fileInWorkingDir("build/tmp/kapt3/stubs/main/baz/UtilKt.java").canonicalPath, + fileInWorkingDir("build/tmp/kapt3/stubs/main/foo/A.java").canonicalPath ), getProcessedSources(output) ) } @@ -61,11 +61,11 @@ class KaptIncrementalWithAggregatingApt : KaptIncrementalIT() { assertSuccessful() assertEquals( setOf( - project.projectFile("JavaClass.java").absolutePath, - fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/UseBKt.java").absolutePath, - fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/B.java").absolutePath, - fileInWorkingDir("build/tmp/kapt3/stubs/main/baz/UtilKt.java").absolutePath, - fileInWorkingDir("build/tmp/kapt3/stubs/main/foo/A.java").absolutePath + project.projectFile("JavaClass.java").canonicalPath, + fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/UseBKt.java").canonicalPath, + fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/B.java").canonicalPath, + fileInWorkingDir("build/tmp/kapt3/stubs/main/baz/UtilKt.java").canonicalPath, + fileInWorkingDir("build/tmp/kapt3/stubs/main/foo/A.java").canonicalPath ), getProcessedSources(output) ) @@ -115,13 +115,13 @@ class KaptIncrementalWithAggregatingApt : KaptIncrementalIT() { assertEquals( setOf( - fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/AA.java").absolutePath, - fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/AAA.java").absolutePath, - fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/BB.java").absolutePath, - fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/FooUseAKt.java").absolutePath, - fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/FooUseBKt.java").absolutePath, - fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/FooUseAAKt.java").absolutePath, - fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/FooUseBBKt.java").absolutePath + fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/AA.java").canonicalPath, + fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/AAA.java").canonicalPath, + fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/BB.java").canonicalPath, + fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/FooUseAKt.java").canonicalPath, + fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/FooUseBKt.java").canonicalPath, + fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/FooUseAAKt.java").canonicalPath, + fileInWorkingDir("app/build/tmp/kapt3/stubs/main/foo/FooUseBBKt.java").canonicalPath ), getProcessedSources(output) ) diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithIsolatingApt.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithIsolatingApt.kt index 342b0c1cb04..bd3e5547bcf 100644 --- a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithIsolatingApt.kt +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/KaptIncrementalWithIsolatingApt.kt @@ -47,7 +47,7 @@ class KaptIncrementalWithIsolatingApt : KaptIncrementalIT() { project.build("build") { assertSuccessful() - assertEquals(setOf(fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/UseBKt.java").absolutePath), getProcessedSources(output)) + assertEquals(setOf(fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/UseBKt.java").canonicalPath), getProcessedSources(output)) } project.projectFile("B.kt").modify { current -> @@ -58,8 +58,8 @@ class KaptIncrementalWithIsolatingApt : KaptIncrementalIT() { assertSuccessful() assertEquals( setOf( - fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/B.java").absolutePath, - fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/UseBKt.java").absolutePath + fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/B.java").canonicalPath, + fileInWorkingDir("build/tmp/kapt3/stubs/main/bar/UseBKt.java").canonicalPath ), getProcessedSources(output) ) diff --git a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptTask.kt b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptTask.kt index 53248d9b0b4..d1d050b82a5 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptTask.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptTask.kt @@ -167,48 +167,43 @@ abstract class KaptTask : ConventionTask(), TaskWithLocalState { protected fun getCompiledSources() = listOfNotNull(kotlinCompileTask.destinationDir, kotlinCompileTask.javaOutputDir) protected fun getIncrementalChanges(inputs: IncrementalTaskInputs): KaptIncrementalChanges { - val changedFiles = getChangedFiles(inputs) - - return if (isIncremental) { - val classpathChanges = if (changedFiles.isEmpty()) { - classpath.files - } else { - classpath.files.let { cp -> - changedFiles.filter { cp.contains(it) } - } - } - val classpathStatus = findClasspathChanges(classpathChanges) - when (classpathStatus) { - is KaptClasspathChanges.Unknown -> KaptIncrementalChanges.Unknown - is KaptClasspathChanges.Known -> KaptIncrementalChanges.Known( - changedFiles.filter { it.extension == "java" }.toSet(), classpathStatus.names - ) - } - } else { - KaptIncrementalChanges.Unknown - } - } - - private fun getChangedFiles(inputs: IncrementalTaskInputs): List { - return if (!isIncremental || !inputs.isIncremental) { + if (!isIncremental) { clearLocalState() - emptyList() - } else { - with(mutableSetOf()) { - inputs.outOfDate { this.add(it.file) } - inputs.removed { this.add(it.file) } - return@with this.toList() - } + return KaptIncrementalChanges.Unknown + } + if (!inputs.isIncremental) { + clearLocalState() + findClasspathChanges(classpath.files) + return KaptIncrementalChanges.Unknown + } + + val changedFiles = with(mutableSetOf()) { + inputs.outOfDate { this.add(it.file) } + inputs.removed { this.add(it.file) } + return@with this.toList() + } + + val classpathChanges = classpath.files.let { cp -> changedFiles.filter { cp.contains(it) } } + + val classpathStatus = findClasspathChanges(classpathChanges) + return when (classpathStatus) { + is KaptClasspathChanges.Unknown -> KaptIncrementalChanges.Unknown + is KaptClasspathChanges.Known -> KaptIncrementalChanges.Known( + changedFiles.filter { it.extension == "java" }.toSet(), classpathStatus.names + ) } } private fun findClasspathChanges(changedClasspath: Iterable): KaptClasspathChanges { - incAptCache!!.mkdirs() + val incAptCacheDir = incAptCache!! + incAptCacheDir.mkdirs() val startTime = System.currentTimeMillis() - val previousSnapshot = ClasspathSnapshot.ClasspathSnapshotFactory.loadFrom(incAptCache!!) - val currentSnapshot = ClasspathSnapshot.ClasspathSnapshotFactory.createCurrent(incAptCache!!, classpath.files.toList(), classpathStructure!!.files) + val previousSnapshot = ClasspathSnapshot.ClasspathSnapshotFactory.loadFrom(incAptCacheDir) + val currentSnapshot = ClasspathSnapshot.ClasspathSnapshotFactory.createCurrent( + incAptCacheDir, classpath.files.toList(), classpathStructure!!.files + ) val classpathChanges = currentSnapshot.diff(previousSnapshot, changedClasspath.toSet()) currentSnapshot.writeToCache() diff --git a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptWithKotlincTask.kt b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptWithKotlincTask.kt index 46777beac91..212e8602cbb 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptWithKotlincTask.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptWithKotlincTask.kt @@ -75,15 +75,10 @@ open class KaptWithKotlincTask : KaptTask(), CompilerArgumentAwareWithInput { - changedFiles = incrementalChanges.changedSources.toList() - classpathChanges = incrementalChanges.changedClasspathJvmNames.toList() - processIncrementally = true - } - else -> { - // do nothing - } + if (incrementalChanges is KaptIncrementalChanges.Known) { + changedFiles = incrementalChanges.changedSources.toList() + classpathChanges = incrementalChanges.changedClasspathJvmNames.toList() + processIncrementally = true } val args = prepareCompilerArguments() diff --git a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptWithoutKotlincTask.kt b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptWithoutKotlincTask.kt index 6d52e5cda31..5c6e162b23a 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptWithoutKotlincTask.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/KaptWithoutKotlincTask.kt @@ -66,6 +66,7 @@ open class KaptWithoutKotlincTask @Inject constructor(private val workerExecutor if (isVerbose) add("VERBOSE") if (mapDiagnosticLocations) add("MAP_DIAGNOSTIC_LOCATIONS") if (includeCompileClasspath) add("INCLUDE_COMPILE_CLASSPATH") + if (incrementalChanges is KaptIncrementalChanges.Known) add("INCREMENTAL_APT") } val optionsForWorker = KaptOptionsForWorker( @@ -77,7 +78,6 @@ open class KaptWithoutKotlincTask @Inject constructor(private val workerExecutor getCompiledSources(), incAptCache, classpathChanges.toList(), - incrementalChanges is KaptIncrementalChanges.Known, destinationDir, classesDir, @@ -172,7 +172,6 @@ private class KaptExecution @Inject constructor( compiledSources, incAptCache, classpathChanges, - processIncrementally, sourcesOutputDir, classesOutputDir, @@ -209,7 +208,6 @@ private data class KaptOptionsForWorker( val compiledSources: List, val incAptCache: File?, val classpathChanges: List, - val processIncrementally: Boolean, val sourcesOutputDir: File, val classesOutputDir: File, diff --git a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshot.kt b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshot.kt index 2dfbe94fc3f..7e1a2ceefd6 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshot.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/internal/kapt/incremental/ClasspathSnapshot.kt @@ -7,7 +7,6 @@ package org.jetbrains.kotlin.gradle.internal.kapt.incremental import java.io.* import java.util.* - open class ClasspathSnapshot protected constructor( private val cacheDir: File, private val classpath: Iterable, @@ -20,7 +19,7 @@ open class ClasspathSnapshot protected constructor( val computedData = dataForFiles(missingFiles) computedClasspathData.putAll(computedData) } - computedClasspathData.filter { files.contains(it.key) } + computedClasspathData } private val computedClasspathData: MutableMap = mutableMapOf() @@ -95,10 +94,11 @@ open class ClasspathSnapshot protected constructor( } } - // We do not compute structural data for unchanged files of the current snapshot for performance reasons. That is why we - // update the previous snapshot as that one contains all entries. - computedClasspathData.putAll(previousData) - computedClasspathData.putAll(currentData) + // We do not compute structural data for unchanged files of the current snapshot for performance reasons. + // That is why we reuse the previous snapshot as that one contains all unchanged entries. + previousData.filter { !computedClasspathData.containsKey(it.key) }.forEach { + computedClasspathData[it.key] = it.value + } val allImpactedClasses = findAllImpacted(changedClasses) diff --git a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptContext.kt b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptContext.kt index 12acf415acc..aa7dfe14866 100644 --- a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptContext.kt +++ b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptContext.kt @@ -59,7 +59,7 @@ open class KaptContext(val options: KaptOptions, val withJdk: Boolean, val logge cacheManager = options.incrementalCache?.let { JavaClassCacheManager(it) } - if (options.processIncrementally) { + if (options.flags[KaptFlag.INCREMENTAL_APT]) { sourcesToReprocess = cacheManager?.invalidateAndGetDirtyFiles( options.changedFiles, options.classpathChanges diff --git a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptOptions.kt b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptOptions.kt index c93116d03d1..29db2707ebe 100644 --- a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptOptions.kt +++ b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/KaptOptions.kt @@ -18,7 +18,6 @@ class KaptOptions( val compiledSources: List, val incrementalCache: File?, val classpathChanges: List, - val processIncrementally: Boolean, val sourcesOutputDir: File, val classesOutputDir: File, @@ -52,7 +51,6 @@ class KaptOptions( var classesOutputDir: File? = null var stubsOutputDir: File? = null var incrementalDataOutputDir: File? = null - var processIncrementally: Boolean = false val processingClasspath: MutableList = mutableListOf() val processors: MutableList = mutableListOf() @@ -75,7 +73,7 @@ class KaptOptions( return KaptOptions( projectBaseDir, compileClasspath, javaSourceRoots, - changedFiles, compiledSources, incrementalCache, classpathChanges, processIncrementally, + changedFiles, compiledSources, incrementalCache, classpathChanges, sourcesOutputDir, classesOutputDir, stubsOutputDir, incrementalDataOutputDir, processingClasspath, processors, processingOptions, javacOptions, KaptFlags.fromSet(flags), mode, detectMemoryLeaks @@ -106,7 +104,9 @@ enum class KaptFlag(val description: String) { CORRECT_ERROR_TYPES("Correct error types"), MAP_DIAGNOSTIC_LOCATIONS("Map diagnostic locations"), STRICT("Strict mode"), - INCLUDE_COMPILE_CLASSPATH("Detect annotation processors in compile classpath"); + INCLUDE_COMPILE_CLASSPATH("Detect annotation processors in compile classpath"), + INCREMENTAL_APT("Incremental annotation processing (apt mode)"), + ; } interface KaptSelector { @@ -172,5 +172,4 @@ fun KaptOptions.logString(additionalInfo: String = "") = buildString { appendln("[incremental apt] Compiled sources directories: ${compiledSources.joinToString()}") appendln("[incremental apt] Cache directory for incremental compilation: $incrementalCache") appendln("[incremental apt] Changed classpath names: ${classpathChanges.joinToString()}") - appendln("[incremental apt] If processing incrementally: $processIncrementally") } \ No newline at end of file diff --git a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/classStructureCache.kt b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/classStructureCache.kt index 434e599e2a3..0e77928b6d8 100644 --- a/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/classStructureCache.kt +++ b/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/classStructureCache.kt @@ -211,9 +211,7 @@ class SourceFileStructure( fun addMentionedConstant(containingClass: String, name: String) { if (!declaredTypes.contains(containingClass)) { - val names = mentionedConstants[containingClass] ?: HashSet() - names.add(name) - mentionedConstants[containingClass] = names + mentionedConstants.getOrPut(containingClass) { HashSet() }.add(name) } } } diff --git a/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/IncrementalKaptTest.kt b/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/IncrementalKaptTest.kt index 106d6430d49..3593c863065 100644 --- a/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/IncrementalKaptTest.kt +++ b/plugins/kapt3/kapt3-base/test/org/jetbrains/kotlin/kapt3/base/incremental/IncrementalKaptTest.kt @@ -5,6 +5,7 @@ package org.jetbrains.kotlin.kapt.base.test.org.jetbrains.kotlin.kapt3.base.incremental +import org.jetbrains.kotlin.base.kapt3.KaptFlag import org.jetbrains.kotlin.base.kapt3.KaptOptions import org.jetbrains.kotlin.base.kapt3.collectJavaSourceFiles import org.jetbrains.kotlin.kapt3.base.KaptContext @@ -70,7 +71,7 @@ class IncrementalKaptTest { incrementalCache = incrementalCacheDir compiledSources.add(classesOutput) changedFiles.add(sourcesDir.resolve("User.java")) - processIncrementally = true + flags.add(KaptFlag.INCREMENTAL_APT) }.build() KaptContext(optionsForSecondRun, true, logger).use { @@ -144,7 +145,7 @@ class IncrementalKaptTest { incrementalCache = incrementalCacheDir compiledSources.add(classesOutput) changedFiles.add(sourcesDir.resolve("User.java")) - processIncrementally = true + flags.add(KaptFlag.INCREMENTAL_APT) }.build() KaptContext(optionsForSecondRun, true, logger).use { diff --git a/plugins/kapt3/kapt3-compiler/src/org/jetbrains/kotlin/kapt3/Kapt3Plugin.kt b/plugins/kapt3/kapt3-compiler/src/org/jetbrains/kotlin/kapt3/Kapt3Plugin.kt index e9c3f9e5cad..7bd9d1151c6 100644 --- a/plugins/kapt3/kapt3-compiler/src/org/jetbrains/kotlin/kapt3/Kapt3Plugin.kt +++ b/plugins/kapt3/kapt3-compiler/src/org/jetbrains/kotlin/kapt3/Kapt3Plugin.kt @@ -101,7 +101,7 @@ class Kapt3CommandLineProcessor : CommandLineProcessor { COMPILED_SOURCES_DIR -> compiledSources.addAll(value.split(File.pathSeparator).map { File(it) }) INCREMENTAL_CACHE -> incrementalCache = File(value) CLASSPATH_CHANGES -> classpathChanges.addAll(value.split(File.pathSeparator).map { it }) - PROCESS_INCREMENTALLY -> processIncrementally = value.toBoolean() + PROCESS_INCREMENTALLY -> setFlag(KaptFlag.INCREMENTAL_APT, value) ANNOTATION_PROCESSOR_CLASSPATH_OPTION -> processingClasspath += File(value) ANNOTATION_PROCESSORS_OPTION -> processors.addAll(value.split(',').map { it.trim() }.filter { it.isNotEmpty() })