Add a TwirlModule to compile Twirl templates #271

ggrossetie · 2018-04-01T16:40:14Z

Based on @pleira implementation

This is a work in progress:

Find a better solution to solve the java.lang.ClassNotFoundException: scala.util.parsing.input.Positional
An exception is thrown when TwirlCompiler.compile function is invoked using Reflection
Unable to call the compile method with a primitive boolean using reflection: https://github.com/playframework/twirl/blob/f4ed321dee30c977b7f303ef1229169180beab45/compiler/src/main/java/play/japi/twirl/compiler/TwirlCompiler.java#L38
Find a way to use the Scala API because the Java API is only available in Twirl 1.3+

For now the main issue is that an exception is thrown when TwirlCompiler.compile is invoked.
Oddly tests are running fine in Intellij IDEA. In this case the reflection is calling JavaMirrors.JavaVanillaMethodMirror.apply(args: Any*). But when I run the tests using mill twirllib.test, the reflection is calling JavaMirrors.JavaTransformingMethodMirror.apply(args: Any*).

I've also tried to use Java reflection but without success:

val clt = cl.loadClass("play.twirl.compiler.TwirlCompiler")
val module = cl.loadClass("play.twirl.compiler.TwirlCompiler$").getField("MODULE$")
val compileMethod = clt.getMethods.find(_.getName == "compile").get
compileMethod.invoke(module, 
    source,
    sourceDirectory,
    generatedDirectory,
    formatterType,
    additionalImports,
    constructorAnnotations,
    codec,
    inclusiveDot) // error: the result type of an implicit conversion must be more specific than AnyRef

Help, feedback or review will be greatly appreciated 😄

ggrossetie · 2018-04-01T16:41:28Z

main/test/src/mill/define/CacherTests.scala

@@ -17,7 +17,7 @@ object CacherTests extends TestSuite{
  }
  object Middle extends Middle
  trait Middle extends Base{
-    def value = T{ super.value() + 2}
+    override def value = T{ super.value() + 2}


Intellij IDEA is complaining about this line (preventing me from running the tests) but Mill can compile the code just fine 😐

Yep, that's fine in mill, because there is a compiler plugin that makes override word optional, but only in build files

Thanks for the explanation.
Do you know if there is a way to configure intellij IDEA in order to use this compiler plugin for this file ?

Technically it should be possible, If I understand correctly, you need to include http://search.maven.org/#artifactdetails%7Ccom.lihaoyi%7Cmill-moduledefs_2.12%7C0.1.7%7Cjar in classpath and add "-P:auto-override-plugin". to scalac options.

rockjam · 2018-04-01T18:25:29Z

@Mogztter I'll take a look at those ClassNotFoundException issues when time permits. It's not clear what's wrong from first look

ggrossetie · 2018-04-01T21:04:38Z

I'll take a look at those ClassNotFoundException issues when time permits. It's not clear what's wrong from first look

@rockjam Ok. but I'm no longer sure that this issue can be reproduced with mill twirllib.test maybe it's an issue with the Intellij IDEA tests' classpath. I will do some more testing tomorrow 😉

EDIT: I can't reproduce this issue when running the tests with mill twirllib.test so definitely an issue with IDEA classpath.

@rockjam is it possible to do remote debugging ? I've tried JAVA_OPTS="-Xdebug -Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=1044" mill twirllib.test but it's not working (my breakpoints are ignored)

ggrossetie · 2018-04-02T09:50:11Z

I think the reflection is struggling with the parameter type Seq[String] = Nil. To workaround this issue, I'm now using the Java API (available in Twirl 1.3+):

https://github.com/playframework/twirl/blob/f4ed321dee30c977b7f303ef1229169180beab45/compiler/src/main/java/play/japi/twirl/compiler/TwirlCompiler.java#L31

Though, I'm unable to call the compile method with a primitive boolean using reflection, so codec and inclusiveDot parameters will not be available.

rockjam · 2018-04-02T09:50:14Z

is it possible to do remote debugging ?

Honestly, I didn't try. You can try
JAVA_OPTS="-Xdebug -Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=1044" mill -i twirllib.testLocal

ggrossetie · 2018-04-03T15:44:51Z

@rockjam I've created a simple project where I'm using Java reflection to call the Twirl compiler and it's working fine: https://github.com/Mogztter/mill-twirl-compiler/blob/master/app/src/Main.scala

If I'm using the same code inside the TwirlWorker, the following exception is thrown: java.lang.IllegalArgumentException: argument type mismatch.

I've checked both classpath and I didn't see any difference.
Please not that I had to add the following dependency scala-parser-combinators_2.12-1.1.0.jar in https://github.com/Mogztter/mill-twirl-compiler/blob/master/build.sc#L10

Do you reproduce this issue ? I must be doing something wrong but I can't see what... thanks again for your help 👍

rockjam · 2018-04-04T21:19:50Z

@Mogztter I got one question, doesn't twirl comes with some kind of CLI, or main class? In this case, we could just run it as a subprocess

ggrossetie · 2018-04-05T23:33:18Z

Indeed it will make the integration easier but I don't think there is a CLI.
The Twirl source code is here: https://github.com/playframework/twirl

I'm really clueless about this exception since I can't reproduce it outside Mill.

rockjam · 2018-04-05T23:38:33Z

I'll try code from your PR to figure out what's going on with it in next couple days

ggrossetie · 2018-04-22T19:41:55Z

@rockjam Did you have a chance to reproduce the issue ?

ggrossetie · 2018-04-26T18:08:00Z

Rebased on master

rockjam · 2018-04-26T21:16:23Z

@Mogztter sorry, didn't have a chance to look at it yet. I'll try to take a look this weekend. Is it ok for you?

ggrossetie · 2018-04-26T22:51:04Z

Sure thanks for taking the time.
In the mean time I will fix the build failure 😀

rockjam · 2018-04-30T15:47:38Z

twirllib/src/mill/twirllib/TwirlModule.scala

+  }
+}
+
+trait TestTwirlModule extends TwirlModule with TestModule {


Not sure that this trait would be useful

rockjam · 2018-04-30T15:51:35Z

twirllib/test/src/mill/twirllib/HelloWorldTests.scala

+      }
+    }
+    'compileTwirl - {
+      'fromScratch - workspaceTest(HelloWorld) { eval =>


