-
Notifications
You must be signed in to change notification settings - Fork 277
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
Update Scala to 2.13 #1522
Update Scala to 2.13 #1522
Conversation
scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala
Outdated
Show resolved
Hide resolved
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.
Thank you for looking into this! Just a few minor comments, otherwise looks great!
build.sbt
Outdated
@@ -3,6 +3,7 @@ import sbtcrossproject.CrossPlugin.autoImport.crossProject | |||
|
|||
def scala211 = "2.11.12" | |||
def scala212 = "2.12.8" | |||
def scala213 = "2.13.0" |
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.
def scala213 = "2.13.0" | |
def scala213 = "2.13.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.
Unpleasant discovery: scalafmt should have similar Scala version with coursier-interface for assembly
working 😒
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.
Can you elaborate? I thought scala-library was shaded with coursier-interface
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.
The problem in reflect.properties
file (#1514 (comment)). coursier-interface jar contains file with content:
#Fri Jun 07 10:55:45 UTC 2019
shell.banner=%n ________ ___ / / ___%n / __/ __// _ | / / / _ |%n __\\ \\/ /__/ __ |/ /__/ __ |%n /____/\\___/_/ |_/____/_/ | |%n |/ %s
copyright.string=Copyright 2002-2019, LAMP/EPFL and Lightbend, Inc.
version.number=2.13.0
osgi.version.number=2.13.0.v20190604-151517-VFINAL-43e040f
maven.version.number=2.13.0
But I hope that we can add sbt-assembly rule to discard scala-reflect from coursier-interface.
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.
It should be fine to discard the reflect.properties file from coursier-interface, unless the code from that scala-library somehow relies on APIs like scala.util.Properties.versionNumberString
.
scalafmt-cli/src/main/scala/org/scalafmt/cli/ScalafmtCoreRunner.scala
Outdated
Show resolved
Hide resolved
scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Router.scala
Outdated
Show resolved
Hide resolved
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.
Awesome! Great work @poslegm 🎉
I left a minor comment, but others look good.
What do you think about scalatags, which introduced scala 2.13 support and dropped 2.11 support in 0.7.0?
Line 154 in fea0efc
"com.lihaoyi" %% "scalatags" % "0.6.8", |
This library is used only here for writing rich test report like this.
Though this would be helpful for scalafmt's optimization, maybe we can drop this feature for now. If we need this report later, we can revive this report somehow (by writing raw HTML or using another template engine).
scalafmt-cli/src/main/scala/org/scalafmt/cli/ScalafmtCoreRunner.scala
Outdated
Show resolved
Hide resolved
The trick to handle older versions of scalatags is to add the following to build.sbt lazy val scalatagsVersion = Def.setting {
scalaBinaryVersion.value match {
case "2.11" => "0.6.8"
case _ => "LATEST.."
}
}
libraryDependencies += "com.lihaoyi" %% "scalatags" % scalatagsVersion.value |
Current status
[error] /Users/poslegm/Documents/scalafmt/scalafmt-benchmarks/src/main/scala/org/scalafmt/benchmarks/MacroBenchmark.scala:5:8: object Corpus is not a member of package scala.meta.testkit
[error] import scala.meta.testkit.Corpus
[error] ^
[error] /Users/poslegm/Documents/scalafmt/scalafmt-benchmarks/src/main/scala/org/scalafmt/benchmarks/MacroBenchmark.scala:6:8: object CorpusFile is not a member of package scala.meta.testkit
[error] import scala.meta.testkit.CorpusFile
[error] ^
[error] /Users/poslegm/Documents/scalafmt/scalafmt-benchmarks/src/main/scala/org/scalafmt/benchmarks/MacroBenchmark.scala:61:15: not found: value Corpus
[error] val x = Corpus
[error] ^
[error] /Users/poslegm/Documents/scalafmt/scalafmt-benchmarks/src/main/scala/org/scalafmt/benchmarks/MacroBenchmark.scala:63:11: not found: value Corpus
[error] Corpus.fastparse.copy(
[error] ^
[error] /Users/poslegm/Documents/scalafmt/scalafmt-benchmarks/src/main/scala/org/scalafmt/benchmarks/MacroBenchmark.scala:65:13: not found: value url
[error] url = Corpus.fastparse.url.replace("olafurpg", "scalameta")
[error] ^
[error] 5 errors found
[info] tests.DynamicSuite *** ABORTED ***
[info] java.lang.NoClassDefFoundError: scala/Serializable
[info] at java.base/java.lang.ClassLoader.defineClass1(Native Method)
[info] at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016)
[info] at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151)
[info] at java.base/java.net.URLClassLoader.defineClass(URLClassLoader.java:515)
[info] at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:423)
[info] at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:417)
[info] at java.base/java.security.AccessController.doPrivileged(AccessController.java:689)
[info] at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:416)
[info] at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:588)
[info] at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
[info] ...
[info] Cause: java.lang.ClassNotFoundException: scala.Serializable
[info] at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:436)
[info] at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:588)
[info] at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
[info] at java.base/java.lang.ClassLoader.defineClass1(Native Method)
[info] at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016)
[info] at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151)
[info] at java.base/java.net.URLClassLoader.defineClass(URLClassLoader.java:515)
[info] at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:423)
[info] at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:417)
[info] at java.base/java.security.AccessController.doPrivileged(AccessController.java:689)
[info] ...
[error] Test suite tests.DynamicSuite failed with java.lang.NoClassDefFoundError: scala/Serializable.
[error] This may be due to the ClassLoaderLayeringStrategy (ScalaLibrary) used by your task.
[error] To improve performance and reduce memory, sbt attempts to cache the class loaders used to load the project dependencies.
[error] The project class files are loaded in a separate class loader that is created for each test run.
[error] The test class loader accesses the project dependency classes using the cached project dependency classloader.
[error] With this approach, class loading may fail under the following conditions:
[error]
[error] * Dependencies use reflection to access classes in your project's classpath.
[error] Java serialization/deserialization may cause this.
[error] * An open package is accessed across layers. If the project's classes access or extend
[error] jvm package private classes defined in a project dependency, it may cause an IllegalAccessError
[error] because the jvm enforces package private at the classloader level.
[error]
[error] These issues, along with others that were not enumerated above, may be resolved by changing the class loader layering strategy.
[error] The Flat and ScalaLibrary strategies bundle the full project classpath in the same class loader.
[error] To use one of these strategies, set the ClassLoaderLayeringStrategy key
[error] in your configuration, for example:
[error]
[error] set dynamic / Test / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.ScalaLibrary
[error] set dynamic / Test / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.Flat
[error]
[error] See ClassLoaderLayeringStrategy.scala for the full list of options. |
@poslegm it's fine to move the benchmark project to the Can you try downgrading sbt to 1.2.8? Those errors look 1.3.2 related. |
Ok, issues 1 and 2 are solved. Thank you @olafurpg! But |
@poslegm the CI is green, is the |
@olafurpg No, I fixed it. The problem was in Scala version for artifact in |
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.
@poslegm Thank you so much for this huge effort! 😄
I left several comments mainly on the version parser.
scalafmt-dynamic/src/main/scala/org/scalafmt/dynamic/ScalafmtVersion.scala
Outdated
Show resolved
Hide resolved
scalafmt-dynamic/src/main/scala/org/scalafmt/dynamic/ScalafmtVersion.scala
Outdated
Show resolved
Hide resolved
scalafmt-dynamic/src/main/scala/org/scalafmt/dynamic/ScalafmtDynamicDownloader.scala
Outdated
Show resolved
Hide resolved
scalafmt-dynamic/src/main/scala/org/scalafmt/dynamic/ScalafmtDynamicDownloader.scala
Outdated
Show resolved
Hide resolved
|
||
import scala.util.control.{NonFatal, NoStackTrace} | ||
|
||
case class ScalafmtVersion(major: Int, minor: Int, patch: Int, rc: Int) { |
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.
[note] Simple implementation of semantic versioning, this is beautiful 👍
(If we have some tried and trusted semver library, maybe we should replace this logic with the library for simplifying the logic, but I'm not sure)
assert( | ||
ScalafmtVersion | ||
.parse("1.2.3-M14") == Left(InvalidVersionException("1.2.3-M14")) | ||
) |
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.
Could you add some more invalid examples? like
2.-1.0
(we could check all the fields are positive or 0)2.1.0.
2.1.0.1
2.1.0-rc1
(small letter)
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.
Added tests. Also I rewrote parser into regex because it's difficult to handle all invalid cases manually.
scalafmt-dynamic/src/main/scala/org/scalafmt/dynamic/ScalafmtDynamic.scala
Outdated
Show resolved
Hide resolved
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 a significant contribution! Thank you @poslegm for your work. LGTM 👍
Awesome! Thank you so much for this big contribution @poslegm ! LGTM |
Fixes #1434
Aaaand now pipeline is green 🚀
Please take a look at ScalafmtVersion introduced in this PR. Now it's difficult to compare scalafmt versions in
ScalafmtDynamicDownloader
only with manual string parsing (e.g..startsWith("0.1")
).Hard-coded scalafmt version with Scala 2.13 in
ScalafmtDynamicDownloader
is 2.2.0. Is it ok? UPD: 2.1.2There are a lot of garbage commits in my branch. I don't like to force push into already published branches, so squash it on merge, please 😄