Merge pull request #5327 from eatkins/revert-scalatest-layer

Revert scalatest layer

main/src/main/java/sbt/internal/ScalaTestFrameworkClassLoader.java

@@ -1,34 +0,0 @@
-/*
- * sbt
- * Copyright 2011 - 2018, Lightbend, Inc.
- * Copyright 2008 - 2010, Mark Harrah
- * Licensed under Apache License 2.0 (see LICENSE)
- */
-
-package sbt.internal;
-
-    import java.net.URL;
-    import java.net.URLClassLoader;
-
-final class ScalaTestFrameworkClassLoader extends URLClassLoader {
-  static {
-    ClassLoader.registerAsParallelCapable();
-  }
-
-  private final URL[] jars;
-
-  ScalaTestFrameworkClassLoader(final URL[] jars, final ClassLoader parent) {
-    super(jars, parent);
-    this.jars = jars;
-  }
-
-  @Override
-  public String toString() {
-    final StringBuilder builder = new StringBuilder();
-    for (int i = 0; i < jars.length; ++ i) {
-      builder.append(jars[i].toString());
-      if (i < jars.length - 2) builder.append(", ");
-    }
-    return "ScalaTestFrameworkClassLoader(" + builder + " parent = " + getParent() + ")";
-  }
-}

main/src/main/scala/sbt/internal/ClassLoaders.scala

