GITHUB-231: Fix the incorrect order of builds.

Fixes https://github.com/cbeust/kobalt/issues/231

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.*
-import com.beust.kobalt.misc.benchmarkMillis
-import com.beust.kobalt.misc.kobaltError
-import com.beust.kobalt.misc.log
 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<String>, projects: List<Project>): List<TaskInfo> {
         val projectMap = hashMapOf<String, Project>().apply {
-            projects.forEach { put(it.name, it)}
+            projects.forEach { project -> put(project.name, project)}
-        }
-        val result = ArrayList(taskNames.map { TaskInfo(it) })
-        val toProcess = ArrayList(result)
-        val newToProcess = arrayListOf<TaskInfo>()
-        val seen = hashSetOf<TaskInfo>()
-        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()
         }
 
-        return result
+        val allTaskInfos = HashSet(taskNames.map { TaskInfo(it) })
+        with(Topological<TaskInfo>()) {
+            val toProcess = ArrayList(allTaskInfos)
+            val seen = HashSet(allTaskInfos)
+            val newTasks = hashSetOf<TaskInfo>()
+            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

modules/kobalt-plugin-api/src/main/kotlin/com/beust/kobalt/misc/Topological.kt

@@ -10,22 +10,25 @@ import java.util.*
  */
 class Topological<T> {
     private val dependingOn = ArrayListMultimap.create<T, T>()
+    private val nodes = hashSetOf<T>()
+
+    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<out T>) {
-        dependingOn.putAll(t, others.toMutableList())
-    }
-
     /**
      * @return the Ts sorted topologically.
      */
-    fun sort(all: ArrayList<T>) : List<T> {
+    fun sort() : List<T> {
+        val all = ArrayList<T>(nodes)
         val result = arrayListOf<T>()
         var dependMap = HashMultimap.create<T, T>()
         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()

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
         }

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<Array<out Any>> {
+        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<String>, expectedTasks: List<String>) {
+        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<TaskManager.TaskInfo>, 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<TaskManager.TaskInfo>, firsts: List<String>, 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")
+        }
+    }
+
 }

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"))
+            addNode("x")
-            Assert.assertEquals(sorted, arrayListOf("a1", "a2", "x", "y", "b1", "b2", "c1"))
+            addNode("y")
+            val sorted = sort()
+            Assert.assertEquals(sorted, arrayListOf("a1", "a2", "x", "y", "b2", "b1", "c1"))
         }
     }