-
-
Notifications
You must be signed in to change notification settings - Fork 286
Scala 2.12 support #300
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
Scala 2.12 support #300
Conversation
Bump dependencies to versions that support Scala 2.12.
They swapped the logger interface and now there's a different went to silence the logging.
Newer versions of scalatest have scalactic as a dependency. Let bazel know about it and add its a transitive dep.
Add a missing dependency on Twitter's util_core from scrooge rule
This makes the code to compile again on my laptop.
The 031e73c restructured java_commmon rule and dropped both `transitive_compile_time_jars` and `transitive_runtime_jars` from that rule. Let's sync scala.bzl with that change.
Switch to a recent nightly build that has a fix for the flat classpath caching bug that is breaking bazel's incremental compilation when worker is enabled. See bazel-contrib#251 (comment) for the details of the bug.
Fix version numbers of ScalaPB dependencies to be Scala 2.12-specific. # Conflicts: # scala/scala.bzl
Link to an issue explaining the jmh failure is related to Java 8 and not directly caused by the Scala 2.12.
For mysterious reasons, the hash has changed for a build that should be unique. A sign that relying on nightly builds is risky. Yolo for now, though.
This reverts commit 55fd807. Switch back to 2.12.3 until 2.12.4 is released. This will make the testing easier for projects that rely on compiler plugins.
See bazel-contrib#295 for why it's failing.
Can one of the admins verify this patch? |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
adding CLA yes since Grzeg is at Stripe and we have signed the CLA. |
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.
Looks good in general. Well done.
@@ -9,7 +9,7 @@ load("//scala:scala.bzl", | |||
def twitter_scrooge(): | |||
native.maven_server( | |||
name = "twitter_scrooge_maven_server", | |||
url = "http://mirror.bazel.build/repo1.maven.org/maven2/", | |||
url = "http://repo1.maven.org/maven2/", |
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.
Why can't we use the Bazel mirror?
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 can't remember the reason to switch, switched back
@@ -23,23 +23,23 @@ specs2_junit_repositories() | |||
# test adding a scala jar: | |||
maven_jar( | |||
name = "com_twitter__scalding_date", | |||
artifact = scala_mvn_artifact("com.twitter:scalding-date:0.16.0-RC4"), | |||
sha1 = "659eb2d42945dea37b310d96af4e12bf83f54d14" | |||
artifact = scala_mvn_artifact("com.twitter:scalding-date:0.17.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.
Can we do the version upgrades in master to minimize the diff?
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.
+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.
@@ -6,6 +6,12 @@ java_binary( | |||
"ScalacProcessor.java", | |||
], | |||
main_class = "io.bazel.rulesscala.scalac.ScalaCInvoker", | |||
# this probably should be set globally for the entire rules_scala |
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 this needs to be in the scala.bzl?
If so we need to also test that the user gets a clear error message when they request a lower version.
And also they can request a higher version and it's ok
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.
Is there a pattern of enforcing minimal Java version on clients of the rule?
@@ -23,23 +23,23 @@ specs2_junit_repositories() | |||
# test adding a scala jar: | |||
maven_jar( | |||
name = "com_twitter__scalding_date", | |||
artifact = scala_mvn_artifact("com.twitter:scalding-date:0.16.0-RC4"), | |||
sha1 = "659eb2d42945dea37b310d96af4e12bf83f54d14" | |||
artifact = scala_mvn_artifact("com.twitter:scalding-date:0.17.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.
+1
@@ -4,36 +4,36 @@ def specs2_repositories(): | |||
|
|||
native.maven_jar( | |||
name = "io_bazel_rules_scala_org_specs2_specs2_core", | |||
artifact = "org.specs2:specs2-core_2.11:" + specs2_version(), | |||
sha1 = "495bed00c73483f4f5f43945fde63c615d03e637", | |||
artifact = "org.specs2:specs2-core_2.12:" + specs2_version(), |
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.
if we used scala_mvn_artifact
we wouldn't have this change here.
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 went wholesale on this in #303
@@ -81,8 +81,8 @@ def scala_proto_repositories(): | |||
|
|||
native.maven_jar( | |||
name = "scala_proto_rules_scalapb_lenses", | |||
artifact = "com.trueaccord.lenses:lenses_2.11:0.4.12", | |||
sha1 = "c5fbf5b872ce99d9a16d3392ccc0d15a0e43d823", | |||
artifact = "com.trueaccord.lenses:lenses_2.12:0.4.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.
if we used scala_mvn_artifact
here we wouldn't have this change and others in the file.
I'm going to reopen this from my Stripe account to remove the cla check error. Apologies scattered review feedback. |
CLAs look good, thanks! |
I'm going to close this in favor of #304 |
Adds support for Scala 2.12.3. In the history, you can see that I tested Scala 2.12.4 nightly builds to verify the issue with the flat classpath caching.
Notes: