From d89407b6275f6b6c73e16f6dce0681fefd51c531 Mon Sep 17 00:00:00 2001 From: Anthony Restaino Date: Fri, 23 Aug 2024 15:17:00 -0400 Subject: [PATCH] Support usage in projects using Gradle 8.10 by avoiding using Project.equals (#257) * Replace usage of Set with Map to avoid Project inequivalence in newer Gradle versions * Fix compilation error in sample project --- .../AffectedModuleDetector.kt | 32 +-- .../AffectedModuleDetectorPlugin.kt | 2 +- .../DependencyTracker.kt | 13 +- .../AffectedModuleDetectorImplTest.kt | 201 ++++++++++++++---- .../sample/tasks/AffectedTasksPlugin.kt | 2 +- 5 files changed, 183 insertions(+), 67 deletions(-) diff --git a/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetector.kt b/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetector.kt index 05237ed6..6fb67195 100644 --- a/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetector.kt +++ b/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetector.kt @@ -56,6 +56,12 @@ import java.io.File */ enum class ProjectSubset { DEPENDENT_PROJECTS, CHANGED_PROJECTS, ALL_AFFECTED_PROJECTS, NONE } +/** + * An identifier for a project, ensuring that projects are always identified by their path. + */ +@JvmInline +value class ProjectPath(val path: String) + /** * A utility class that can discover which files are changed based on git history. * @@ -344,7 +350,7 @@ class AffectedModuleDetectorImpl constructor( } private val allProjects by lazy { - rootProject.subprojects.toSet() + rootProject.subprojects.associateBy { it.projectPath } } private val projectGraph by lazy { @@ -369,7 +375,7 @@ class AffectedModuleDetectorImpl constructor( override fun shouldInclude(project: Project): Boolean { val isRootProject = project.isRoot - val isProjectAffected = affectedProjects.contains(project) + val isProjectAffected = affectedProjects.contains(project.projectPath) val isProjectProvided = isProjectProvided2(project) val isModuleExcludedByName = config.excludedModules.contains(project.name) val isModuleExcludedByRegex = config.excludedModules.any { project.path.matches(it.toRegex()) } @@ -390,10 +396,10 @@ class AffectedModuleDetectorImpl constructor( override fun getSubset(project: Project): ProjectSubset { return when { - changedProjects.contains(project) -> { + changedProjects.contains(project.projectPath) -> { ProjectSubset.CHANGED_PROJECTS } - dependentProjects.contains(project) -> { + dependentProjects.contains(project.projectPath) -> { ProjectSubset.DEPENDENT_PROJECTS } else -> { @@ -410,7 +416,7 @@ class AffectedModuleDetectorImpl constructor( private fun findChangedProjects( top: Sha, includeUncommitted: Boolean = true - ): Set { + ): Map { git.findChangedFiles( top = top, includeUncommitted = includeUncommitted @@ -421,7 +427,7 @@ class AffectedModuleDetectorImpl constructor( changedFiles.add(fileName) } - val changedProjects = mutableSetOf() + val changedProjects = mutableMapOf() for (filePath in changedFiles) { val containingProject = findContainingProject(filePath) @@ -432,7 +438,7 @@ class AffectedModuleDetectorImpl constructor( "Adding to unknownFiles." ) } else { - changedProjects.add(containingProject) + changedProjects[containingProject.projectPath] = containingProject logger?.info( "For file $filePath containing project is $containingProject. " + "Adding to changedProjects." @@ -447,10 +453,10 @@ class AffectedModuleDetectorImpl constructor( * Gets all dependent projects from the set of changedProjects. This doesn't include the * original changedProjects. Always build is still here to ensure at least 1 thing is built */ - private fun findDependentProjects(): Set { - return changedProjects.flatMap { - dependencyTracker.findAllDependents(it) - }.toSet() + private fun findDependentProjects(): Map { + return changedProjects.flatMap { (_, project) -> + dependencyTracker.findAllDependents(project).entries + }.associate { it.key to it.value } } /** @@ -466,7 +472,7 @@ class AffectedModuleDetectorImpl constructor( * Also detects modules whose tests are codependent at runtime. */ @Suppress("ComplexMethod") - private fun findAffectedProjects(): Set { + private fun findAffectedProjects(): Map { // In this case we don't care about any of the logic below, we're only concerned with // running the changed projects in this test runner if (projectSubset == ProjectSubset.CHANGED_PROJECTS) { @@ -525,3 +531,5 @@ class AffectedModuleDetectorImpl constructor( } val Project.isRoot get() = this == rootProject + +val Project.projectPath: ProjectPath get() = ProjectPath(path) diff --git a/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorPlugin.kt b/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorPlugin.kt index cc6960b3..27258aa1 100644 --- a/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorPlugin.kt +++ b/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorPlugin.kt @@ -229,7 +229,7 @@ class AffectedModuleDetectorPlugin : Plugin { val tracker = DependencyTracker(project, null) project.tasks.configureEach { task -> if (task.name.contains(ANDROID_TEST_PATTERN)) { - tracker.findAllDependents(project).forEach { dependentProject -> + tracker.findAllDependents(project).forEach { (_, dependentProject) -> dependentProject.tasks.forEach { dependentTask -> AffectedModuleDetector.configureTaskGuard(dependentTask) } diff --git a/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/DependencyTracker.kt b/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/DependencyTracker.kt index 3d235cfd..1a1c3431 100644 --- a/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/DependencyTracker.kt +++ b/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/DependencyTracker.kt @@ -54,21 +54,22 @@ class DependencyTracker constructor( result } - fun findAllDependents(project: Project): Set { + fun findAllDependents(project: Project): Map { logger?.info("finding dependents of ${project.path}") - val result = mutableSetOf() + val result = mutableMapOf() fun addAllDependents(project: Project) { - if (result.add(project)) { + if (result.put(project.projectPath, project) == null) { dependentList[project]?.forEach(::addAllDependents) } } addAllDependents(project) logger?.info( - "dependents of ${project.path} is ${result.map { - it.path + "dependents of ${project.path} is ${result.map { (path, _) -> + path.path }}" ) // the project isn't a dependent of itself - return result.minus(project) + result.remove(project.projectPath) + return result } } diff --git a/affectedmoduledetector/src/test/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorImplTest.kt b/affectedmoduledetector/src/test/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorImplTest.kt index ee272554..d6793bc1 100644 --- a/affectedmoduledetector/src/test/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorImplTest.kt +++ b/affectedmoduledetector/src/test/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorImplTest.kt @@ -242,7 +242,18 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p1, p2, p3, p4, p5, p6, p7, p8, p9, p10) + mapOf( + p1.projectPath to p1, + p2.projectPath to p2, + p3.projectPath to p3, + p4.projectPath to p4, + p5.projectPath to p5, + p6.projectPath to p6, + p7.projectPath to p7, + p8.projectPath to p8, + p9.projectPath to p9, + p10.projectPath to p10 + ) ) ) } @@ -263,7 +274,18 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p1, p2, p3, p4, p5, p6, p7, p8, p9, p10) + mapOf( + p1.projectPath to p1, + p2.projectPath to p2, + p3.projectPath to p3, + p4.projectPath to p4, + p5.projectPath to p5, + p6.projectPath to p6, + p7.projectPath to p7, + p8.projectPath to p8, + p9.projectPath to p9, + p10.projectPath to p10 + ) ) ) } @@ -284,7 +306,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() + mapOf() ) ) } @@ -305,7 +327,12 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p1, p3, p4, p5) + mapOf( + p1.projectPath to p1, + p3.projectPath to p3, + p4.projectPath to p4, + p5.projectPath to p5 + ) ) ) } @@ -326,7 +353,11 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p3, p4, p5) + mapOf( + p3.projectPath to p3, + p4.projectPath to p4, + p5.projectPath to p5 + ) ) ) } @@ -347,7 +378,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p1) + mapOf(p1.projectPath to p1) ) ) } @@ -371,7 +402,14 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p1, p2, p3, p4, p5, p6) + mapOf( + p1.projectPath to p1, + p2.projectPath to p2, + p3.projectPath to p3, + p4.projectPath to p4, + p5.projectPath to p5, + p6.projectPath to p6 + ) ) ) } @@ -395,7 +433,12 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p3, p4, p5, p6) + mapOf( + p3.projectPath to p3, + p4.projectPath to p4, + p5.projectPath to p5, + p6.projectPath to p6 + ) ) ) } @@ -419,7 +462,10 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p1, p2) + mapOf( + p1.projectPath to p1, + p2.projectPath to p2 + ) ) ) } @@ -440,7 +486,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() + mapOf() ) ) } @@ -461,7 +507,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() + mapOf() ) ) } @@ -482,7 +528,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p7) + mapOf(p7.projectPath to p7) ) ) } @@ -507,7 +553,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p12) + mapOf(p12.projectPath to p12) ) ) } @@ -532,7 +578,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p12) + mapOf(p12.projectPath to p12) ) ) } @@ -557,7 +603,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() + mapOf() ) ) } @@ -582,7 +628,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() + mapOf() ) ) } @@ -607,7 +653,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() + mapOf() ) ) } @@ -632,7 +678,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() + mapOf() ) ) } @@ -657,7 +703,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p8) + mapOf(p8.projectPath to p8) ) ) } @@ -682,7 +728,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p8) + mapOf(p8.projectPath to p8) ) ) } @@ -707,7 +753,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() + mapOf() ) ) } @@ -732,7 +778,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() + mapOf() ) ) } @@ -756,7 +802,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p12) + mapOf(p12.projectPath to p12) ) ) } @@ -780,7 +826,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p12) + mapOf(p12.projectPath to p12) ) ) } @@ -804,7 +850,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() // a change to a project in the normal build doesn't affect the ui build + mapOf() // a change to a project in the normal build doesn't affect the ui build ) ) // and compose is in changed and so excluded from dependent } @@ -828,7 +874,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p7) // a change in compose is known not to matter to the normal build + mapOf(p7.projectPath to p7) // a change in compose is known not to matter to the normal build ) ) } @@ -852,7 +898,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p7) + mapOf(p7.projectPath to p7) ) ) } @@ -876,7 +922,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() // a change in compose is known not to matter to the normal build + mapOf() // a change in compose is known not to matter to the normal build ) ) // and p7 is in changed and so not in dependent } @@ -899,7 +945,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() // a change in androidx root normally doesn't affect the ui build + mapOf() // a change in androidx root normally doesn't affect the ui build ) ) // unless otherwise specified (e.g. gradlew) } @@ -922,7 +968,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - emptySet() + emptyMap() ) ) // a change in ui/root affects all ui projects } @@ -945,7 +991,18 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p1, p2, p3, p4, p5, p6, p7, p8, p9, p10) + mapOf( + p1.projectPath to p1, + p2.projectPath to p2, + p3.projectPath to p3, + p4.projectPath to p4, + p5.projectPath to p5, + p6.projectPath to p6, + p7.projectPath to p7, + p8.projectPath to p8, + p9.projectPath to p9, + p10.projectPath to p10 + ) ) ) // a change to buildSrc affects everything in both builds } @@ -969,7 +1026,10 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p12, p13) // a change to buildSrc affects everything in both builds + mapOf( + p12.projectPath to p12, + p13.projectPath to p13 + ) // a change to buildSrc affects everything in both builds ) ) } @@ -992,7 +1052,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() // a change to ui gradlew affects only the ui build + mapOf() // a change to ui gradlew affects only the ui build ) ) } @@ -1015,7 +1075,10 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p12, p13) // a change to root gradlew affects everything in both builds + mapOf( + p12.projectPath to p12, + p13.projectPath to p13 + ) // a change to root gradlew affects everything in both builds ) ) } @@ -1038,7 +1101,10 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p12, p13) // a change to development affects everything in both builds + mapOf( + p12.projectPath to p12, + p13.projectPath to p13 + ) // a change to development affects everything in both builds ) ) } @@ -1066,7 +1132,10 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p12, p13) // not sure what this folder is for, but it affects all of both? + mapOf( + p12.projectPath to p12, + p13.projectPath to p13 + ) // not sure what this folder is for, but it affects all of both? ) ) } @@ -1088,7 +1157,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p1) + mapOf(p1.projectPath to p1) ) ) // Test changed @@ -1131,7 +1200,11 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p3, p4, p5) + mapOf( + p3.projectPath to p3, + p4.projectPath to p4, + p5.projectPath to p5 + ) ) ) // Test changed @@ -1174,7 +1247,12 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p1, p3, p4, p5) + mapOf( + p1.projectPath to p1, + p3.projectPath to p3, + p4.projectPath to p4, + p5.projectPath to p5 + ) ) ) // Test changed @@ -1216,7 +1294,12 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p16, p17, p18, p19) + mapOf( + p16.projectPath to p16, + p17.projectPath to p17, + p18.projectPath to p18, + p19.projectPath to p19 + ) ) ) } @@ -1237,7 +1320,12 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p16, p17, p18, p19) + mapOf( + p16.projectPath to p16, + p17.projectPath to p17, + p18.projectPath to p18, + p19.projectPath to p19 + ) ) ) } @@ -1258,7 +1346,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - emptySet() + mapOf() ) ) } @@ -1279,7 +1367,12 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p16, p17, p18, p19) + mapOf( + p16.projectPath to p16, + p17.projectPath to p17, + p18.projectPath to p18, + p19.projectPath to p19 + ) ) ) } @@ -1300,7 +1393,10 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p18, p19) + mapOf( + p18.projectPath to p18, + p19.projectPath to p19 + ) ) ) } @@ -1321,7 +1417,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p19) + mapOf(p19.projectPath to p19) ) ) } @@ -1493,7 +1589,18 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf(p1, p2, p3, p4, p5, p6, p7, p8, p9, p10) + mapOf( + p1.projectPath to p1, + p2.projectPath to p2, + p3.projectPath to p3, + p4.projectPath to p4, + p5.projectPath to p5, + p6.projectPath to p6, + p7.projectPath to p7, + p8.projectPath to p8, + p9.projectPath to p9, + p10.projectPath to p10 + ) ) ) } @@ -1517,7 +1624,7 @@ class AffectedModuleDetectorImplTest { MatcherAssert.assertThat( detector.affectedProjects, CoreMatchers.`is`( - setOf() + mapOf() ) ) } diff --git a/sample/buildSrc/src/main/kotlin/com/dropbox/sample/tasks/AffectedTasksPlugin.kt b/sample/buildSrc/src/main/kotlin/com/dropbox/sample/tasks/AffectedTasksPlugin.kt index 5af78ae5..6fd05c93 100644 --- a/sample/buildSrc/src/main/kotlin/com/dropbox/sample/tasks/AffectedTasksPlugin.kt +++ b/sample/buildSrc/src/main/kotlin/com/dropbox/sample/tasks/AffectedTasksPlugin.kt @@ -110,7 +110,7 @@ class AffectedTasksPlugin : Plugin { val tracker = DependencyTracker(project, null) project.tasks.configureEach { task -> if (task.name.contains(ANDROID_TEST_BUILD_VARIANT)) { - tracker.findAllDependents(project).forEach { dependentProject -> + tracker.findAllDependents(project).forEach { (_, dependentProject) -> dependentProject.tasks.forEach { dependentTask -> AffectedModuleDetector.configureTaskGuard(dependentTask) }