-
Notifications
You must be signed in to change notification settings - Fork 634
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
Configures Lagom build to default to 2.12 #1175
Conversation
We will need a new version of lightbend-markdown-server and corresponding plugin The sbt plugin has the server version hardcoded to 2.11 and therefore we can't move the documentation project to 2.12. (It took me ages to find out why sbt was insisting in bring |
I just sent a PR to lightbend-markdown-server to fix it. |
@@ -3,7 +3,7 @@ lazy val plugins = (project in file(".")).dependsOn(dev) | |||
lazy val dev = ProjectRef(Path.fileProperty("user.dir").getParentFile, "sbt-plugin") | |||
|
|||
resolvers += Resolver.typesafeIvyRepo("releases") | |||
addSbtPlugin("com.lightbend.markdown" %% "sbt-lightbend-markdown" % "1.6.0") | |||
addSbtPlugin("com.lightbend.markdown" %% "sbt-lightbend-markdown" % "1.6.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending. We need a new release of lightbend-markdown with support for 2.12
Done!
docs/build.sbt
Outdated
|
||
val AkkaVersion = "2.5.8" | ||
val JUnitVersion = "4.11" | ||
val JUnitInterfaceVersion = "0.11" | ||
val ScalaTestVersion = "3.0.4" | ||
val PlayVersion = "2.6.9" | ||
val PlayVersion = "2.6.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will conflict with #1182. Was it needed for this change, or just something you were fixing up because you noticed it was out of sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just bumped the version, not really needed. We can remove to avoid the conflict with #1182
@@ -83,7 +83,7 @@ We also recommend using Maven dependency management in your root project pom to | |||
|
|||
#### A note on Scala versions | |||
|
|||
When adding dependencies to Lagom libraries, you need to ensure that you include the Scala major version in the artifact ID, for example, `lagom-javadsl-api_2.11`. | |||
When adding dependencies to Lagom libraries, you need to ensure that you include the Scala major version in the artifact ID, for example, `lagom-javadsl-api_2.12`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should encourage use of the scala.binary.version
property in the main doc as well as the migration guide (especially since the artifact uses it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that option for the docs, but kind of give up because many fragments don't have the full pom.xml
and a reference to a variable without context could be confusing.
Do you think it will be ok if we add a note recommending the usage of scala.binary.verison
variable and use it everywhere in the docs?
I didn't have that issue before because we were not cross-compiling, but now we do.
@@ -59,7 +69,7 @@ The change to drop support for multiple services will require another code updat | |||
) | |||
``` | |||
|
|||
That is now deprecated and will issue a warning on runtime (unfortunately Scala 2.11 will not cause a compilation warning because of the deprecation). The method replacing `describeServices` is `describeService` (in singular) and it will take an `Option[Descriptor]` instead of a list: | |||
That is now deprecated and will issue a warning on runtime (unfortunately Scala will not cause a compilation warning because of the deprecation). The method replacing `describeServices` is `describeService` (in singular) and it will take an `Option[Descriptor]` instead of a list: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, Scala 2.12 has a deprecatedOverriding
annotation we can use in the future. However, despite the scaladoc saying it is available since 2.10, it was private[scala]
until 2.12, so we can't use it until we drop 2.11 support.
It remains to be seen whether we drop 2.11 support before we drop this deprecated method 😄
The one caveat is that you should set the "Production sources directory" to `target/scala-2.11/src_managed/main` and the "Test source directory" to `target/src_managed/test`, instead of using the ones recommended in the linked instructions. The reason is that there are a couple of issues related to using immutables in IntelliJ with the IntelliJ sbt plugin (specifically, [SCL-8543](https://youtrack.jetbrains.com/issue/SCL-8543) and [IDEA-117540]( https://youtrack.jetbrains.com/issue/IDEA-117540)). | ||
Finally, under "Project Structure" you should undo the exclusion of the `target`-directory and verify whether `target/scala-2.11/src_managed/main` is marked as 'Sources' for each module. | ||
The one caveat is that you should set the "Production sources directory" to `target/scala-2.12/src_managed/main` and the "Test source directory" to `target/src_managed/test`, instead of using the ones recommended in the linked instructions. The reason is that there are a couple of issues related to using immutables in IntelliJ with the IntelliJ sbt plugin (specifically, [SCL-8543](https://youtrack.jetbrains.com/issue/SCL-8543) and [IDEA-117540]( https://youtrack.jetbrains.com/issue/IDEA-117540)). | ||
Finally, under "Project Structure" you should undo the exclusion of the `target`-directory and verify whether `target/scala-2.12/src_managed/main` is marked as 'Sources' for each module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These paths might be misleading to anyone who doesn't migrate to 2.12. I wonder if there's a way to rephrase this to make it clear that you need to use the right directory for the selected Scala version. Maybe we can add a note saying the directories need to be adjusted for Scala 2.11 and link to "A note on Scala versions" in the doc below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding the following:
(or target/scala-2.11/src_managed/main
if using Scala 2.11)
@@ -9,7 +9,7 @@ Consider some of the basic requirements of a Reactive Microservice as identified | |||
|
|||
The following Lagom characteristics promote these best practices: | |||
|
|||
* Lagom is asynchronous by default --- its APIs make inter-service communication via streaming a first-class concept. All Lagom APIs use the asynchronous IO capabilities of [Akka Stream](https://akka.io/) for asynchronous streaming; the Java API uses [JDK8 `CompletionStage`](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletionStage.html) for asynchronous computation; the Scala API uses [Futures](https://www.scala-lang.org/api/2.11.8/#scala.concurrent.Future). | |||
* Lagom is asynchronous by default --- its APIs make inter-service communication via streaming a first-class concept. All Lagom APIs use the asynchronous IO capabilities of [Akka Stream](https://akka.io/) for asynchronous streaming; the Java API uses [JDK8 `CompletionStage`](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletionStage.html) for asynchronous computation; the Scala API uses [Futures](https://www.scala-lang.org/files/archive/api/2.12.4/scala/concurrent/Future.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://www.scala-lang.org/api/2.12.x/scala/concurrent/Future.html might be a slightly more future-proof link (2.12.x instead of 2.12.4) and we use it elsewhere in ServiceImplementation.md
.
There is another open PR that will update it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, but should we also use the property in the other pages where we refer to Maven artifacts? I'm worried that someone might blindly copy and paste one of those POM fragments into a 2.11 project. Maven won't give you a nice warning like sbt does when you mix multiple Scala versions together. You'll just get a cryptic runtime error. If someone pastes in a snippet that refers to a non-existent property, at least they'll see an error that makes sense and would be easy to Google.
I went ahead and added a commit that changes all of the documentation to use |
Java users especially might not be aware of the reasons to update.
That's strange, I remember to have searched for all occurrences of |
Is there lagom-sbt-plguin for 2.12 somewhere? The default project seems not to find the plugin since there's plugin only for version 2.10 available in the repository. |
@Zecklar not yet published, but will be for 1.4.0 final. See #932. |
1.4.x 67b3dda |
The sbt plugin projects are still using 2.10.6 by default. Is this intentional? |
@steinybot yes, they need to build for sbt 0.13, which uses Scala 2.10. |
@TimMoore Is there ETA for 1.4.0 final? Or alternatively unofficial builds for the plugin that I could copy to local repo? |
@Zecklar later this week (or you can build from source) |
Fixes: #1160
Fixes: #1152