From 8ef5a67b64b04b90703a6a815bd879adcdb89bb5 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 1 Apr 2019 12:11:03 -0700 Subject: [PATCH] Add better error message if run fails It is possible with the new layering strategies that tests may fail if a java package private class is accessed across classloader layers. This will result in an IllegalAccessError that is hard to debug. With this commit, I add an error message that will be displayed if run throws an IllegalAccessError that suggests that the user try the ScalaInstance layering strategy or the flat layering strategy. --- main/src/main/scala/sbt/Defaults.scala | 31 +++++++++---------- run/src/main/scala/sbt/Run.scala | 19 ++++++++++-- .../package-private/build.sbt | 1 + .../src/main/scala/sbt/classpath/Run.scala | 12 +++++++ .../classloader-cache/package-private/test | 9 ++++++ .../sbt/scriptedtest/ScriptedTests.scala | 1 + .../src/main/scala/sbt/TestFramework.scala | 18 ++++++++++- 7 files changed, 72 insertions(+), 19 deletions(-) create mode 100644 sbt/src/sbt-test/classloader-cache/package-private/build.sbt create mode 100644 sbt/src/sbt-test/classloader-cache/package-private/src/main/scala/sbt/classpath/Run.scala create mode 100644 sbt/src/sbt-test/classloader-cache/package-private/test diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 9fc852485..add344fc7 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -1056,22 +1056,21 @@ object Defaults extends BuildCommon { val result = output map { out => out.events.foreach { case (suite, e) => - e.throwables - .collectFirst { - case t - if t - .isInstanceOf[NoClassDefFoundError] && strategy != ClassLoaderLayeringStrategy.Flat => - t - } - .foreach { t => - s.log.error( - s"Test suite $suite failed with $t. This may be due to the ClassLoaderLayeringStrategy" - + s" ($strategy) used by your task. This issue may be resolved by changing the" - + " ClassLoaderLayeringStrategy in your configuration (generally Test or IntegrationTest)," - + "e.g.:\nTest / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.Flat\n" - + "See ClassLoaderLayeringStrategy.scala for the full list of options." - ) - } + if (strategy != ClassLoaderLayeringStrategy.Flat) { + e.throwables + .find { t => + t.isInstanceOf[NoClassDefFoundError] || t.isInstanceOf[IllegalAccessError] + } + .foreach { t => + s.log.error( + s"Test suite $suite failed with $t. This may be due to the ClassLoaderLayeringStrategy" + + s" ($strategy) used by your task. This issue may be resolved by changing the" + + " ClassLoaderLayeringStrategy in your configuration (generally Test or IntegrationTest)," + + " e.g.:\nTest / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.Flat\n" + + "See ClassLoaderLayeringStrategy.scala for the full list of options." + ) + } + } } val summaries = runners map { diff --git a/run/src/main/scala/sbt/Run.scala b/run/src/main/scala/sbt/Run.scala index 754974e1e..ad0496c7a 100644 --- a/run/src/main/scala/sbt/Run.scala +++ b/run/src/main/scala/sbt/Run.scala @@ -93,11 +93,26 @@ class Run(newLoader: Seq[File] => ClassLoader, trapExit: Boolean) extends ScalaR val main = getMainMethod(mainClassName, loader) invokeMain(loader, main, options) } - private def invokeMain(loader: ClassLoader, main: Method, options: Seq[String]): Unit = { + private def invokeMain( + loader: ClassLoader, + main: Method, + options: Seq[String] + ): Unit = { val currentThread = Thread.currentThread val oldLoader = Thread.currentThread.getContextClassLoader currentThread.setContextClassLoader(loader) - try { main.invoke(null, options.toArray[String]); () } finally { + try { main.invoke(null, options.toArray[String]); () } catch { + case t: Throwable => + t.getCause match { + case e: java.lang.IllegalAccessError => + val msg = s"Error running $main.\n$e\n" + + "If using a layered classloader, this can occur if jvm package private classes are " + + "accessed across layers. This can be fixed by changing to the Flat or " + + "ScalaInstance class loader layering strategies." + throw new IllegalAccessError(msg) + case _ => throw t + } + } finally { currentThread.setContextClassLoader(oldLoader) } } diff --git a/sbt/src/sbt-test/classloader-cache/package-private/build.sbt b/sbt/src/sbt-test/classloader-cache/package-private/build.sbt new file mode 100644 index 000000000..68a4b5df0 --- /dev/null +++ b/sbt/src/sbt-test/classloader-cache/package-private/build.sbt @@ -0,0 +1 @@ +libraryDependencies += "org.scala-sbt.ivy" % "ivy" % "2.3.0-sbt-cb9cc189e9f3af519f9f102e6c5d446488ff6832" \ No newline at end of file diff --git a/sbt/src/sbt-test/classloader-cache/package-private/src/main/scala/sbt/classpath/Run.scala b/sbt/src/sbt-test/classloader-cache/package-private/src/main/scala/sbt/classpath/Run.scala new file mode 100644 index 000000000..de1de4fab --- /dev/null +++ b/sbt/src/sbt-test/classloader-cache/package-private/src/main/scala/sbt/classpath/Run.scala @@ -0,0 +1,12 @@ +package org.apache.ivy.plugins.parser.m2 + +import org.apache.ivy.core.module.descriptor.DefaultDependencyDescriptor + +object Run { + def main(args: Array[String]): Unit = { + new PomModuleDescriptorBuilder.ConfMapper { + override def addMappingConfs(dd: DefaultDependencyDescriptor, isOptional: Boolean): Unit = {} + } + () + } +} \ No newline at end of file diff --git a/sbt/src/sbt-test/classloader-cache/package-private/test b/sbt/src/sbt-test/classloader-cache/package-private/test new file mode 100644 index 000000000..bda116d0c --- /dev/null +++ b/sbt/src/sbt-test/classloader-cache/package-private/test @@ -0,0 +1,9 @@ +-> run + +> set Compile / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.Flat + +> run + +> set Runtime / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.ScalaInstance + +> run diff --git a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala index 8375bf835..128e794ae 100644 --- a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala +++ b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala @@ -177,6 +177,7 @@ final class ScriptedTests( case "classloader-cache/jni" => LauncherBased // sbt/Package$ case "classloader-cache/library-mismatch" => LauncherBased // sbt/Package$ case "classloader-cache/runtime-layers" => LauncherBased // sbt/Package$ + case "classloader-cache/package-private" => LauncherBased // sbt/Package$ case "compiler-project/dotty-compiler-plugin" => LauncherBased // sbt/Package$ case "compiler-project/run-test" => LauncherBased // sbt/Package$ case "compiler-project/src-dep-plugin" => LauncherBased // sbt/Package$ diff --git a/testing/src/main/scala/sbt/TestFramework.scala b/testing/src/main/scala/sbt/TestFramework.scala index 3f7c68341..21df1cc8f 100644 --- a/testing/src/main/scala/sbt/TestFramework.scala +++ b/testing/src/main/scala/sbt/TestFramework.scala @@ -122,9 +122,25 @@ final class TestRunner( val results = new scala.collection.mutable.ListBuffer[Event] val handler = new EventHandler { def handle(e: Event): Unit = { results += e } } val loggers: Vector[ContentLogger] = listeners.flatMap(_.contentLogger(testDefinition)) + def errorEvents(e: Throwable): Array[sbt.testing.Task] = { + val taskDef = testTask.taskDef + val event = new Event { + val status = Status.Error + val throwable = new OptionalThrowable(e) + val fullyQualifiedName = taskDef.fullyQualifiedName + val selector = new TestSelector(name) + val fingerprint = taskDef.fingerprint + val duration = -1L + } + results += event + Array.empty + } val nestedTasks = try testTask.execute(handler, loggers.map(_.log).toArray) - finally { + catch { + case NonFatal(e) => errorEvents(e) + case e: IllegalAccessError => errorEvents(e) + } finally { loggers.foreach(_.flush()) } val event = TestEvent(results)