Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect classpath precedence for scala-xml #3405

Closed
joescii opened this issue Aug 4, 2017 · 9 comments
Closed

Incorrect classpath precedence for scala-xml #3405

joescii opened this issue Aug 4, 2017 · 9 comments
Labels

Comments

@joescii
Copy link

joescii commented Aug 4, 2017

While working on fixing a serialization bug in scala-xml, I have been able to convince myself that sbt 0.13.16 incorrectly places the project's scala-xml version per libraryDependencies AFTER the version of scala-xml that the scala compiler happens to pull in for sbt to use itself.

steps

The high level procedure is you need to publish a local build of scala-xml and pull a sample project you can demo the behavior on.

# Pull, build, publish my branch of scala-xml
git clone https://github.com/scala/scala-xml.git
cd ./scala-xml/
git remote add joescii https://github.com/joescii/scala-xml.git
git fetch joescii
git checkout -b nodeseq-serialization joescii/nodeseq-serialization 
sbt +publishLocal

# Pull and compile my project reproducing the issue
cd ..
git clone https://github.com/joescii/sbt-reproduced.git
cd ./sbt-reproduced/
git fetch origin
git checkout scala-xml
sbt compile

# sbt run will throw an exception because an older scala-xml jar with a serialization bug is used
sbt run

# sbt stage + native launch works perfectly
sbt stage
./target/universal/stage/bin/sbt-reproduced 

problem

The application should run in sbt the same way it runs outside. Using sbt run, you'll get something like the following:

descartes:sbt-reproduced joescii$ sbt run
[warn] Executing in batch mode.
[warn]   For better performance, hit [ENTER] to switch to interactive mode, or
[warn]   consider launching sbt without any commands, or explicitly passing 'shell'
[info] Loading global plugins from /Users/joescii/.clients/.sbt/0.13/plugins
[info] Loading project definition from /private/tmp/git/sbt-reproduced/project
[info] Set current project to sbt-reproduced (in build file:/private/tmp/git/sbt-reproduced/)
[info] Running code 
Serializing NodeSeq.Empty...
[error] (run-main-0) java.io.NotSerializableException: scala.xml.NodeSeq$$anon$1
java.io.NotSerializableException: scala.xml.NodeSeq$$anon$1

	at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1182)
	at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:348)
	at code$.serialize$1(code.scala:11)
	at code$.roundTrip(code.scala:22)
	at code$.delayedEndpoint$code$1(code.scala:26)
	at code$delayedInit$body.apply(code.scala:6)
	at scala.Function0$class.apply$mcV$sp(Function0.scala:34)
	at scala.runtime.AbstractFunction0.apply$mcV$sp(AbstractFunction0.scala:12)
	at scala.App$$anonfun$main$1.apply(App.scala:76)
	at scala.App$$anonfun$main$1.apply(App.scala:76)
	at scala.collection.immutable.List.foreach(List.scala:392)
	at scala.collection.generic.TraversableForwarder$class.foreach(TraversableForwarder.scala:35)
	at scala.App$class.main(App.scala:76)
	at code$.main(code.scala:6)
	at code.main(code.scala)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
[trace] Stack trace suppressed: run last compile:run for the full output.
java.lang.RuntimeException: Nonzero exit code: 1
	at scala.sys.package$.error(package.scala:27)
[trace] Stack trace suppressed: run last compile:run for the full output.
[error] (compile:run) Nonzero exit code: 1
[error] Total time: 0 s, completed Aug 4, 2017 11:47:07 AM

expectation

Running it after packaging with sbt-native-packager yields the expected result:

descartes:sbt-reproduced joescii$ ./target/universal/stage/bin/sbt-reproduced 
Serializing NodeSeq.Empty...

See? It works!!

notes

sbt version: 0.13.16
macOS Sierra 10.12.6

Versions of everything:

descartes:sbt-reproduced joescii$ java -version
java version "1.8.0_131"
Java(TM) SE Runtime Environment (build 1.8.0_131-b11)
Java HotSpot(TM) 64-Bit Server VM (build 25.131-b11, mixed mode)
descartes:sbt-reproduced joescii$ brew info sbt
sbt: stable 0.13.16, devel 1.0.0-RC2
Build tool for Scala projects
http://www.scala-sbt.org
/usr/local/Cellar/sbt/0.13.15 (378 files, 63.3MB)
  Built from source on 2017-05-22 at 09:19:37
/usr/local/Cellar/sbt/0.13.16 (384 files, 63.5MB) *
  Built from source on 2017-08-04 at 11:46:50
@eed3si9n
Copy link
Member

eed3si9n commented Aug 5, 2017

Thanks for the detailed reproduction steps.
I was able to reproduce it on my machine:

> run
[info] Running code
Serializing NodeSeq.Empty...
[error] (run-main-2) java.io.NotSerializableException: scala.xml.NodeSeq$$anon$1
java.io.NotSerializableException: scala.xml.NodeSeq$$anon$1
	at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1184)
	at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:348)

I thought maybe the classpath is somehow messed up, but runtime:fullClasspath seems to return something that looks ok:

