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

Upgrade to Scala 2.12 #6691

Merged
merged 9 commits into from
Nov 30, 2016
Merged

Upgrade to Scala 2.12 #6691

merged 9 commits into from
Nov 30, 2016

Conversation

wsargent
Copy link
Member

@wsargent wsargent commented Nov 2, 2016

Sets up a Play version that works against 2.12.0 by default and does a cross-build to 2.11.8.

All the dependencies have been worked through, so if this builds, we should be good.

@@ -16,7 +16,7 @@ logLevel := Level.Warn

scalacOptions ++= Seq("-deprecation", "-language:_")

addSbtPlugin("com.typesafe.play" % "interplay" % sys.props.getOrElse("interplay.version", "1.1.2"))
addSbtPlugin("com.typesafe.play" % "interplay" % sys.props.getOrElse("interplay.version", "1.3.0-SNAPSHOT"))
Copy link
Member

Choose a reason for hiding this comment

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

Was version 1.3.0-SNAPSHOT published? Looks like the version was not changed at playframework/interplay#18.

Current master also shows that the last version still 1.2.1-SNAPSHOT:

https://github.com/playframework/interplay/blob/master/version.sbt

@@ -1,4 +1,4 @@
#
# Copyright (C) 2009-2016 Lightbend Inc. <https://www.lightbend.com>
#
sbt.version=0.13.11
sbt.version=0.13.13
Copy link
Member

Choose a reason for hiding this comment

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

This should also be updated at other files (documentation, templates and subprojects).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll file a milestone issue for upgrading to sbt 0.13.13

@KarelCemus KarelCemus mentioned this pull request Nov 6, 2016
8 tasks
@schmitch
Copy link
Contributor

schmitch commented Nov 9, 2016

actually akka-http and akka-http-core are now released for 2.12 as 10.0.0-RC2.
Also sbt-io is also released: http://dl.bintray.com/typesafe/ivy-releases/org.scala-sbt/io_2.12/0.13.13/

Brings us down to our own dependencies.

@wsargent
Copy link
Member Author

wsargent commented Nov 9, 2016

@schmitch great, I'll see about updating our internal docs...

@wsargent wsargent changed the title WIP - Upgrade to Scala 2.12 Upgrade to Scala 2.12 Nov 10, 2016
@wsargent
Copy link
Member Author

@schmitch okay, akka-http and sbt-io has been updated and play-doc 1.3.0 / interplay 1.7.0 has been released -- if this passes the build I think we're good.

@@ -56,6 +56,14 @@ object BuildSettings {
Resolver.typesafeRepo("releases"),
Resolver.typesafeIvyRepo("releases")
),
scalacOptions in(Compile, doc) := {
Copy link
Contributor

@schmitch schmitch Nov 10, 2016

Choose a reason for hiding this comment

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

maybe we should use ++= instead of := and wrap it with .value, I told you to add it since I didn't found other occurences which might override that.

@schmitch
Copy link
Contributor

schmitch commented Nov 10, 2016

Actually scripted tests will now only run on 2.12 and the normal tests won't actually cross-build as it seems. we need to adjust the travis build file.

Also scripted tests fail, two of them are easy to fix:

Fix for the scripted tests:

diff --git a/framework/src/sbt-plugin/src/sbt-test/routes-compiler-plugin/injected-routes-compilation/app/controllers/Application.scala b/framework/src/sbt-plugin/src/sbt-test/routes-compiler-plugin/injected-routes-compilation/app/controllers/Application.scala
index 5e97254..6e6a816 100644
--- a/framework/src/sbt-plugin/src/sbt-test/routes-compiler-plugin/injected-routes-compilation/app/controllers/Application.scala
+++ b/framework/src/sbt-plugin/src/sbt-test/routes-compiler-plugin/injected-routes-compilation/app/controllers/Application.scala
@@ -4,7 +4,7 @@
 package controllers

 import play.api.mvc._
-import scala.collection.JavaConversions._
+import scala.collection.JavaConverters._
 import javax.inject.Inject

 class Application @Inject() (c: ControllerComponents) extends AbstractController(c) {
@@ -36,7 +36,7 @@ class Application @Inject() (c: ControllerComponents) extends AbstractController
     Ok(`b[]`mkString(",") + " " + `b%%`)
   }
   def takeJavaList(x: java.util.List[Integer]) = Action {
-    Ok(x.mkString(","))
+    Ok(x.asScala.mkString(","))
   }
   def urlcoding(dynamic: String, static: String, query: String) = Action {
     Ok(s"dynamic=$dynamic static=$static query=$query")
diff --git a/framework/src/sbt-plugin/src/sbt-test/routes-compiler-plugin/routes-compilation/app/controllers/Application.scala b/framework/src/sbt-plugin/src/sbt-test/routes-compiler-plugin/routes-compilation/app/controllers/Application.scala
index 40cd0b0..8ce048f 100644
--- a/framework/src/sbt-plugin/src/sbt-test/routes-compiler-plugin/routes-compilation/app/controllers/Application.scala
+++ b/framework/src/sbt-plugin/src/sbt-test/routes-compiler-plugin/routes-compilation/app/controllers/Application.scala
@@ -5,7 +5,7 @@ package controllers

 import play.api.mvc._
 import javax.inject.Inject
-import scala.collection.JavaConversions._
+import scala.collection.JavaConverters._

 class Application @Inject() (c: ControllerComponents) extends AbstractController(c) {
   def index = Action {
@@ -30,7 +30,7 @@ class Application @Inject() (c: ControllerComponents) extends AbstractController
     Ok(x.mkString(","))
   }
   def takeJavaList(x: java.util.List[Integer]) = Action {
-    Ok(x.mkString(","))
+    Ok(x.asScala.mkString(","))
   }
   def urlcoding(dynamic: String, static: String, query: String) = Action {
     Ok(s"dynamic=$dynamic static=$static query=$query")

after that we get a really strange failure of akka-http-server:
[error] (Play-Akka-Http-Server-Experimental/*:scripted) java.lang.NoSuchMethodError: scala.Predef$.refArrayOps([Ljava/lang/Object;)Lscala/collection/mutable/ArrayOps;
with a stack trace that is hard to guess, where the actual error really is.

@wsargent
Copy link
Member Author

@schmitch I suspect that is happening from

https://github.com/sbt/sbt/blob/0.13.13_2.12cross/scripted/sbt/src/main/scala/sbt/test/ScriptedTests.scala#L180

@wsargent
Copy link
Member Author

More update: when I run bin/testSbtPlugins, I run into https://issues.scala-lang.org/browse/SI-10027 (closed in 2.12.1)

[warn] /home/wsargent/work/playframework/upgrade-2.12/framework/src/play-docs/src/main/scala/play/docs/DocumentationHandler.scala:30: constructor PlayDoc in class PlayDoc is deprecated (since 1.3.0): Use the primary constructor
[warn]     new PlayDoc(repo, repo, "resources", PlayVersion.current, PageIndex.parseFrom(repo, "Home", Some("manual")), "Next")
[warn]     ^
[warn] one warning found
java.lang.StackOverflowError
    at scala.tools.nsc.util.JavaCharArrayReader.next(JavaCharArrayReader.scala:28)
    at scala.tools.nsc.javac.JavaScanners$JavaScanner.putCommentChar(JavaScanners.scala:580)
    at scala.tools.nsc.javac.JavaScanners$JavaScanner.skipBlockComment(JavaScanners.scala:585)

@schmitch
Copy link
Contributor

yeah but actually that can be fixed with higher JAVA_OPTS, i use:

export JAVA_OPTS="-Xmx4G -Xss2M -XX:MaxMetaspaceSize=512M -XX:ReservedCodeCacheSize=512M -Dfile.encoding=UTF-8"

@wsargent
Copy link
Member Author

@schmitch we only have 3GB on the Travis CI box, but we can start there...

@wsargent
Copy link
Member Author

The scripted error seems to be happening because of the sbt mediator. There's a workaround: sbt/sbt#2786 (comment)

There are a couple of scaladoc errors resulting from scaladoc complaining that private Java classes are escaping their boundaries -- they've been made package private.

The stackoverflow https://issues.scala-lang.org/browse/SI-10027 only occurs in Play-Java, so I've pointed Play-Java project to the Scala 2.12 nightly build just to get something working.

@wsargent wsargent force-pushed the upgrade-2.12 branch 2 times, most recently from c3bf287 to 985e398 Compare November 11, 2016 01:01
@wsargent
Copy link
Member Author

wsargent commented Nov 11, 2016

Well, I can get most of the scripted tests working with MediatorWorkaround, but the Akka-Http-Server tests are still failing regardless:

> project Play-Akka-Http-Server-Experimental
> scripted
[info] Wrote /home/wsargent/work/playframework/upgrade-2.12/framework/src/play-akka-http-server/target/scala-2.12/play-akka-http-server-experimental_2.12-2.6.0-SNAPSHOT.pom
[info] :: delivering :: com.typesafe.play#play-akka-http-server-experimental_2.12;2.6.0-SNAPSHOT :: 2.6.0-SNAPSHOT :: integration :: Thu Nov 10 18:37:27 PST 2016
[info]  delivering ivy file to /home/wsargent/work/playframework/upgrade-2.12/framework/src/play-akka-http-server/target/scala-2.12/ivy-2.6.0-SNAPSHOT.xml
[info] Formatting 1 Scala source {file:/home/wsargent/work/playframework/upgrade-2.12/framework/}Play(compile) ...
[info] Reformatted 1 Scala source {file:/home/wsargent/work/playframework/upgrade-2.12/framework/}Play(compile).
[info] Packaging /home/wsargent/work/playframework/upgrade-2.12/framework/src/play-akka-http-server/target/scala-2.12/play-akka-http-server-experimental_2.12-2.6.0-SNAPSHOT.jar ...
[info] Done packaging.
[info]  published play-akka-http-server-experimental_2.12 to /home/wsargent/.ivy2/local/com.typesafe.play/play-akka-http-server-experimental_2.12/2.6.0-SNAPSHOT/poms/play-akka-http-server-experimental_2.12.pom
[info]  published play-akka-http-server-experimental_2.12 to /home/wsargent/.ivy2/local/com.typesafe.play/play-akka-http-server-experimental_2.12/2.6.0-SNAPSHOT/jars/play-akka-http-server-experimental_2.12.jar
[info]  published ivy to /home/wsargent/.ivy2/local/com.typesafe.play/play-akka-http-server-experimental_2.12/2.6.0-SNAPSHOT/ivys/ivy.xml
[trace] Stack trace suppressed: run last Play-Akka-Http-Server-Experimental/*:scripted for the full output.
[error] (Play-Akka-Http-Server-Experimental/*:scripted) java.lang.NoSuchMethodError: scala.Predef$.refArrayOps([Ljava/lang/Object;)Lscala/collection/mutable/ArrayOps;
[error] Total time: 0 s, completed Nov 10, 2016 6:37:27 PM

https://github.com/wsargent/playframework/blob/upgrade-2.12/framework/src/play-akka-http-server/src/sbt-test/akka-http/play-akka-http-plugin/project/MediatorWorkaround.scala

@schmitch
Copy link
Contributor

Might be a different error?

@gmethvin
Copy link
Member

What's the status of this PR? Are we ready to merge soon?

Also it looks to me we could backport to 2.5.x as well (perhaps with different dependency versions for Scala 2.12).

@wsargent
Copy link
Member Author

wsargent commented Nov 28, 2016

There's a couple of tests involving Akka-HTTP that don't resolve and have been disabled. Otherwise, it's good for review.

@@ -104,7 +104,7 @@ lazy val PlayAkkaHttpServerProject = PlayCrossBuiltProject("Play-Akka-Http-Serve
.settings(libraryDependencies ++= akkaHttp)
// Include scripted tests here as well as in the SBT Plugin, because we
// don't want the SBT Plugin to have a dependency on an experimental module.
.settings(playFullScriptedSettings: _*)
//.settings(ScriptedPlugin.scriptedSettings ++ playScriptedSettings)
Copy link
Member Author

Choose a reason for hiding this comment

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

Commented out because the sbt-mediator throws exception here even if there are no sbt scripted tests...

@@ -122,7 +122,8 @@ trait Action[A] extends EssentialAction {
* @tparam A the body content type
*/
trait BodyParser[+A] extends (RequestHeader => Accumulator[ByteString, Either[Result, A]]) {
self =>
// "with Any" because we need to prevent 2.12 SAM inference here
self: BodyParser[A] with Any =>
Copy link
Member Author

Choose a reason for hiding this comment

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

Scala 2.12 looks at Function types and attempts to convert them to JDK 1.8 SAM types -- since BodyParser extends Function1, it ends up conflicting with the other Action.apply method that takes only a (Request[A] => Result) (and therefore is also a SAM type, but does not share a common base type).

The solution here is to ensure the BodyParser cannot be treated as a SAM type by the compiler, by kludging the explicit self type reference.

@@ -25,7 +25,7 @@ play.http.filters = "filters.MyFilters"

## Configuring the security headers

Scaladoc is available in the [play.filters.headers](api/scala/play/filters/headers/package.html) package.
Scaladoc is available in the [play.filters.headers](api/scala/play/filters/headers/) package.
Copy link
Member Author

Choose a reason for hiding this comment

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

2.12 doesn't believe in package.html

GzipFilter gzipFilter = new GzipFilter(
new GzipFilterConfig().withShouldGzip((req, res) ->
gzipFilterConfig.withShouldGzip((BiFunction<Http.RequestHeader, Result, Object>) (req, res) ->
Copy link
Member Author

@wsargent wsargent Nov 29, 2016

Choose a reason for hiding this comment

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

GzipFilterConfig has two withShouldGzip, both of which equate to SAM types. The 2.12 compiler can't distinguish, so we need an explicit cast here.

It might be a better idea to remove one of these methods or move the methods out to different classes altogether for Java API / Scala API.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's a shame we have to support Scala 2.11 too.

Maybe for Play 2.6 we should rename the Java one to something like withShouldGzipJava, then we can remove it when we drop support for 2.11. Most Java users should be able to upgrade to Scala 2.12 pretty easily so they'll be able to use the Scala function version.

Copy link
Contributor

Choose a reason for hiding this comment

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

what about creating two source folders one for 2.11 and one for 2.12 and keep the old version as is and the new version for 2.12 won't have two functions?

@wsargent wsargent merged commit c9d2b33 into playframework:master Nov 30, 2016
@wsargent wsargent deleted the upgrade-2.12 branch November 30, 2016 00:58
@gmethvin
Copy link
Member

💯 👍 😄

@ghost
Copy link

ghost commented Nov 30, 2016

Is this going to be released soon? Would be totally awesome! :)

@rethab
Copy link
Contributor

rethab commented Dec 8, 2016

@gmethvin is the backport going to happen? Or, what's holding it back?

@wsargent
Copy link
Member Author

wsargent commented Dec 8, 2016

@RookieMaus Play 2.6.x is scheduled for Q1 2017 -- see https://docs.google.com/document/d/11sVi1-REAIDFVHvwBrfRt1uXkBzROHQYgmcZNGJtDnA/edit#

@rethab Getting 2.6.x out the door is the priority now.

@jroper
Copy link
Member

jroper commented Feb 9, 2017

With regards to backporting to 2.5, I can't see anything about this PR that is not binary compatible, or can't be made to be binary compatible.

The only change to public API is that BodyParser now has an Any self type. This has no impact on binary compatibility as self types are erased at runtime and only read by the compiler, and since Scala 2.11 doesn't ever generate lambdas, it should have zero impact on source compatibility.

There are several upgrades to libraries that may be binary incompatible, but that's easy to deal with in sbt where you can control the version pulled in based on the Scala version. There don't seem to be any changes that are dependent on those upgrades.

Nearly all the changes in this PR are related to testing and docs, and so should be innocuous to backport.

@schmitch
Copy link
Contributor

Well to bring 2.12 to 2.5 we would eat least need to have a version of twirl and play-doc that is binary compatible, or is there a way to change sbt plugins based on scala version?
Another problem might be to actually have a 2.12 build that works around fork run, but I guess it's possible to disable it for 2.12

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.

None yet

6 participants