@@ -161,25 +161,11 @@ private[sbt] object ClassLoaders {
         val cpFiles = fullCP.map(_._1)
 
         val allDependencies = cpFiles.filter(allDependenciesSet)
-        /*
+        def isReflectJar(f: File): Boolean =
-         * We need to put scalatest and its dependencies (scala-xml) in its own layer so that the
-         * classloader for scalatest is generally not ever closed. This is because one project
-         * closing its scalatest loader can cause a crash in another project if that project
-         * is reading the scalatest bundle properties because the underlying JarFile instance is
-         * shared across URLClassLoaders if it points to the same jar.
-         *
-         * TODO: Make this configurable and/or add other frameworks in this layer.
-         */
-        val (frameworkDependencies, otherDependencies) = allDependencies.partition { d =>
-          val name = d.getName
-          name.startsWith("scalatest_") || name.startsWith("scalactic_") ||
-          name.startsWith("scala-xml_")
-        }
-        def isReflectJar(f: File) =
           f.getName == "scala-reflect.jar" || f.getName.startsWith("scala-reflect-")
-        val scalaReflectJar = otherDependencies.collectFirst {
+        val scalaReflectJar = allDependencies.collectFirst {
-          case f if isReflectJar(f) => si.allJars.find(isReflectJar).getOrElse(f)
+          case f if isReflectJar(f) => si.allJars.find(isReflectJar)
-        }
+        }.flatten
         val scalaReflectLayer = scalaReflectJar
           .map { file =>
             cache.apply(
@@ -190,46 +176,35 @@ private[sbt] object ClassLoaders {
           }
           .getOrElse(scalaLibraryLayer)
 
-        // layer 2 framework jars
+        // layer 2 (optional if in the test config and the runtime layer is not shared)
-        val frameworkLayer =
-          if (frameworkDependencies.isEmpty) scalaReflectLayer
-          else {
-            cache.apply(
-              frameworkDependencies.map(file => file -> IO.getModifiedTimeOrZero(file)).toList,
-              scalaLibraryLayer,
-              () => new ScalaTestFrameworkClassLoader(frameworkDependencies.urls, scalaReflectLayer)
-            )
-          }
-
-        // layer 3 (optional if in the test config and the runtime layer is not shared)
         val dependencyLayer: ClassLoader =
-          if (layerDependencies && otherDependencies.nonEmpty) {
+          if (layerDependencies && allDependencies.nonEmpty) {
             cache(
-              otherDependencies.toList.map(f => f -> IO.getModifiedTimeOrZero(f)),
+              allDependencies.toList.map(f => f -> IO.getModifiedTimeOrZero(f)),
-              frameworkLayer,
+              scalaReflectLayer,
               () =>
                 new ReverseLookupClassLoaderHolder(
-                  otherDependencies,
+                  allDependencies,
-                  frameworkLayer,
+                  scalaReflectLayer,
                   close,
                   allowZombies,
                   logger
                 )
             )
-          } else frameworkLayer
+          } else scalaReflectLayer
 
         val scalaJarNames = (si.libraryJars ++ scalaReflectJar).map(_.getName).toSet
         // layer 3
         val filteredSet =
           if (layerDependencies) allDependencies.toSet ++ si.libraryJars ++ scalaReflectJar
-          else Set(frameworkDependencies ++ si.libraryJars ++ scalaReflectJar: _*)
+          else Set(si.libraryJars ++ scalaReflectJar: _*)
         val dynamicClasspath = cpFiles.filterNot(f => filteredSet(f) || scalaJarNames(f.getName))
         dependencyLayer match {
           case dl: ReverseLookupClassLoaderHolder =>
-            dl.checkout(dynamicClasspath, tmp)
+            dl.checkout(cpFiles, tmp)
           case cl =>
             cl.getParent match {
-              case dl: ReverseLookupClassLoaderHolder => dl.checkout(dynamicClasspath, tmp)
+              case dl: ReverseLookupClassLoaderHolder => dl.checkout(cpFiles, tmp)
               case _ =>
                 new LayeredClassLoader(dynamicClasspath.urls, cl, tmp, close, allowZombies, logger)
             }

main/src/main/scala/sbt/internal/LayeredClassLoaders.scala

@@ -56,7 +56,7 @@ private[internal] final class ReverseLookupClassLoaderHolder(
    *
    * @return a ClassLoader
    */
-  def checkout(dynamicClasspath: Seq[File], tempDir: File): ClassLoader = {
+  def checkout(fullClasspath: Seq[File], tempDir: File): ClassLoader = {
     if (closed.get()) {
       val msg = "Tried to extract class loader from closed ReverseLookupClassLoaderHolder. " +
         "Try running the `clearCaches` command and re-trying."
@@ -69,7 +69,7 @@ private[internal] final class ReverseLookupClassLoaderHolder(
     reverseLookupClassLoader.setup(tempDir)
     new BottomClassLoader(
       ReverseLookupClassLoaderHolder.this,
-      dynamicClasspath.map(_.toURI.toURL).toArray,
+      fullClasspath.map(_.toURI.toURL).toArray,
       reverseLookupClassLoader,
       tempDir,
       closeThis,

main/src/main/scala/sbt/internal/SysProp.scala

@@ -85,7 +85,7 @@ object SysProp {
    */
   lazy val color: Boolean = ConsoleAppender.formatEnabledInEnv
 
-  def closeClassLoaders: Boolean = getOrTrue("sbt.classloader.close")
+  def closeClassLoaders: Boolean = getOrFalse("sbt.classloader.close")
 
   def fileCacheSize: Long =
     SizeParser(System.getProperty("sbt.file.cache.size", "128M")).getOrElse(128L * 1024 * 1024)

sbt/src/sbt-test/tests/scalatest/build.sbt

@@ -0,0 +1,12 @@
+ThisBuild / scalaVersion     := "2.13.1"
+ThisBuild / version          := "0.1.0-SNAPSHOT"
+ThisBuild / organization     := "com.example"
+ThisBuild / organizationName := "example"
+
+lazy val root = (project in file("."))
+  .settings(
+    name := "sbt-5308",
+    libraryDependencies += "org.scalatest" %% "scalatest" % "3.1.0" % Test,
+    libraryDependencies += "com.vladsch.flexmark" % "flexmark-all" % "0.35.10" % Test,
+    Test / testOptions += Tests.Argument(TestFrameworks.ScalaTest, "-h", "target/generated/test-reports"),
+  )

sbt/src/sbt-test/tests/scalatest/src/test/scala/example/HelloSpec.scala

@@ -0,0 +1,9 @@
+package example
+
+import org.scalatest._
+
+class HelloSpec extends FlatSpec with Matchers {
+  "hello" should "say hello" in {
+    assert("hello" == "hello")
+  }
+}

sbt/src/sbt-test/tests/scalatest/test

@@ -0,0 +1 @@
+> test

scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala

@@ -248,7 +248,8 @@ final class ScriptedTests(
         LauncherBased // there is an issue with jansi and coursier
       case "watch/commands" =>
         LauncherBased // java.lang.ClassNotFoundException: javax.tools.DiagnosticListener when run with java 11 and an old sbt launcher
-      case "watch/managed" => LauncherBased // sbt/Package$
+      case "watch/managed"   => LauncherBased // sbt/Package$
+      case "tests/scalatest" => LauncherBased
       case "tests/test-cross" =>
         LauncherBased // the sbt metabuild classpath leaks into the test interface classloader in older versions of sbt
       case _ => RunFromSourceBased