From 0d04b8783c38ad207f612c9e1eb85b7b6a144c8b Mon Sep 17 00:00:00 2001 From: Hung Nguyen Date: Tue, 19 Sep 2023 18:08:02 +0100 Subject: [PATCH] [IC] Handle custom source set located outside project directory To support build cache relocatability, we need to convert absolute paths into relative paths before storing them in IC caches. Before this commit, we computed relative paths based on the (root) project directory and FAIL if some source files are located outside the project directory. With this commit, we will NOT FAIL in that case. This means relative paths may start with "../". It's not "clean", but it can work. Test: New test in BuildCacheRelocationIT ^KT-61852 Fixed --- .../storage/RelocatableFileToPathConverter.kt | 7 +- .../RelocatableFileToPathConverterTest.kt | 39 +++--- .../storage/SourceToOutputFilesMapTest.kt | 4 +- .../jetbrains/kotlin/gradle/BaseGradleIT.kt | 4 +- .../kotlin/gradle/BuildCacheRelocationIT.kt | 127 ++++++++---------- 5 files changed, 85 insertions(+), 96 deletions(-) diff --git a/build-common/src/org/jetbrains/kotlin/incremental/storage/RelocatableFileToPathConverter.kt b/build-common/src/org/jetbrains/kotlin/incremental/storage/RelocatableFileToPathConverter.kt index 12df621583f..80534caca45 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/storage/RelocatableFileToPathConverter.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/storage/RelocatableFileToPathConverter.kt @@ -16,12 +16,9 @@ import java.io.File */ class RelocatableFileToPathConverter(private val baseDir: File) : FileToPathConverter { - private val unixStyleBaseDirPathPrefix = "${baseDir.invariantSeparatorsPath}/" - override fun toPath(file: File): String { - check(file.invariantSeparatorsPath.startsWith(unixStyleBaseDirPathPrefix)) { - "The given file '${file.path}' is located outside the base directory '${baseDir.path}'" - } + // Note: If the given file is located outside `baseDir`, the relative path will start with "../". It's not "clean", but it can work. + // TODO: Re-design the code such that `baseDir` always contains the given file (also add a precondition check here). return file.relativeTo(baseDir).invariantSeparatorsPath } diff --git a/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableFileToPathConverterTest.kt b/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableFileToPathConverterTest.kt index 461d28effa8..70c66813912 100644 --- a/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableFileToPathConverterTest.kt +++ b/build-common/test/org/jetbrains/kotlin/incremental/storage/RelocatableFileToPathConverterTest.kt @@ -5,34 +5,43 @@ package org.jetbrains.kotlin.incremental.storage -import org.jetbrains.kotlin.TestWithWorkingDir +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Rule import org.junit.Test -import kotlin.test.assertFailsWith +import org.junit.rules.TemporaryFolder +import java.io.File -class RelocatableFileToPathConverterTest : TestWithWorkingDir() { +class RelocatableFileToPathConverterTest { + @get:Rule + val tmpDir = TemporaryFolder() + + private lateinit var baseDir: File private lateinit var pathConverter: RelocatableFileToPathConverter - override fun setUp() { - super.setUp() - pathConverter = RelocatableFileToPathConverter(workingDir) + @Before + fun setUp() { + baseDir = tmpDir.newFolder("baseDir") + pathConverter = RelocatableFileToPathConverter(baseDir) } @Test fun testToPath() { - assertEquals("com/example/Foo.kt", pathConverter.toPath(workingDir.resolve("com/example/Foo.kt"))) - } - - @Test - fun testToPathFails() { - assertFailsWith(IllegalStateException::class) { - pathConverter.toPath(workingDir.resolve("../outsideWorkingDir/com/example/Foo.kt").normalize()) - } + assertEquals("com/example/Foo.kt", pathConverter.toPath(baseDir.resolve("com/example/Foo.kt"))) } @Test fun testToFile() { - assertEquals(workingDir.resolve("com/example/Foo.kt"), pathConverter.toFile("com/example/Foo.kt")) + assertEquals(baseDir.resolve("com/example/Foo.kt"), pathConverter.toFile("com/example/Foo.kt")) + } + + @Test + fun testFileOutsideBaseDirectory() { + val fileOutsideBaseDir = baseDir.resolve("../outsideBaseDir/com/example/Foo.kt").normalize() + + assertEquals("../outsideBaseDir/com/example/Foo.kt", pathConverter.toPath(fileOutsideBaseDir)) + assertEquals(fileOutsideBaseDir, pathConverter.toFile("../outsideBaseDir/com/example/Foo.kt")) } } \ No newline at end of file diff --git a/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/storage/SourceToOutputFilesMapTest.kt b/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/storage/SourceToOutputFilesMapTest.kt index ba25f45f97e..dee416ad8b8 100644 --- a/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/storage/SourceToOutputFilesMapTest.kt +++ b/compiler/incremental-compilation-impl/test/org/jetbrains/kotlin/incremental/storage/SourceToOutputFilesMapTest.kt @@ -84,7 +84,7 @@ class SourceToOutputFilesMapTest { @Test fun testSetRelativeFails() { - assertFailsWith { + assertFailsWith { stofMap[fooDotKt] = listOf(File("relativePath")) } } @@ -93,7 +93,7 @@ class SourceToOutputFilesMapTest { fun testGetRelativeFails() { stofMap[fooDotKt] = listOf(fooDotClass) - assertFailsWith { + assertFailsWith { stofMap[fooDotKt.relativeTo(srcDir)] } } diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/BaseGradleIT.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/BaseGradleIT.kt index 4e14c5eabc9..2a447f0038b 100644 --- a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/BaseGradleIT.kt +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/BaseGradleIT.kt @@ -269,7 +269,7 @@ abstract class BaseGradleIT { val hierarchicalMPPStructureSupport: Boolean? = null, val withReports: List = emptyList(), val enableKpmModelMapping: Boolean? = null, - val useDaemonFallbackStrategy: Boolean = false, + val useDaemonFallbackStrategy: Boolean? = null, val useParsableDiagnosticsFormatting: Boolean = true, val showDiagnosticsStacktrace: Boolean? = false, // false by default to not clutter the testdata + stacktraces change often val stacktraceMode: String? = StacktraceOption.FULL_STACKTRACE_LONG_OPTION, @@ -916,7 +916,7 @@ abstract class BaseGradleIT { add("-Pkotlin.build.report.output=${options.withReports.joinToString { it.name }}") } - add("-Pkotlin.daemon.useFallbackStrategy=${options.useDaemonFallbackStrategy}") + options.useDaemonFallbackStrategy?.let { add("-Pkotlin.daemon.useFallbackStrategy=$it") } add("-Dorg.gradle.unsafe.configuration-cache=${options.configurationCache}") add("-Dorg.gradle.unsafe.configuration-cache-problems=${options.configurationCacheProblems.name.lowercase(Locale.getDefault())}") diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/BuildCacheRelocationIT.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/BuildCacheRelocationIT.kt index 28a45c5530b..5ce44658d15 100644 --- a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/BuildCacheRelocationIT.kt +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/BuildCacheRelocationIT.kt @@ -22,10 +22,7 @@ import org.gradle.util.GradleVersion import org.jetbrains.kotlin.gradle.testbase.* import org.junit.jupiter.api.DisplayName import java.io.File -import kotlin.io.path.createDirectory -import kotlin.io.path.isRegularFile -import kotlin.io.path.readText -import kotlin.io.path.walk +import kotlin.io.path.* @DisplayName("Build cache relocation") class BuildCacheRelocationIT : KGPBaseTest() { @@ -312,63 +309,6 @@ class BuildCacheRelocationIT : KGPBaseTest() { } } - @JvmGradlePluginTests - @DisplayName("Kotlin incremental compilation should work correctly after cache hint") - @GradleTest - fun testKotlinIncrementalCompilation(gradleVersion: GradleVersion) { - val options = defaultBuildOptions.copy(useGradleClasspathSnapshot = false) - checkKotlinIncrementalCompilationAfterCacheHit(gradleVersion, options) { - assertNonIncrementalCompilation() - } - } - - @JvmGradlePluginTests - @DisplayName("Kotlin incremental compilation with `kotlin.incremental.useClasspathSnapshot` feature should work correctly") - @GradleTest - fun testKotlinIncrementalCompilation_withGradleClasspathSnapshot(gradleVersion: GradleVersion) { - checkKotlinIncrementalCompilationAfterCacheHit(gradleVersion, defaultBuildOptions.copy(useGradleClasspathSnapshot = true)) { - assertIncrementalCompilation(listOf("src/main/kotlin/foo.kt", "src/main/kotlin/fooUsage.kt").toPaths()) - } - } - - @JvmGradlePluginTests - @DisplayName("Kotlin incremental compilation with `kotlin.incremental.classpath.snapshot.enabled` feature should work correctly") - @GradleTest - fun testKotlinIncrementalCompilation_withICClasspathSnapshot(gradleVersion: GradleVersion) { - checkKotlinIncrementalCompilationAfterCacheHit(gradleVersion, defaultBuildOptions.copy(useICClasspathSnapshot = true)) { - assertIncrementalCompilation(listOf("src/main/kotlin/foo.kt", "src/main/kotlin/fooUsage.kt").toPaths()) - assertOutputContains("Incremental compilation with ABI snapshot enabled") - } - } - - private fun checkKotlinIncrementalCompilationAfterCacheHit( - gradleVersion: GradleVersion, - buildOptions: BuildOptions = defaultBuildOptions, - assertions: BuildResult.() -> Unit - ) { - val (firstProject, secondProject) = prepareTestProjects("buildCacheSimple", gradleVersion, buildOptions) - - // First build, should be stored into the build cache: - firstProject.build("assemble") { - assertTasksPackedToCache(":compileKotlin") - } - - // A cache hit: a clean build without any changes to the project - secondProject.build("clean", "assemble") { - assertTasksFromCache(":compileKotlin") - } - - // Check whether compilation after a cache hit is incremental (KT-34862) - val fooKtSourceFile = secondProject.kotlinSourcesDir().resolve("foo.kt") - fooKtSourceFile.modify { it.replace("Int = 1", "String = \"abc\"") } - secondProject.build("assemble", buildOptions = buildOptions.copy(logLevel = LogLevel.DEBUG), assertions = assertions) - // Revert the change to the return type of foo(), and check if we get a cache hit - fooKtSourceFile.modify { it.replace("String = \"abc\"", "Int = 1") } - secondProject.build("clean", "assemble") { - assertTasksFromCache(":compileKotlin") - } - } - private fun checkKaptCachingIncrementalBuild( firstProject: TestProject, secondProject: TestProject @@ -410,27 +350,70 @@ class BuildCacheRelocationIT : KGPBaseTest() { } @JvmGradlePluginTests - @DisplayName("test relocatability for projects using custom build directory") // Regression test for KT-58547 + @DisplayName("Kotlin incremental compilation should work correctly after cache hint") @GradleTest - fun testCustomBuildDirectory(gradleVersion: GradleVersion) { - val (firstProject, secondProject) = prepareTestProjects("buildCacheSimple", gradleVersion) - firstProject.buildGradle.append("buildDir = \"../BUILD_DIR_1\"") - secondProject.buildGradle.append("buildDir = \"../BUILD_DIR_2\"") + fun testKotlinIncrementalCompilationAfterCacheHit(gradleVersion: GradleVersion) { + checkKotlinIncrementalCompilationAfterCacheHit(gradleVersion) + } - firstProject.build(":compileKotlin") + @JvmGradlePluginTests + @DisplayName("test custom source set and build directory located outside project directory") // Regression test for KT-61852 and KT-58547 + @GradleTest + fun testCustomSourceSetAndBuildDirectory(gradleVersion: GradleVersion) { + val projects = mutableListOf() + checkKotlinIncrementalCompilationAfterCacheHit( + gradleVersion, + buildOptions = defaultBuildOptions.copy(useDaemonFallbackStrategy = false) + ) { project -> + project.projectPath.toFile().resolve("../SOURCE_SET_OUTSIDE_PROJECT/src/main/kotlin").let { + it.mkdirs() + it.resolve("SomeClass.kt").createNewFile() + } + project.buildGradle.append("sourceSets { main.java.srcDirs += \"../SOURCE_SET_OUTSIDE_PROJECT/src/main/kotlin\" }") + project.buildGradle.append("buildDir = \"../BUILD_DIR_OUTSIDE_PROJECT\"") + + projects.add(project) + } + + // Also check that output files do not contain non-relocatable paths + val projectPath = projects.first().projectPath val outputFilesContainingNonRelocatablePaths = - firstProject.projectPath.resolve("../BUILD_DIR_1/kotlin/compileKotlin").walk() - .filter { - // Use readText() even for binary files as we don't have a better way for now - it.isRegularFile() && it.readText().contains("BUILD_DIR_1") - }.toList() + projectPath.resolve("../BUILD_DIR_OUTSIDE_PROJECT/kotlin/compileKotlin").walk().filter { + // Use readText() even for binary files as we don't have a better way for now + it.isRegularFile() && it.readText().let { text -> + text.contains(projectPath.parent.name) || text.contains("BUILD_DIR_OUTSIDE_PROJECT") + } + }.toList() assert(outputFilesContainingNonRelocatablePaths.isEmpty()) { "The following output files contain non-relocatable paths:\n" + outputFilesContainingNonRelocatablePaths.joinToString("\n") } + } + private fun checkKotlinIncrementalCompilationAfterCacheHit( + gradleVersion: GradleVersion, + buildOptions: BuildOptions = defaultBuildOptions, + configureProject: (TestProject) -> Unit = {}, + ) { + val (firstProject, secondProject) = + prepareTestProjects("buildCacheSimple", gradleVersion, buildOptions, buildJdk = null, configureProject) + + // Build the first project -- It should be a cache miss + firstProject.build(":compileKotlin") { + assertTasksPackedToCache(":compileKotlin") + } + + // Build the second project -- It should be a cache hit secondProject.build(":compileKotlin") { assertTasksFromCache(":compileKotlin") } + + // Make a change to the second project and build again -- Compilation should be incremental + secondProject.kotlinSourcesDir().resolve("foo.kt").modify { + it.replace("Int = 1", "String = \"abc\"") + } + secondProject.build("assemble", buildOptions = buildOptions.copy(logLevel = LogLevel.DEBUG)) { + assertIncrementalCompilation(listOf("src/main/kotlin/foo.kt", "src/main/kotlin/fooUsage.kt").toPaths()) + } } }