Skip to content

Fix classpath in QuoteDriver when executing from sbt #3871

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

Merged
merged 1 commit into from
Jan 22, 2018

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki
Copy link
Contributor Author

Found the bug and fixed it in nicolasstucki/scala-quoted-util@39087d4

@smarter
Copy link
Member

smarter commented Jan 19, 2018

I have no idea what the context is so I can't really review this. I'll just note that you can pass -usejavacp to the compiler so that the Scala classpath contains the Java classpath.

@nicolasstucki
Copy link
Contributor Author

Maybe the -usejavacp solves this issue cleanly. I will try it out.

@nicolasstucki
Copy link
Contributor Author

The -usejavacp did not work, the issue is that in this case the java classpath does not contain the current classpath. The classpath only contains the SBT launcher which then uses a custom classloader to load the classpath that will be used to run the application.

var classpath = System.getProperty("java.class.path")
this.getClass.getClassLoader match {
case cl: URLClassLoader =>
// Loads the classes load by this class loader
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the classes loaded by

this.getClass.getClassLoader match {
case cl: URLClassLoader =>
// Loads the classes load by this class loader
// When executing `run` or `test` in sbt the classpath is not in th property java.class.path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the property

@nicolasstucki nicolasstucki force-pushed the fix-QuoteDriver-classpath branch from 72173d2 to 014164d Compare January 22, 2018 10:41
@nicolasstucki
Copy link
Contributor Author

Rebased and fixed comments

@@ -61,7 +62,15 @@ class QuoteDriver extends Driver {

override def initCtx: Context = {
val ictx = super.initCtx.fresh
val classpath = System.getProperty("java.class.path")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using ctx.settings.classpath instead, isn't that enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the classpath of the library in not in there. That is why I needed to add it explicitly. We also need the current classpath in case the quote refers to something in the application (such as class or static method).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example

class Foo
object Foo {
  def main(...) = {
    val q = '(new Foo)
    q.run // Foo need to be present in the classpath of the compiler that will compile q
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In scala.reflect macros, it assumes separation (ahead-of) compilation of macro definitions. The user-app must depend on the lib that contains Foo in order to use that macro, thus it's automatically on the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. But the app is guaranteed to have separate compilation as the macro is staged.

@smarter
Copy link
Member

smarter commented Jan 22, 2018

I assume macros in scalac have the same problem. We should check how they handle classpaths.

@nicolasstucki nicolasstucki merged commit e3e60b0 into scala:master Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants