More robust partest/test switching for concurrent sbt instances

4 files changed, 56 insertions, 58 deletions

diff --git a/.gitignore b/.gitignore
index 69d86527a..95d95ca11 100644
--- a/.gitignore
+++ b/.gitignore
@@ -28,4 +28,5 @@ classes/
 
 # Partest
 tests/partest-generated/
+tests/locks/
 /test-classes/
diff --git a/project/Build.scala b/project/Build.scala
index 7d5d59ec5..d21b4c807 100644
--- a/project/Build.scala
+++ b/project/Build.scala
@@ -17,8 +17,6 @@ object DottyBuild extends Build {
     // "-XX:+HeapDumpOnOutOfMemoryError", "-Xmx1g", "-Xss2m"
   )
 
-  var partestLock: FileLock = null
-
   val defaults = Defaults.defaultSettings ++ Seq(
     scalaVersion in Global := "2.11.5",
     version in Global := "0.1-SNAPSHOT",
@@ -63,25 +61,18 @@ object DottyBuild extends Build {
 
     // enable verbose exception messages for JUnit
     testOptions in Test += Tests.Argument(TestFrameworks.JUnit, "-a", "-v", "--run-listener=test.ContextEscapeDetector"),
-    testOptions in Test += Tests.Cleanup({ () => if (partestLock != null) partestLock.release }),
-    // When this file is locked, running test generates the files for partest.
-    // Otherwise it just executes the tests directly. So running two separate
-    // sbt instances might result in unexpected behavior if one does partest
-    // and the other test, since the second still sees the locked file and thus
-    // generates partest files instead of running JUnit tests, but doesn't
-    // partest them.
+    testOptions in Test += Tests.Cleanup({ () => partestLockFile.delete }),
+
     lockPartestFile := {
-      val partestLockFile = "." + File.separator + "tests" + File.separator + "partest.lock"
-      try {
-        partestLock = new RandomAccessFile(partestLockFile, "rw").getChannel.tryLock
-        if (partestLock == null)
-          throw new RuntimeException("ERROR: sbt partest: file is locked already. Bad things happen when trying to mix test/partest in two concurrent sbt instances.")
-      } catch {
-        case ex: java.nio.channels.OverlappingFileLockException => // locked already, Tests.Cleanup didn't run
-          if (partestLock != null)
-            partestLock.release
-          throw new RuntimeException("ERROR: sbt partest: file was still locked, please try again or restart sbt.")
-      }
+      // When this file is present, running `test` generates the files for
+      // partest. Otherwise it just executes the tests directly.
+      val lockDir = partestLockFile.getParentFile
+      lockDir.mkdirs
+      // Cannot have concurrent partests as they write to the same directory.
+      if (lockDir.list.size > 0)
+        throw new RuntimeException("ERROR: sbt partest: another partest is already running, pid in lock file: " + lockDir.list.toList.mkString(" "))
+      partestLockFile.createNewFile
+      partestLockFile.deleteOnExit
     },
     runPartestRunner <<= Def.taskDyn {
       val jars = Seq((packageBin in Compile).value.getAbsolutePath) ++ 
@@ -99,29 +90,29 @@ object DottyBuild extends Build {
 
     // http://grokbase.com/t/gg/simple-build-tool/135ke5y90p/sbt-setting-jvm-boot-paramaters-for-scala
     javaOptions <++= (managedClasspath in Runtime, packageBin in Compile) map { (attList, bin) =>
-       // put the Scala {library, reflect} in the classpath
-       val path = for {
-         file <- attList.map(_.data)
-         path = file.getAbsolutePath
-       } yield "-Xbootclasspath/p:" + path
-       // dotty itself needs to be in the bootclasspath
-       val fullpath = ("-Xbootclasspath/a:" + bin) :: path.toList
-       // System.err.println("BOOTPATH: " + fullpath)
-
-       val travis_build = // propagate if this is a travis build
-         if (sys.props.isDefinedAt(TRAVIS_BUILD))
-           List(s"-D$TRAVIS_BUILD=${sys.props(TRAVIS_BUILD)}") ::: travisMemLimit
-         else
-           List()
-
-       val tuning =
-         if (sys.props.isDefinedAt("Oshort"))
-           // Optimize for short-running applications, see https://github.com/lampepfl/dotty/issues/222
-           List("-XX:+TieredCompilation", "-XX:TieredStopAtLevel=1")
+      // put the Scala {library, reflect} in the classpath
+      val path = for {
+        file <- attList.map(_.data)
+        path = file.getAbsolutePath
+      } yield "-Xbootclasspath/p:" + path
+      // dotty itself needs to be in the bootclasspath
+      val fullpath = ("-Xbootclasspath/a:" + bin) :: path.toList
+      // System.err.println("BOOTPATH: " + fullpath)
+
+      val travis_build = // propagate if this is a travis build
+        if (sys.props.isDefinedAt(TRAVIS_BUILD))
+          List(s"-D$TRAVIS_BUILD=${sys.props(TRAVIS_BUILD)}") ::: travisMemLimit
+        else
+          List()
+
+      val tuning =
+        if (sys.props.isDefinedAt("Oshort"))
+          // Optimize for short-running applications, see https://github.com/lampepfl/dotty/issues/222
+          List("-XX:+TieredCompilation", "-XX:TieredStopAtLevel=1")
         else
           List()
 
-      tuning ::: agentOptions ::: travis_build ::: fullpath
+      ("-DpartestParentID=" + pid) :: tuning ::: agentOptions ::: travis_build ::: fullpath
     }
   ) ++ addCommandAlias("partest", ";test:compile;lockPartestFile;test:test;runPartestRunner")
 
@@ -174,10 +165,14 @@ object DottyBuild extends Build {
   lazy val benchmarks = Project(id = "dotty-bench", settings = benchmarkSettings,
     base = file("bench")) dependsOn(dotty % "compile->test")
 
-  lazy val lockPartestFile = TaskKey[Unit]("lockPartestFile", "Creates the file lock on  ./tests/partest.lock")
-  lazy val runPartestRunner = TaskKey[Unit]("runPartestRunner", "Runs partests")
-  lazy val partestDeps = SettingKey[Seq[ModuleID]]("partestDeps", "Finds jars for partest dependencies")
+  // Partest tasks
+  lazy val lockPartestFile = TaskKey[Unit]("lockPartestFile", "Creates the lock file at ./tests/locks/partest-<pid>.lock")
+  val partestLockFile = new File("." + File.separator + "tests" + File.separator + "locks" + File.separator + s"partest-$pid.lock")
+  val pid = java.lang.Long.parseLong(java.lang.management.ManagementFactory.getRuntimeMXBean().getName().split("@")(0))
 
+  lazy val runPartestRunner = TaskKey[Unit]("runPartestRunner", "Runs partest")
+
+  lazy val partestDeps = SettingKey[Seq[ModuleID]]("partestDeps", "Finds jars for partest dependencies")
   def getJarPaths(modules: Seq[ModuleID], ivyHome: Option[File]): Seq[String] = ivyHome match {
     case Some(home) => 
       modules.map({ module => 
diff --git a/test/test/CompilerTest.scala b/test/test/CompilerTest.scala
index d710a26d8..0e5c9a6b4 100644
--- a/test/test/CompilerTest.scala
+++ b/test/test/CompilerTest.scala
@@ -14,9 +14,9 @@ import org.junit.Test
 /** This class has two modes: it can directly run compiler tests, or it can
   * generate the necessary file structure for partest in the directory
   * DPConfig.testRoot. Both modes are regular JUnit tests. Which mode is used
-  * depends on the existence of the tests/runPartest.flag file which is created
-  * by sbt to trigger partest generation. Sbt can then run partest on the
-  * generated sources.
+  * depends on the existence of the tests/locks/partest-ppid.lock file which is
+  * created by sbt to trigger partest generation. Sbt will then run partest on
+  * the generated sources.
   *
   * Through overriding the partestableXX methods, tests can always be run as
   * JUnit compiler tests. Run tests cannot be run by JUnit, only by partest.
@@ -45,18 +45,20 @@ abstract class CompilerTest extends DottyTest {
   def partestableList(testName: String, files: List[String], args: List[String], xerrors: Int) = true
 
   val generatePartestFiles = {
-    val partestLockFile = "." + JFile.separator + "tests" + JFile.separator + "partest.lock"
-    try {
-      val partestLock = new RandomAccessFile(partestLockFile, "rw").getChannel.tryLock
-      if (partestLock != null) { // file not locked by sbt -> don't generate partest
-        partestLock.release
-        false
-      } else true
-    } catch {
-      // if sbt doesn't fork in Test, the tryLock request will throw instead of
-      // returning null, because locks are per JVM, not per thread
-      case ex: java.nio.channels.OverlappingFileLockException => true
-    }
+    /* Because we fork in test, the JVM in which this JUnit test runs has a
+     * different pid from the one that started the partest. But the forked VM
+     * receives the pid of the parent as system property. If the lock file
+     * exists, the parent is requesting partest generation. This mechanism
+     * allows one sbt instance to run test (JUnit only) and another partest.
+     * We cannot run two instances of partest at the same time, because they're
+     * writing to the same directories. The sbt lock file generation prevents
+     * this.
+     */
+    val pid = System.getProperty("partestParentID")
+    if (pid == null)
+      false
+    else
+      new JFile("." + JFile.separator + "tests" + JFile.separator + "locks" + JFile.separator + s"partest-$pid.lock").exists
   }
 
   // Delete generated files from previous run
diff --git a/tests/partest.lock b/tests/partest.lock
deleted file mode 100644
index e69de29bb..000000000
--- a/tests/partest.lock
+++ /dev/null