include only scala-library into the classpath during run #505

eed3si9n · 2018-03-02T07:36:05Z

Fixes sbt/sbt#3405
Ref scala/scala-xml#195

sbt's run is emulated using a classloader trick that includes ScalaInstance as the parent classloader under the classpath. The problem is the ScalaInstance classloader currently contains both compiler, library, and their transitive JARs:

res0: Array[java.io.File] = Array(scala-library.jar, scala-compiler.jar, jline.jar, scala-reflect.jar, scala-xml_2.12.jar)

This could have been causing various issues, but most recently it showed up as wrong version of scala-xml getting prioritized over what's passed by the user.

retronym · 2018-03-02T07:39:04Z

internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClasspathUtilities.scala

+  /**
+   * Include only the scala-library.jar, if any, into the boot classpath.
+   * 1. Supposedly using boot classpath here is for performance reason:
+   *    https://groups.google.com/d/msg/scala-internals/tT1pjH5GECE/dtgRj3w7ovIJ


"boot classpath" isn't an accurate description of what's happening here. The performance trick is to cache and share libraryLoader in ScalaInstance, rather than forming new classpaths containing scala-library.jar.

createClasspathResources(cp, instance) does map things into BootClassPath.

retronym · 2018-03-02T07:45:31Z

internal/zinc-classpath/src/main/scala/sbt/internal/inc/ScalaInstance.scala

+  lazy val libraryLoader: ClassLoader = {
+    classpath.ClasspathUtilities.toLibraryLoader(this)
+  }
+


I think it would be preferable to do this way in in ScalaProvider, ie change the way it creates loader, and then just accept libraryLoader and fullLoader here.

Your change will end up with scala-library loaded separates in loader and libraryLoader. That's a functional problem if you ever need interop between programs running in those loaders. It is also a small performance problem in space and time for the VM.

You could sort of cheat without changing the interface of ScalaProvider and the intervening call chain.

The launcer could create the layered classloaders as I described, but still only pass the full one into ScalaInstance. Here, you could look at loader.getParent to get the libraryLoader (perhaps verifying that isIsntanceOf[LauncherLibraryClassloader] to know that the secret handshake is complete

If the secret handshake wasn't made (ie, we are being called by an old launcher), you could either do as you've done here, or just let the bug persist.

That's in launcher land. I would rather not make ppl reinstall launchers. (Assuming bunch of ppl are using official sbt package, not sbt-extra)

Here's what I've got in mind: https://github.com/retronym/launcher/tree/topic/library-loader

We could deliver both fixes: the launcher fix (that makes this part of the code trivial), and the existing fix you've got here (to fix the bug when using older launchers)

You'd need to deal with the xsbti.ExtendedScalaProvider interface reflectively in SBT to avoid a LinkageError with older launchers. This isn't much fun, but is a pretty standard way of evolving interfaces and implementations that require compatibility.

@jvican

I think this is the way to go -- being able to recycle JVM optimizations that have already happened for the Scala library is important, and classloading the library again goes against that.

I agree. I think I am now starting to see the light of what Jason is saying.

sbt/launcher#47

retronym · 2018-03-02T08:05:27Z

internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClasspathUtilities.scala

+          x.getName.startsWith("scala-library")
+        }).getOrElse {
+          throw new InvalidScalaProvider(s"Couldn't find scala-library")
+        }


I guess to future proof this could find the transitive dependencies of scala-library, which could be computed in launcher, but would need some extra metadata (the scala build would need to add this to our distribution) when loading from scalaHome.

eed3si9n · 2018-03-07T02:52:06Z

internal/zinc-classpath/src/main/scala/sbt/internal/inc/ScalaInstance.scala

+    val (loader, loaderLibraryOnly) =
+      (try {
+        provider match {
+          case p: ScalaProvider2 @unchecked => Option((provider.loader, p.loaderLibraryOnly))


updated to try the new loader first refectively, and fall back to manual creation.

typesafe-tools · 2018-03-07T18:06:44Z

The validator has checked the following projects against Scala 2.12,
tested using dbuild, projects built on top of each other.

Project	Reference	Commit
sbt	1.1.x	sbt/sbt@`27bbde8`
zinc	pull/505/head	`733e629`
io	1.1.x	sbt/io@`13eb241`
librarymanagement	1.1.x	sbt/librarymanagement@`4c5e1b9`
util	1.1.x	sbt/util@`226a543`
website	1.1.x

✅ The result is: SUCCESS
(restart)

eed3si9n · 2018-03-09T11:25:24Z

Addressed the review comments.
Could we get the ok on this, so we can include it in next sbt 1.1.x?

jvican · 2018-03-09T14:15:55Z

Cannot make the review today, but you'll have it by next Monday!

jvican

LGTM, just a couple of comments. Could you link to the PR that will integrate with this work over sbt/sbt?

jvican · 2018-03-12T16:17:25Z

internal/zinc-classpath/src/main/scala/sbt/internal/inc/ScalaInstance.scala

-    new ScalaInstance(version, loader, libraryJar, compilerJar, jars, None)
+    // sbt launcher 1.0.3 will construct layered classloader. Use them if we find them.
+    // otherwise, construct layered loaders manually.
+    val (loader, loaderLibraryOnly) =


Nitpicks: can we wrap all this logic in curly braces and separate the computation of the default value (what's in getOrElse) from the rest of the logic?

jvican · 2018-03-12T16:18:45Z

internal/zinc-classpath/src/main/scala/sbt/internal/inc/ScalaInstance.scala

    val version = actualVersion(loader)(" (library jar  " + library.getAbsolutePath + ")")
    val compiler = compilerJar(scalaHome)
-    new ScalaInstance(version, loader, library, compiler, all.toArray, None)
+    new ScalaInstance(version, loader, loaderLibraryOnly, library, compiler, all.toArray, None)


This is going to break quite a bit of code. I would honestly consider cutting a major breaking release... xsbti.compile is quite core to the Zinc API.

This is going to break quite a bit of code.

How so? Do you think there would be observable changes to sbt or other Zinc downstream users? (Besides being able to use the correct scala-xml version)

Didn't mean behaviour-wise, but API-wise 😄. I think ScalaInstance is used pretty much everywhere.

I see what you mean. Maybe I can try to add another constructor that is bincompat and deprecate it.

Yes, please do 👍

ok. Addressed the bincompat.

retronym · 2018-03-13T01:48:18Z

internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClasspathUtilities.scala

  def makeLoader(classpath: Seq[File], instance: ScalaInstance): ClassLoader =
-    filterByClasspath(classpath, makeLoader(classpath, instance.loader, instance))
+    filterByClasspath(classpath, makeLoader(classpath, instance.loaderLibraryOnly, instance))


We actually should make even the addition of loaderLibraryOnly conditional on autoScalaLibrary := true. In the Scala build, we have that setting disabled (because the scala-library we want on the classpath is actually a regular sub-project in our build, not the one in our reference Scala version).

Here's an example of how we get the unwanted class on our classpath: scala/scala#6300 (comment). So far, we have worked around by forking tests.

ok. That sounds like a bug on sbt's Run or TestFramework. By the time someone called makeLoader(...) with instance I think it's ok to include loaderLibraryOnly. If autoScalaLibrary is false then we have to be careful not to create ScalaInstance based loader basically.

jvican

I don't have anything else to add, 👍

eed3si9n · 2018-03-19T09:44:57Z

Cool. I'll squash the history and merge.

Fixes sbt/sbt#3405 Ref scala/scala-xml#195 sbt's `run` is emulated using a classloader trick that includes ScalaInstance as the parent classloader under the classpath. The problem is the ScalaInstance classloader currently contains both compiler, library, and their transitive JARs: ```scala res0: Array[java.io.File] = Array(scala-library.jar, scala-compiler.jar, jline.jar, scala-reflect.jar, scala-xml_2.12.jar) ``` This could have been causing various issues, but most recently it showed up as wrong version of scala-xml getting prioritized over what's passed by the user. 1. new field loaderLibraryOnly is added to xsbti.ScalaInstance. 2. it is initialized to the library loader if the launcher creates it, otherwise create layered loader here. This aims to isolate the library loader, and retain the perf.

I think I've made a mistake in #505 by using NoSuchMethodError. When trying to use Zinc 1.1.2, I ran into `java.lang.NoSuchMethodException`. This is because `NoSuchMethodException` would be thrown for reflective calls, not `NoSuchMethodError`.

I think I've made a mistake in sbt#505 by using NoSuchMethodError. When trying to use Zinc 1.1.2, I ran into `java.lang.NoSuchMethodException`. This is because `NoSuchMethodException` would be thrown for reflective calls, not `NoSuchMethodError`.

SethTisue · 2018-03-27T16:41:23Z

thanks @eed3si9n , so glad you took this on 👏👏👏

eed3si9n requested a review from retronym March 2, 2018 07:36

retronym reviewed Mar 2, 2018

View reviewed changes

eed3si9n mentioned this pull request Mar 2, 2018

Layered class loading of library and compiler sbt/launcher#47

Merged

jvican added bug in progress labels Mar 5, 2018

eed3si9n changed the title ~~include only scala-library into the boot classpath during run~~ include only scala-library into the classpath during run Mar 7, 2018

eed3si9n commented Mar 7, 2018

View reviewed changes

jvican reviewed Mar 12, 2018

View reviewed changes

retronym reviewed Mar 13, 2018

View reviewed changes

jvican mentioned this pull request Mar 19, 2018

Update to latest zinc scalacenter/bloop#383

Closed

jvican previously approved these changes Mar 19, 2018

View reviewed changes

eed3si9n dismissed jvican’s stale review via 9397b6a March 19, 2018 10:06

eed3si9n force-pushed the wip/run-fix branch from 361a235 to 9397b6a Compare March 19, 2018 10:06

jvican approved these changes Mar 20, 2018

View reviewed changes

dwijnand added this to the 1.1.2 milestone Mar 21, 2018

eed3si9n merged commit a229439 into sbt:1.1.x Mar 23, 2018

eed3si9n deleted the wip/run-fix branch March 23, 2018 12:29

eed3si9n mentioned this pull request Mar 23, 2018

Fixes the reflection fallback for ScalaInstance #514

Merged

ashawley mentioned this pull request Mar 27, 2018

make scala-xml a provided dependency for scala-compiler, remove it from scaladoc? scala/scala-dev#291

Closed

eed3si9n mentioned this pull request Mar 29, 2018

SBT does not use the specified JLine sbt/sbt#4026

Closed

eed3si9n mentioned this pull request May 14, 2018

Non-fork test ends up with unwanted scala-library even with autoScalaLibrary := false sbt/sbt#4009

Open

eed3si9n mentioned this pull request Jan 30, 2019

java.lang.NoClassDefFoundError: scala/math/Ordering$$anon$9 in runtime after adding sbt plugin sbt/sbt#4525

Closed

1 task

SethTisue mentioned this pull request Apr 6, 2019

on Scala 2.11, version 1.1.2 may run afoul of sbt classpath issue scala/scala-parser-combinators#197

Closed

eed3si9n mentioned this pull request Apr 7, 2019

[1.2.x] on Scala 2.11 scala-parser-combinators 1.1.2 on test requires forking sbt/sbt#4609

Closed

include only scala-library into the classpath during run #505

include only scala-library into the classpath during run #505

Uh oh!

Conversation

eed3si9n commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

retronym Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

retronym Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typesafe-tools commented Mar 7, 2018

Uh oh!

eed3si9n commented Mar 9, 2018

Uh oh!

jvican commented Mar 9, 2018

Uh oh!

jvican left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

retronym Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvican left a comment

Choose a reason for hiding this comment

Uh oh!

eed3si9n commented Mar 19, 2018

Uh oh!

SethTisue commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

retronym Mar 2, 2018 •

edited

Loading

retronym Mar 2, 2018 •

edited

Loading

retronym Mar 13, 2018 •

edited

Loading

SethTisue commented Mar 27, 2018 •

edited

Loading