From 7e983ed529c12a7c9adb480c036ddb8ec2a8da60 Mon Sep 17 00:00:00 2001 From: Cedric Beust Date: Fri, 10 Jun 2016 21:52:21 -0800 Subject: [PATCH] GITHUB-231: Fix the incorrect order of builds. Fixes https://github.com/cbeust/kobalt/issues/231 --- .../com/beust/kobalt/internal/TaskManager.kt | 63 +++++++++-------- .../com/beust/kobalt/misc/Topological.kt | 13 ++-- .../com/beust/kobalt/app/BuildScriptUtil.kt | 5 +- .../beust/kobalt/internal/BuildOrderTest.kt | 68 ++++++++++++++++++- .../beust/kobalt/internal/DynamicGraphTest.kt | 6 +- 5 files changed, 113 insertions(+), 42 deletions(-) diff --git a/modules/kobalt-plugin-api/src/main/kotlin/com/beust/kobalt/internal/TaskManager.kt b/modules/kobalt-plugin-api/src/main/kotlin/com/beust/kobalt/internal/TaskManager.kt index 481d43d9..f4708c79 100644 --- a/modules/kobalt-plugin-api/src/main/kotlin/com/beust/kobalt/internal/TaskManager.kt +++ b/modules/kobalt-plugin-api/src/main/kotlin/com/beust/kobalt/internal/TaskManager.kt @@ -4,10 +4,7 @@ import com.beust.kobalt.* import com.beust.kobalt.api.* import com.beust.kobalt.api.annotation.IncrementalTask import com.beust.kobalt.api.annotation.Task -import com.beust.kobalt.misc.Strings -import com.beust.kobalt.misc.benchmarkMillis -import com.beust.kobalt.misc.kobaltError -import com.beust.kobalt.misc.log +import com.beust.kobalt.misc.* import com.google.common.annotations.VisibleForTesting import com.google.common.collect.ArrayListMultimap import com.google.common.collect.ListMultimap @@ -182,34 +179,40 @@ class TaskManager @Inject constructor(val args: Args, */ fun calculateDependentTaskNames(taskNames: List, projects: List): List { val projectMap = hashMapOf().apply { - projects.forEach { put(it.name, it)} - } - val result = ArrayList(taskNames.map { TaskInfo(it) }) - val toProcess = ArrayList(result) - val newToProcess = arrayListOf() - val seen = hashSetOf() - var stop = false - while (! stop) { - toProcess.forEach { ti -> - projectMap[ti.project]?.let { project -> - project.projectExtra.dependsOn.forEach { dp -> - val newTask = TaskInfo(dp.projectName, ti.taskName) - // Insert the project at the top of the list since we haven't added its dependents yet - result.add(0, newTask) - if (! seen.contains(newTask)) { - newToProcess.add(newTask) - seen.add(newTask) - } - } - } - } - stop = newToProcess.isEmpty() - toProcess.clear() - toProcess.addAll(newToProcess) - newToProcess.clear() + projects.forEach { project -> put(project.name, project)} } - return result + val allTaskInfos = HashSet(taskNames.map { TaskInfo(it) }) + with(Topological()) { + val toProcess = ArrayList(allTaskInfos) + val seen = HashSet(allTaskInfos) + val newTasks = hashSetOf() + while (toProcess.any()) { + toProcess.forEach { ti -> + val project = projectMap[ti.project] + val dependents = project!!.projectExtra.dependsOn + if (dependents.any()) { + dependents.forEach { depProject -> + val tiDep = TaskInfo(depProject.name, ti.taskName) + allTaskInfos.add(tiDep) + addEdge(ti, tiDep) + if (! seen.contains(tiDep)) { + newTasks.add(tiDep) + seen.add(tiDep) + } + } + } else { + allTaskInfos.add(ti) + addNode(ti) + } + } + toProcess.clear() + toProcess.addAll(newTasks) + newTasks.clear() + } + val result = sort() + return result + } } val LOG_LEVEL = 3 diff --git a/modules/kobalt-plugin-api/src/main/kotlin/com/beust/kobalt/misc/Topological.kt b/modules/kobalt-plugin-api/src/main/kotlin/com/beust/kobalt/misc/Topological.kt index b0a92315..38c4c503 100644 --- a/modules/kobalt-plugin-api/src/main/kotlin/com/beust/kobalt/misc/Topological.kt +++ b/modules/kobalt-plugin-api/src/main/kotlin/com/beust/kobalt/misc/Topological.kt @@ -10,22 +10,25 @@ import java.util.* */ class Topological { private val dependingOn = ArrayListMultimap.create() + private val nodes = hashSetOf() + + fun addNode(t: T) = nodes.add(t) fun addEdge(t: T, other: T) { + addNode(t) + addNode(other) dependingOn.put(t, other) } - fun addEdge(t: T, others: Array) { - dependingOn.putAll(t, others.toMutableList()) - } - /** * @return the Ts sorted topologically. */ - fun sort(all: ArrayList) : List { + fun sort() : List { + val all = ArrayList(nodes) val result = arrayListOf() var dependMap = HashMultimap.create() dependingOn.keySet().forEach { dependMap.putAll(it, dependingOn.get(it))} + nodes.forEach { dependMap.putAll(it, emptyList())} while (all.size > 0) { val freeNodes = all.filter { dependMap.get(it).isEmpty() diff --git a/src/main/kotlin/com/beust/kobalt/app/BuildScriptUtil.kt b/src/main/kotlin/com/beust/kobalt/app/BuildScriptUtil.kt index fbeaef40..a077a9e6 100644 --- a/src/main/kotlin/com/beust/kobalt/app/BuildScriptUtil.kt +++ b/src/main/kotlin/com/beust/kobalt/app/BuildScriptUtil.kt @@ -20,7 +20,6 @@ import java.io.InputStream import java.lang.reflect.Modifier import java.net.URL import java.net.URLClassLoader -import java.util.* import java.util.jar.JarInputStream class BuildScriptUtil @Inject constructor(val plugins: Plugins, val files: KFiles, @@ -113,14 +112,14 @@ class BuildScriptUtil @Inject constructor(val plugins: Plugins, val files: KFile context.pluginInfo.projectContributors.forEach { contributor -> val descriptions = contributor.projects() descriptions.forEach { pd -> - all.add(pd.project) + topologicalProjects.addNode(pd.project) pd.dependsOn.forEach { dependsOn -> topologicalProjects.addEdge(pd.project, dependsOn) all.add(dependsOn) } } } - val result = topologicalProjects.sort(ArrayList(all)) + val result = topologicalProjects.sort() return result } diff --git a/src/test/kotlin/com/beust/kobalt/internal/BuildOrderTest.kt b/src/test/kotlin/com/beust/kobalt/internal/BuildOrderTest.kt index dabdb163..70051e82 100644 --- a/src/test/kotlin/com/beust/kobalt/internal/BuildOrderTest.kt +++ b/src/test/kotlin/com/beust/kobalt/internal/BuildOrderTest.kt @@ -3,7 +3,7 @@ package com.beust.kobalt.internal import com.beust.kobalt.TestModule import com.beust.kobalt.api.Kobalt import com.beust.kobalt.project -import org.testng.Assert +import org.assertj.core.api.Assertions.assertThat import org.testng.annotations.BeforeClass import org.testng.annotations.DataProvider import org.testng.annotations.Guice @@ -35,7 +35,71 @@ class BuildOrderTest @Inject constructor(val taskManager: TaskManager) { val allProjects = listOf(p1, p2, p3) with(taskManager) { val taskInfos = calculateDependentTaskNames(tasks, allProjects) - Assert.assertEquals(taskInfos.map { it.id }, expectedTasks) + assertThat(taskInfos.map { it.id }).isEqualTo(expectedTasks) } } + + @DataProvider + fun tasks2(): Array> { + return arrayOf( + arrayOf(listOf("p14:assemble"), + listOf(1, 2, 3, 4, 5, 8, 11, 6, 7, 9, 10, 12, 13, 14).map { "p$it:assemble"}) + ) + } + + @Test(dataProvider = "tasks2") + fun shouldBuildInRightOrder2(tasks: List, expectedTasks: List) { + val p1 = project { name ="p1" } + val p2 = project { name ="p2" } + val p3 = project { name ="p3" } + val p4 = project { name ="p4" } + val p5 = project { name ="p5" } + val p6 = project(p5) { name ="p6" } + val p7 = project(p5) { name ="p7" } + val p8 = project { name ="p8" } + val p9 = project(p6, p5, p2, p3) { name ="p9" } + val p10 = project(p9) { name ="p10" } + val p11 = project { name ="p11" } + val p12 = project(p1, p7, p9, p10, p11) { name ="p12" } + val p13 = project(p4, p8, p9, p12) { name ="p13" } + val p14 = project(p12, p13) { name ="p14" } + + fun appearsBefore(taskInfos: List, first: String, second: String) { + var sawFirst = false + var sawSecond = false + taskInfos.forEach { ti -> + if (ti.project == first) { + sawFirst = true + } + if (ti.project == second) { + assertThat(sawFirst) + .`as`("Expected to see $first before $second in ${taskInfos.map {it.project}}") + .isTrue() + sawSecond = true + } + } + assertThat(sawFirst).`as`("Didn't see $first").isTrue() + assertThat(sawSecond).`as`("Didn't see $second").isTrue() + } + + fun appearsBefore(taskInfos: List, firsts: List, second: String) { + firsts.forEach { first -> + appearsBefore(taskInfos, first, second) + } + } + + // 1, 2, 3, 4, 5, 8, 11, 6, 7, 9, 10, 12, 13, 14 + val allProjects = listOf(p1, p2, p3, p4, p5, p6, p7, p8, p9, p10, p11, p12, p13, p14) + with(taskManager) { + val taskInfos = calculateDependentTaskNames(tasks, allProjects) + assertThat(taskInfos.size).isEqualTo(expectedTasks.size) + appearsBefore(taskInfos, "p5", "p6") + appearsBefore(taskInfos, "p5", "p7") + appearsBefore(taskInfos, "p9", "p10") + appearsBefore(taskInfos, listOf("p1", "p7", "p9", "p10", "p11"), "p12") + appearsBefore(taskInfos, listOf("p4", "p8", "p9", "p12"), "p13") + appearsBefore(taskInfos, listOf("p12", "p13"), "p14") + } + } + } diff --git a/src/test/kotlin/com/beust/kobalt/internal/DynamicGraphTest.kt b/src/test/kotlin/com/beust/kobalt/internal/DynamicGraphTest.kt index 07b2ffd2..6298c994 100644 --- a/src/test/kotlin/com/beust/kobalt/internal/DynamicGraphTest.kt +++ b/src/test/kotlin/com/beust/kobalt/internal/DynamicGraphTest.kt @@ -125,8 +125,10 @@ class DynamicGraphTest { addEdge("b2", "a2") addEdge("c1", "b1") addEdge("c1", "b2") - val sorted = sort(arrayListOf("a1", "a2", "b1", "b2", "c1", "x", "y")) - Assert.assertEquals(sorted, arrayListOf("a1", "a2", "x", "y", "b1", "b2", "c1")) + addNode("x") + addNode("y") + val sorted = sort() + Assert.assertEquals(sorted, arrayListOf("a1", "a2", "x", "y", "b2", "b1", "c1")) } }