fromScratch adds one more level of nesting for single test. How about unwrapping it, and leaving 'compileTwirl -workspaceTest(HelloWorld) {...} ?

rockjam · 2018-04-30T15:53:20Z

twirllib/test/src/mill/twirllib/HelloWorldTests.scala

+        assert(
+          result.classes.path == eval.outPath / 'core / 'compileTwirl / 'dest / 'html,
+          outputFiles.nonEmpty,
+          outputFiles.forall(expectedClassfiles.contains),


compileClassfiles has only a single element. I guess we could check for a single file in tests too.

rockjam · 2018-04-30T16:35:11Z

@Mogztter I couldn't reproduce theClassNotFoundException issue. I left some comments regarding PR itself. Apart from that it looks fine

ggrossetie · 2018-04-30T17:17:25Z

@rockjam Thanks for your review!

I couldn't reproduce theClassNotFoundException issue.

You were able to call the Scala API https://github.com/playframework/twirl/blob/881b41e58350320570e6d3786cca87db5d04ecb4/compiler/src/main/scala/play/twirl/compiler/TwirlCompiler.scala#L167 ?

I left some comments regarding PR itself. Apart from that it looks fine

Ok I will fix them 👍

rockjam · 2018-04-30T18:58:45Z

@Mogztter I actually did succeed with scala API of twirl. Here is a commit that uses scala API, but passes default values(from reflected TwirlCompiler class) of parameters to compile method. You can take a look here: rockjam@0daf9ca

I also included scala parser-combinator library in classpath, since otherwise it failed. with ClassNotFoundException

rockjam · 2018-04-30T19:11:12Z

@Mogztter and here is the second take that removes reflective calls to default methods. But I'm not sure that @lihaoyi will aprove such tricks with classloading, since we mix mill's classloader with classloader of twirl, and we may go into trouble when mill and twirl will have different scala versions. rockjam@dc546f6

ggrossetie · 2018-05-01T18:29:56Z

I actually did succeed with scala API of twirl. Here is a commit that uses scala API, but passes default values(from reflected TwirlCompiler class) of parameters to compile method. You can take a look here: rockjam/mill@0daf9ca

Oh I see... 🤔
Is it possible to invoke the "default value function" with the method parameter ? I mean if additionalImports or constructorAnnotations is defined in the TwirlWorker, I want these parameters to be passed to the TwirlCompiler API.

I also included scala parser-combinator library in classpath, since otherwise it failed. with ClassNotFoundException

So it's not just me 😌

nd here is the second take that removes reflective calls to default methods. But I'm not sure that @lihaoyi will aprove such tricks with classloading, since we mix mill's classloader with classloader of twirl, and we may go into trouble when mill and twirl will have different scala versions. rockjam/mill@dc546f6

Indeed, we should not mess with the classloader 😉
I think the first solution is good as long as we can pass method parameters to the TwirlCompiler API, namely:

additionalImports
constructorAnnotations
codec
inclusiveDot

rockjam · 2018-05-01T18:43:57Z

@Mogztter Well, to pass additionalImports, constructorAnnotations and codec you have to invoke constructor of types these values have via runtime reflection in twirl's classloader, I guess

ggrossetie · 2018-05-06T22:38:12Z

Sorry I've been busy lately, I will try to go back to this pull request next week.
The next step will be to try to compile a simple Play! application with a Twirl template (to make sure that everything is working fine together) 😄

rockjam · 2018-05-06T22:50:34Z

Sounds great. We could add a sample play app in our integration test suite after this PR gets merged

ggrossetie · 2018-05-11T16:29:54Z

Sounds great. We could add a sample play app in our integration test suite after this PR gets merged

That would be awesome 👍
But I think we should do it first because we will for sure find some limitations with the current implementation.

We can use this template https://github.com/playframework/play-scala-seed.g8 ?

rockjam · 2018-05-11T16:46:28Z

I guess we could. Or we could port some real play application, built with mill. But let's do it in separate PR

ggrossetie · 2018-05-11T16:57:10Z

Ok 👌

I've integrated your commit and rebased on master so I think we are ready.
I can squash the commits if you want ?

One last question about the API, do you think we should keep the twirl prefix ?

Or we could port some real play application, built with mill

Yes I guess we could. A popular Play application on GitHub is Lichess: https://github.com/ornicar/lila

But it will be quite challenging to build it with Mill 😋

lihaoyi · 2018-05-11T17:17:06Z

No need to squash, Github can squash during the merge through the UI whenever @rockjam is ready.

Lichess seems like a bit ambitious, probably should start with something smaller like Hello World =P

rockjam · 2018-05-12T19:28:19Z

One last question about the API, do you think we should keep the twirl prefix ?

Do you mean twirl prefix in package names? If yes, lets keep it.

I will take a look at this PR this weekend

ggrossetie · 2018-05-12T21:54:41Z

@rockjam No I meant the prefix in function names:

twirlVersion -> version
twirlSources -> sources
twirlClasspath -> classpath
compileTwirl -> compile

Also I noticed that mill dev.assembly does not contain TwirlModule, I guess we need to add twirllib here https://github.com/lihaoyi/mill/blob/032ae8b5138863c1523cf34a612ba1b19870713b/build.sc#L294

I will take a look at this PR this weekend

Thanks 👍

Ammonite-Bot · 2018-05-12T22:17:41Z

Keeping the `twirl` prefix seems like a reasonable thing to do; that will avoids ambiguity when you have N different versions that a user may care about (scalaversion, scalafmt version, scalajs version, ...)

…

On Sat, May 12, 2018 at 2:54 PM, Guillaume Grossetie < ***@***.***> wrote: @rockjam <https://github.com/rockjam> No I meant the prefix in function names: - twirlVersion -> version - twirlSources -> sources - twirlClasspath -> classpath - compileTwirl -> compile Also I noticed that mill dev.assembly does not contain TwirlModule, I guess we need to add twirllib here https://github.com/lihaoyi/mill/blob/ 032ae8b/build.sc#L294 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#271 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATpwjL339odqusT_DKxKwrB87TMgoV_2ks5tx1okgaJpZM4TC4_z> .

ggrossetie · 2018-05-13T19:02:28Z

Ok should we rename compileTwirl to twirlCompile ?

Lichess seems like a bit ambitious, probably should start with something smaller like Hello World =P

I agree.
We should probably also need to implement a routes compiler : https://github.com/playframework/playframework/blob/master/framework/src/sbt-plugin/src/main/scala/play/sbt/routes/RoutesCompiler.scala

rockjam · 2018-05-14T21:48:56Z

@Mogztter created separate issue for play routes compiler: #324

Ok should we rename compileTwirl tout twirlCompile ?

I don't have strong preference. compileTwirl sounds fine

rockjam · 2018-05-14T21:56:34Z

twirllib/src/mill/twirllib/TwirlWorker.scala

+              sourceDirectory,
+              generatedDirectory,
+              formatterType,
+              defaultAdditionalImportsMethod.invoke(additionalImports),


How does it work, can you explain a little bit? Why do we invoke method with additionalImports parameter?

I will add a test case.

The basic idea is to be able to override/define the default argument by invoking the method.
So instead of using Nil the Twirl compiler can be configured with additional imports. But maybe that's not how to do it ?

@rockjam Good catch! It's not working 😞
defaultAdditionalImportsMethod is a method that does not take any argument and the call is wrong (ie. the first argument must be the object):

defaultAdditionalImportsMethod.invoke(null, additionalImports)

If I replace defaultAdditionalImportsMethod.invoke(null) with additionalImports then the reflection does not work anymore and the following exception is thrown:

java.lang.IllegalArgumentException: argument type mismatch

Back to square one I guess 😢
So it's working but we cannot define/override additionalImports, constructorAnnotations, codec, or inclusiveDot.

Interestingly, additionalImports is an instance of scala.collection.immutable.$colon$colon (maybe that's why the reflection is confused ?)

We can keep the defaults for these arguments for now, and merge this PR like that. And we can address modifying of these arguments in subsequent PR. How about that?

Ok, I will revert my changes to invoke the method without argument

@rockjam Done! I've created (private) functions in TwirlModule and add a comment so when we know how to override these arguments, everything will be ready.

ggrossetie · 2018-05-17T08:24:54Z

@rockjam Thanks for creating the issue.

rockjam · 2018-05-23T13:08:54Z

@Mogztter travis fails for some reason. Can you rebase it on master to trigger CI?

* Add tests * Update the code to comply with the new API * Use reflection to call TwirlCompiler.compile function

…in the future)

ggrossetie · 2018-05-23T18:20:32Z

@rockjam Done!

rockjam · 2018-05-24T10:19:33Z

@Mogztter thank you

ggrossetie commented Apr 1, 2018

View reviewed changes

ggrossetie force-pushed the twirllib branch from 40b085a to 2fef9f0 Compare April 2, 2018 09:48

lihaoyi force-pushed the master branch 2 times, most recently from 9ebd19b to 779e453 Compare April 6, 2018 22:55

ggrossetie force-pushed the twirllib branch 2 times, most recently from f277471 to 896ed8a Compare April 26, 2018 18:07

ggrossetie force-pushed the twirllib branch from 896ed8a to 4d39299 Compare April 26, 2018 23:00

rockjam reviewed Apr 30, 2018

View reviewed changes

ggrossetie force-pushed the twirllib branch from deff656 to e6088b8 Compare May 11, 2018 15:57

ggrossetie changed the title ~~WIP: Add a TwirlModule to compile Twirl templates~~ Add a TwirlModule to compile Twirl templates May 12, 2018

rockjam reviewed May 14, 2018

View reviewed changes

pleira and others added 9 commits May 23, 2018 20:19

initial implementation

d57ab74

Upgrade to the latest version

8d3a915

* Add tests * Update the code to comply with the new API * Use reflection to call TwirlCompiler.compile function

Run twirllib.test on CI

82c534c

Use the Java API as a workaround

eaba1a3

wip

69c0800

Cleanup the code (code review)

69469c9

Add an example to call the Scala API

f3ac2f4

twirl that works with scala API

95b9fe7

Create functions to override the default settings (will be available …

45ef3c4

…in the future)

ggrossetie force-pushed the twirllib branch from 94f48a1 to 45ef3c4 Compare May 23, 2018 18:20

rockjam merged commit 02436b4 into com-lihaoyi:master May 24, 2018

lefou added this to the 0.2.3 milestone May 2, 2019

Add a TwirlModule to compile Twirl templates #271

Add a TwirlModule to compile Twirl templates #271

Conversation

ggrossetie commented Apr 1, 2018 • edited

Choose a reason for hiding this comment

rockjam Apr 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockjam commented Apr 1, 2018

ggrossetie commented Apr 1, 2018 • edited

ggrossetie commented Apr 2, 2018 • edited

rockjam commented Apr 2, 2018

ggrossetie commented Apr 3, 2018

rockjam commented Apr 4, 2018

ggrossetie commented Apr 5, 2018

rockjam commented Apr 5, 2018

ggrossetie commented Apr 22, 2018

ggrossetie commented Apr 26, 2018

rockjam commented Apr 26, 2018

ggrossetie commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockjam commented Apr 30, 2018

ggrossetie commented Apr 30, 2018

rockjam commented Apr 30, 2018 • edited

rockjam commented Apr 30, 2018

ggrossetie commented May 1, 2018

rockjam commented May 1, 2018

ggrossetie commented May 6, 2018

rockjam commented May 6, 2018

ggrossetie commented May 11, 2018

rockjam commented May 11, 2018

ggrossetie commented May 11, 2018

lihaoyi commented May 11, 2018

rockjam commented May 12, 2018

ggrossetie commented May 12, 2018 • edited

Ammonite-Bot commented May 12, 2018 via email

ggrossetie commented May 13, 2018 • edited

rockjam commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggrossetie May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggrossetie commented May 17, 2018

rockjam commented May 23, 2018

ggrossetie commented May 23, 2018

rockjam commented May 24, 2018

ggrossetie commented Apr 1, 2018 •

edited

rockjam Apr 1, 2018 •

edited

ggrossetie commented Apr 1, 2018 •

edited

ggrossetie commented Apr 2, 2018 •

edited

rockjam commented Apr 30, 2018 •

edited

ggrossetie commented May 12, 2018 •

edited

ggrossetie commented May 13, 2018 •

edited

ggrossetie May 21, 2018 •

edited