> show runtime:fullClasspath
[info] * Attributed(/Users/eed3si9n/work/quicktest/sbt-reproduced/target/scala-2.11/classes)
[info] * Attributed(/Users/eed3si9n/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.11.11.jar)
[info] * Attributed(/Users/eed3si9n/.ivy2/local/org.scala-lang.modules/scala-xml_2.11/1.0.7-SNAPSHOT/bundles/scala-xml_2.11.jar)
[success] Total time: 0 s, completed Aug 4, 2017 10:15:02 PM

Here's a related observation from console:

scala> scala.xml.NodeSeq.Empty.isInstanceOf[scala.xml.NodeSeq]
res0: Boolean = true

scala> scala.xml.NodeSeq.Empty.isInstanceOf[Serializable]
res1: Boolean = false

@eed3si9n eed3si9n added the Bug label Aug 5, 2017
@joescii
Copy link
Author

joescii commented Aug 7, 2017

Yep, I had checked the fullClasspath as well before deciding this was an issue.

A few other interesting things I have found. This issue influenced how I set up my libraryDependencies to ensure my project explicitly used the version of scala-xml I wanted.

The scala-xml project itself gets something right, but not sure what. It appears that its test configuration is correct, because my tests pass when I added Serializable to the NodeSeq inheritance hierarchy, which is the expected behavior.

@ashawley
Copy link
Contributor

Issue in the scala compiler is scala/scala-dev#291

@SethTisue
Copy link
Member

there are multiple tickets on this, see scala/scala-xml#195 (comment)

@eed3si9n
Copy link
Member

eed3si9n commented Mar 2, 2018

Looking at this again. I think the problem is that during run sbt constructs a classloader that puts all JARs associated with scalaVersion and calls main. That's how we approximate run.

https://github.com/sbt/zinc/blob/v1.1.1/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClasspathUtilities.scala#L66-L72

  def createClasspathResources(classpath: Seq[File], instance: ScalaInstance): Map[String, String] =
    createClasspathResources(classpath, instance.allJars)

  def createClasspathResources(appPaths: Seq[File], bootPaths: Seq[File]): Map[String, String] = {
    def make(name: String, paths: Seq[File]) = name -> Path.makeString(paths)
    Map(make(AppClassPath, appPaths), make(BootClassPath, bootPaths))
  }

This puts compiler JARs into boot classpath. Is boot classpath for taking higher precedence than rt.jar? If so we'd likely just need Scala library here, and not the whole family (compiler, reflect)?

@eed3si9n
Copy link
Member

eed3si9n commented Mar 2, 2018

This behavior goes back to 2010 - 600053b
In there there's a scripted test added that's calling scala.tools.nsc.Interpreter, a class that's part of the compiler:

> ++2.8.0.RC2

@ashawley
Copy link
Contributor

ashawley commented Mar 2, 2018

FWIW, Mark Harrah left us a note:

https://github.com/sbt/sbt/wiki/Scala-modularization-and-classpaths

It seems relevant. There was also this associated discussion on scala-internals

https://groups.google.com/forum/#!msg/scala-internals/tT1pjH5GECE/1wtOZoG9bgQJ

@eed3si9n
Copy link
Member

eed3si9n commented Mar 2, 2018

and it's all configurable..? https://github.com/sbt/zinc/blob/v1.1.1/internal/compiler-interface/src/main/contraband-java/xsbti/compile/ClasspathOptions.java

https://github.com/sbt/zinc/blob/v1.1.1/internal/zinc-compile-core/src/main/java/xsbti/compile/ClasspathOptionsUtil.java

    /**
     * Define boot {@link ClasspathOptions} where:
     * 1. the Scala standard library is present in the classpath;
     * 2. the boot classpath is automatically configured; and,
     * 3. the Scala standard library JAR is fetched from the classpath.
     */
    public static ClasspathOptions boot() {
        return ClasspathOptions.of(true, false, false, true, true);
    }

eed3si9n added a commit to eed3si9n/zinc that referenced this issue Mar 2, 2018
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.
eed3si9n added a commit to eed3si9n/zinc that referenced this issue Mar 19, 2018
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.
@eed3si9n
Copy link
Member

eed3si9n commented Mar 27, 2018

This is now fixed in sbt 1.1.2.

sbt:sbt-reproduced> run
[warn] Choosing local for org.scala-lang.modules#scala-xml_2.11;1.0.7-SNAPSHOT
[info] Compiling 1 Scala source to /Users/eed3si9n/work/quicktest/sbt-reproduced/target/scala-2.11/classes ...
[info] Non-compiled module 'compiler-bridge_2.11' for Scala 2.11.11. Compiling...
[info]   Compilation completed in 8.873s.
[info] Done compiling.
[info] Packaging /Users/eed3si9n/work/quicktest/sbt-reproduced/target/scala-2.11/sbt-reproduced_2.11-0.1.0-SNAPSHOT.jar ...
[info] Done packaging.
[info] Running foo.code
Serializing NodeSeq.Empty...

See? It works!!
[success] Total time: 14 s, completed Mar 27, 2018 11:50:20 AM
sbt:sbt-reproduced> exit

eed3si9n added a commit to scala/compiler-interface that referenced this issue Apr 23, 2019
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.
dwijnand pushed a commit to dwijnand/sbt that referenced this issue Apr 25, 2019
Fixes 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants