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

Compiler classpath : add file entries of URLClassLoader if available #4741

Closed
wants to merge 3 commits into from

Conversation

@rjolly
Copy link
Contributor

commented Sep 11, 2015

This is a follow-up to #4456 . In order to inform the classpath of the compiler, when run from an application such as a script engine or a reflection toolbox, we look for URLClassLoaderS in the classloader hierarchy. Then we collect all "file" entries of said classloaders and we add them to the compiler's classpath. Both a system property and a command line parameter are provided to enable the mechanism.

@scala-jenkins scala-jenkins added this to the 2.11.8 milestone Sep 11, 2015

@retronym

This comment has been minimized.

Copy link
Member

commented Sep 14, 2015

I like where this is going! Could you please reimplement getClassPath? It currently looks too close to the the code in twitter Eval, which we can't use unless it is contributed by the original author.

@retronym retronym self-assigned this Sep 14, 2015

@retronym

This comment has been minimized.

Copy link
Member

commented Sep 14, 2015

The build failure is spurious; SI-9264 strikes again.

[quick.compiler] error: scala.reflect.internal.FatalError: symbol value x2#827763 does not exist in scala.tools.nsc.transform.patmat.Solving$Solver$class.findTseitinModelFor, which contains locals value unitLit#525145,value 
@retronym

This comment has been minimized.

Copy link
Member

commented Sep 14, 2015

Let's add an automated test for toolbox along the lines of:

⚡ tail -n1000 test/files/run/t6393/*.scala
==> test/files/run/t6393/A.scala <==
package p1 {
  package p2 {
    class A
    object A
  }
}

==> test/files/run/t6393/Test.scala <==
object Test {
  def main(args: Array[String]): Unit = {
    import scala.tools.reflect.ToolBox
    val m = scala.reflect.runtime.currentMirror
    val u = m.universe
    import u._
    val tb = m.mkToolBox("-Yuseurlcp")

    { import p1.p2; new p2.A }
    tb.eval(q"import p1.p2; new p2.A")
  }
}
@retronym

This comment has been minimized.

Copy link
Member

commented Sep 14, 2015

/cc @xeno-by

@rjolly rjolly force-pushed the rjolly:scripting14 branch from ae486d7 to f49e7d3 Sep 14, 2015

@rjolly

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2015

I think the option passed to mkToolBox is ineffective. See the compiler classpath as shown by Ylog-classpath, it contains test/files/run/t6393-run.obj but to no avail.

@retronym

This comment has been minimized.

Copy link
Member

commented Sep 15, 2015

@rjolly I've taken a look at the way that toolbox creates the Global instance. I was able to make it work with a regular Global, which makes the relative imports work.

https://github.com/scala/scala/compare/scala:2.11.x...retronym:review/4741?expand=1

In that branch, I predicated this behaviour on the presence of the new -useurlcp option. We need to think this through a little bit more, it might be cleaner to have a different factory method alongside mkToolBox that makes it clear that we won't use reflection and that the user must provide compiler options to build up the classpath (-useurlcp, -usejavacp, -classpath).

To complete the picture, I'd like to enable this to work under SBT. There is no currently no command line option to call settings.embeddedDefaults(someClassLoader) Perhaps the new factory method could just call that unconditionally (it does no harm if not running under SBT.)

@retronym

This comment has been minimized.

Copy link
Member

commented Sep 15, 2015

@xeno-by WDYT?

@rjolly rjolly force-pushed the rjolly:scripting14 branch from f49e7d3 to 8fd0ad3 Sep 15, 2015

@rjolly

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2015

This is what I understand of the embeddedDefaults mechanism:

  • sbt needs to run its tasks with a specific classpath
  • for this, its creates specific classloaders (URLClassLoader with relevant jars)
  • this is working well for general tasks
  • but for the compiler, this is not sufficient : it needs to get its classpath by another way than the classloader
  • enter embeddedDefaults, which works as follows : class in relevant jar -> classloader -> ressource with names of jar files -> jar files -> command line option for compiler

If we use our new url classpath mechanism, there is no more need for this : the compiler will get its classpath automatically from the classloader of the current sbt task. We just need to pick the right one, and the context class loader seems a good bet.

@retronym

This comment has been minimized.

Copy link
Member

commented Sep 16, 2015

Here are the original design notes for embeddedDefaults: https://gist.github.com/harrah/404272

Here's an empirical look at the difference between using the entries from the current URL classloader vs the embeddedDefaults mechanism: https://gist.github.com/retronym/556c3c3e9aa98cc3357d

AFAICT, the only difference is that the latter populates the -bootclasspath parameter.

embeddedDefaults only works for in-process execution in SBT, under a forked run you need to use the URL classloader approach.

@rjolly

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2015

I do not see the point of a bootclasspath different from the java one.

rjolly and others added some commits Sep 11, 2015

SI-6393 Allow use of a non-reflection based Global in toolbox
  - Conditionally use a regular global as the toolbox compiler
  - Use the thread context classloader, rather than `classOf[Classpath].getClassLoader`
  - Pass the toolbox classloader down through the context classloader

A few API design questions remain and are outlined in a comment.

@rjolly rjolly force-pushed the rjolly:scripting14 branch from 8fd0ad3 to e6eb777 Sep 16, 2015

@xeno-by

This comment has been minimized.

Copy link
Member

commented Sep 16, 2015

I think I'll be able to take a look only after Scala World (next Thursday). Is that okay for you, guys?

@retronym

This comment has been minimized.

Copy link
Member

commented Sep 18, 2015

@rjolly I extended my test case to show the difference between the boot classpath of the JVM running SBT and the boot classpath used by a compiler under embeddedDefaults.

import java.net.URLClassLoader

object Test {
  def main(args: Array[String]): Unit = {
    val loader = getClass.getClassLoader
    loader match {
      case u: URLClassLoader => println("u.getURLs = " + u.getURLs.map(u => new java.io.File(u.toURI).toString).mkString(":"))
      case _ => println("loader.getClass", loader.getClass)
    }
    println("sun.boot.class.path = " + sys.props("sun.boot.class.path"))
    println("")
    val settings = new scala.tools.nsc.Settings
    settings.embeddedDefaults[Test.type]
    println("embeddedDefaults")
    println(" - classpath: " + settings.classpath.value)
    println(" - bootclasspath: " + settings.bootclasspath.value)
  }
}
u.getURLs = /Users/jason/code/scratch2/target/scala-2.11/classes:/Users/jason/.ivy2/cache/org.scala-lang/scala-compiler/jars/scala-compiler-2.11.7.jar:/Users/jason/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.11.7.jar:/Users/jason/.ivy2/cache/org.scala-lang/scala-reflect/jars/scala-reflect-2.11.7.jar:/Users/jason/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11/bundles/scala-xml_2.11-1.0.4.jar:/Users/jason/.ivy2/cache/org.scala-lang.modules/scala-parser-combinators_2.11/bundles/scala-parser-combinators_2.11-1.0.4.jar
sun.boot.class.path = /Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home/jre/lib/resources.jar:/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home/jre/lib/rt.jar:/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home/jre/lib/sunrsasign.jar:/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home/jre/lib/jsse.jar:/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home/jre/lib/jce.jar:/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home/jre/lib/charsets.jar:/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home/jre/lib/jfr.jar:/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home/jre/classes

embeddedDefaults
 - classpath: /Users/jason/code/scratch2/target/scala-2.11/classes:/Users/jason/.ivy2/cache/org.scala-lang/scala-compiler/jars/scala-compiler-2.11.7.jar:/Users/jason/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.11.7.jar:/Users/jason/.ivy2/cache/org.scala-lang/scala-reflect/jars/scala-reflect-2.11.7.jar:/Users/jason/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11/bundles/scala-xml_2.11-1.0.4.jar:/Users/jason/.ivy2/cache/org.scala-lang.modules/scala-parser-combinators_2.11/bundles/scala-parser-combinators_2.11-1.0.4.jar
 - bootclasspath: /Users/jason/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.11.7.jar:/Users/jason/.ivy2/cache/org.scala-lang/scala-compiler/jars/scala-compiler-2.11.7.jar:/Users/jason/.ivy2/cache/org.scala-lang/scala-reflect/jars/scala-reflect-2.11.7.jar:/Users/jason/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11/bundles/scala-xml_2.11-1.0.4.jar:/Users/jason/.ivy2/cache/org.scala-lang.modules/scala-parser-combinators_2.11/bundles/scala-parser-combinators_2.11-1.0.4.jar:/Users/jason/.ivy2/cache/jline/jline/jars/jline-2.12.1.jar

The latter is formed from the JARs from the Scala version defined in build.sbt.

sbt> consoleProject
...
scala> scalaInstance.eval.jars
res1: List[java.io.File] = List(/Users/jason/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.11.7.jar, /Users/jason/.ivy2/cache/org.scala-lang/scala-compiler/jars/scala-compiler-2.11.7.jar, /Users/jason/.ivy2/cache/org.scala-lang/scala-reflect/jars/scala-reflect-2.11.7.jar, /Users/jason/.ivy2/cache/org.scala-lang.modules/scala-xml_2.11/bundles/scala-xml_2.11-1.0.4.jar, /Users/jason/.ivy2/cache/org.scala-lang.modules/scala-parser-combinators_2.11/bundles/scala-parser-combinators_2.11-1.0.4.jar, /Users/jason/.ivy2/cache/jline/jline/jars/jline-2.12.1.jar)

I don't have a perfect understanding of all of all the moving parts here, but I would tend to be conservative and assume that embeddedDefaults serves a purpose, and that we make it possible to use this from both the new toolbox compiler and from the JSR scripting engine.

@rjolly

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2015

The ScriptEngine and its settings are defined near the bottom of https://github.com/scala/scala/blob/2.11.x/src/repl/scala/tools/nsc/interpreter/IMain.scala so that should not be hard to do. What I don't understand, is that you get from embeddedDefaults what you put in it, right ? I mean, you should have app.class.path and boot.class.path as resources somewhere in your jars/dirs ? I can't find them anywhere in mine. So, not surprisingly, embeddedDefaults has no effect for me, and I can't reproduce your test case.

@retronym

This comment has been minimized.

Copy link
Member

commented Sep 20, 2015

SBT's custom classloader that is used to run user code in-process creates the contents of these files programatically at runtime, there is no corresponding file or JAR entry. It is just a back channel between SBT and the compiler code that populates the classpath.

@retronym

This comment has been minimized.

Copy link
Member

commented Nov 13, 2015

Ping @xeno-by

@xeno-by

This comment has been minimized.

Copy link
Member

commented Nov 16, 2015

Speaking of embeddedDefaults, it's the first time I hear about it (or I've already forgotten everything that I've heard :)), so I don't think I can comment on it.

Speaking of the way how the switch between reflect and noreflect is done, could we just go for autodetection - if the classloader passed in is a URLClassLoader, then automatically switch into the noreflect mode?

@rjolly

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2015

Well, it will be the case in almost all situations (we are interested in), making ReflectGlobal useless. Instead of looking for a cheap way to achieve our goal, I think we should tackle the real problem, which is that reflection can not list the type members of a (package) object. Note that the problem has its roots in Java, whose Package class lacks a getClasses method : http://docs.oracle.com/javase/7/docs/api/java/lang/Package.html . Another solution would be to disallow wilcard imports (was it considered ?)

@xeno-by

This comment has been minimized.

Copy link
Member

commented Nov 24, 2015

To be honest, I don't think that we can tackle the underlying problem at all, because there's no official API available for that. At this point, personally I would be happy with any workaround regardless of its generality, because there's been a significant gap in functionality for quite a long time.

Disallowing relative imports means effectively codifying the state of the art (i.e. "programs without relative imports work, programs without relative imports don't"), ruling out an important class of use cases.

@rjolly

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2015

Are we really sure that enumerating the class path is necessary to resolve relative imports ? What about this algorithm: when encountering an unknown name, append it to every wildcard import, and try to load the resulting path. If one succeeds, then success. If less or more than one, issue an error (not found, or ambiguous, as would be the case with a normal compiler). https://issues.scala-lang.org/browse/SI-6393 says "it has to speculate and provisionally create a package for root.math, which ends up being a wrong choice". We need to know where this choice is made, and replace it with a complete search.

@SethTisue SethTisue modified the milestones: 2.11.9, 2.11.8 Feb 23, 2016

@adriaanm

This comment has been minimized.

Copy link
Member

commented Oct 18, 2016

There's a lot of good stuff in here. Sadly, I don't think we should be changing this stuff in 2.11 any more at this point. I know this has been open for a while -- apologies.

There's a lot of potential for classpath improvements in 2.12/2.13, both for performance and Java 9 compatibility. Perhaps you'd be interested to take this up there?

@adriaanm adriaanm closed this Oct 18, 2016

@SethTisue SethTisue removed this from the 2.11.9 milestone Oct 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.