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

(wip) Create a Play! module to compile the router #421

Closed
wants to merge 5 commits into from

Conversation

ggrossetie
Copy link
Contributor

@ggrossetie ggrossetie commented Aug 30, 2018

🚧 Work In Progress

I've moved the TwirlCompiler and the RouterCompiler on the newly created contrib.playlib module. Currently I'm stuck with a classpath/reflection issue.

The following call throws an exception:

val cl = new URLClassLoader(routerClasspath.map(_.toIO.toURI.toURL).toArray, null)
val routesCompilerTaskClass = cl.loadClass("play.routes.compiler.RoutesCompiler$RoutesCompilerTask")
val routerCompilerTaskConstructor = routesCompilerTaskClass.getConstructor(
  classOf[File],
  cl.loadClass("scala.collection.Seq"),
  classOf[Boolean],
  classOf[Boolean],
  classOf[Boolean])

val args = Array[AnyRef](task.file: java.io.File,
  task.additionalImports: scala.collection.Seq[String],
  Boolean.box(true),
  Boolean.box(true),
  Boolean.box(false))

val routesCompilerTaskInstance = routerCompilerTaskConstructor
  .newInstance(args: _*)
  .asInstanceOf[Object] // IllegalArgumentException is thrown!
java.lang.IllegalArgumentException: argument type mismatch
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)

The goal of the code is to instantiate the case class RoutesCompilerTask : https://github.com/playframework/playframework/blob/73284a8ea6001c80570dafb804a0ec99a7e7a6f2/framework/src/routes-compiler/src/main/scala/play/routes/compiler/RoutesCompiler.scala#L75

This issue is pretty hard to debug because the exception is not thrown when I'm using Intellij IDEA to debug 😞

Ref: #324

@lihaoyi
Copy link
Member

lihaoyi commented Aug 31, 2018

Your problem is that the scala.collection.Seq you are passing in is a different scala.collection.Seq than the URLClassLoader expects. As the classloader is isolated (parent is null), it does not share any classes with the Mill classloader, and thus both classloaders have different copies of scala.collection.Seq which are not compatible.

The solution to this is to construct the scala.collection.Seq to pass in using reflection on the cl.loadClass("scala.collection.Seq") Class[_] object, calling methods and using Java arrays to construct the cl's Seq since only java standard library classes will be shared between the cl and Mill classloaders

@lihaoyi
Copy link
Member

lihaoyi commented Aug 31, 2018

Something like

cl.loadClass("scala.collection.mutable.WrappedArray$ofRef")
.getConstructors()(0)
.newInstance(task.additionalImports.toArray)

@ggrossetie
Copy link
Contributor Author

Thanks for your help @lihaoyi
You were 100% correct. I also had to "convert" the object returned by the RouterCompiler.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 1, 2018

I'd rather we keep twirl and play-routes separate for now. Twirl is used in a lot more places than Play: people using Cask, Scalatra, Unfiltered, Akka-http, etc. all can (and do) make use of Twirl.

Perhaps those that want the "full" play experience can just import $ivy both mill-contrib-twirllib and mill-contrib-playroutes

@ggrossetie
Copy link
Contributor Author

I'd rather we keep twirl and play-routes separate for now. Twirl is used in a lot more places than Play: people using Cask, Scalatra, Unfiltered, Akka-http, etc. all can (and do) make use of Twirl.

Fair enough 👍

Perhaps those that want the "full" play experience can just import $ivy both mill-contrib-twirllib and mill-contrib-playroutes

I don't think that the Play! router makes sense outside of a Play! project, so maybe we should extract Twirl to a dedicated module but keep the Play! router in the playlib module ?

Will it be possible to use the TwirlCompiler in the Play! module ? Since a Play! application has a well defined directory structure it should be easy to provide a good user experience:

// build.sc
import $ivy.`com.lihaoyi::mill-contrib-twirllib`
import $ivy.`com.lihaoyi::mill-contrib-playlib`
import mill._, scalalib._, twirllib._, playlib._

object foo extends PlayModule {
  def scalaVersion = "2.12.4"
  def playVersion = "2.6.18"
  def twirlVersion = "1.3.15" // could be resolved from playVersion
  def ivyDeps = Agg(
    "com.typesafe.play" %% "play-ws-standalone" % "1.1.10",
    "com.typesafe.play" %% "play-mailer" % "6.0.0"
  )
}

@Ammonite-Bot
Copy link
Collaborator

Ammonite-Bot commented Sep 1, 2018 via email

Right(filesJava.asScala)
case "scala.util.Left" =>
// TODO: convert the error of type RoutesCompilationError to a CompilationResult
Left(Seq(CompilationResult(Path(""), PathRef(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.

I'm not quite sure about the return type... Do we want to return a CompilationResult ?

Copy link
Member

Choose a reason for hiding this comment

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

If it failed, we can either return a Option, Either, or mill.eval.Result.Error/mill.eval.Result.Exception. I don't care much either way, except returning an invalid CompilationResult is probably unacceptable since anyone trying to use it will find garbage inside

@lihaoyi lihaoyi force-pushed the master branch 4 times, most recently from b30fad6 to c49e1b1 Compare November 6, 2018 09:40
@SoerenSilkjaer
Copy link

Are anyone still working on this pr?

@ggrossetie
Copy link
Contributor Author

@sorenvalentinjp Sorry I don't have time right now to work on it but you can take my work and fix the remaining issues pointed out by @lihaoyi

@lihaoyi
Copy link
Member

lihaoyi commented Dec 15, 2018

closing due to inactivity

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