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

Support for selecting scala version #544

Merged
merged 67 commits into from
Jul 17, 2018

Conversation

jhnj
Copy link
Contributor

@jhnj jhnj commented Jun 26, 2018

Currently the scala version used is hardcoded and e.g. using rules_scala to build 2.12 requires pointing to a "2.12" branch. This PR aims to allow users of rules_scala to use any 2.11/2.12 version.

The original goal was to allow the user to specify specifc scala versions for specific targets but as
that would have required more structural changes we decided to focus on a single global Scala version. Due to this I might have made some decisions that don't make sense when just using 1 version.

This is tested with some internal Stripe repos.

Changes:

  • All deps related to a specific Scala version are passed around in a provider: ScalacProvider.
    This could be a toolchain as in increase the use of scala_toolchain #530, provider was
    chosen due to "multiple versions, same workspace" legacy

  • New scala versions are added using new_scala_repository. This will download scala, create a build file for the scalac worker and add all of this to a provider. The build file could be declared statically as well (done dynamically to allow for multiple versions in the future).

  • Both 2.11 and 2.12 versions of libraries (scalatest, scrooge, specs2...) are specified and the correct version is selected using native.bind. The versions of Scrooge and scalatest are also bumped as the current ones don't support 2.12.

  • Currently I've only tested it manually for 2.12 on a different branch, to test both 2.11/2.12 requires a new workspace for testing. I've got a partly working wip version of that as well.

Potential problems:

This PR renames the labels for some scala libraries, e.g. @scala//:scala-xml -> @scala_xml_2_12//jar which
might infere with e.g. replacements when using bazel-deps.

replacements:
  org.scala-lang:
    scala-compiler:
      lang: scala/unmangled
      target: "@scala//:scala-compiler"

TODO (maybe):

  • Add the ability to select e.g. the typelevel compiler + specify where you want to download scala from
  • Multiple scala versions in the same workspace? Create multiple ScalacProviders and override the default to use different versions. Would also require changing some rules to macros

@johnynek @andyscott @travisbrown

johan-stripe and others added 24 commits June 13, 2018 16:34
* Provider[ScalaWorker] contains (will contain) everything
  needed to run a specific version of the scala compiler
* Manually create ScalaWorker for 2.11/2.12
* Run scala_repl using compiler from [ScalaWorker]
* run scala_binary, scala_macro_library,
  scala_test, scala_junit_test using [ScalaWorker]
* Tests now working with 2.12
* Generate build file for scalac_2_12 in repository_rule
* Download scala version specified as parameter
* Set label for ScalaWorker as @{name}//:{name}
* Add initial [ScalaWorker] for both 2.11/2.12
* import scalatest + scalactic for 2.12
* Add 2.12 versions of scalatest_runner and scalatest_reporter
* Manually specify scalatest_reporter for scala_test rule
- Pass both 2.11/2.12 versions as implicit attrs
- Add "major_version" to [ScalaWorker]
- remove unused attr major_version from ScalacProvider
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@johnynek
Copy link
Member

This is pretty exciting!

A few comments.

scala/scala.bzl Outdated
url =
"https://mirror.bazel.build/oss.sonatype.org/content/groups/public/org/scalatest/scalatest_2.11/2.2.6/scalatest_2.11-2.2.6.jar",
sha256 =
Copy link
Member

Choose a reason for hiding this comment

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

let's make sure we find a solution to lock these back down with shas.

Maybe a dict for Map[Version, Map[Name, Sha]] something like that.

Copy link
Member

Choose a reason for hiding this comment

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

Another possible motivation for the dict is that if someone wants to use 2.13 they can't because the versions (2.11/2.12) are hardcoded here, right? Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it's possible to just add the shas, just haven't done that. As we only need one scalatest version for 2.11 and one for 2.12 we can hardcode it, could ofc also be done programmatically with something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't actually think about 2.13. Makes sense to use a dict, to make it easier to add versions / allow the user to add a version without requiring changes in rules_scala

scala/scala.bzl Outdated
"http://central.maven.org/maven2/org/scala-lang/modules/scala-xml_2.11/1.0.5/scala-xml_2.11-1.0.5.jar"
)
native.http_jar(
name = "scala_xml_2_12",
Copy link
Member

Choose a reason for hiding this comment

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

this changes using the sdk single jar approach to using individual jars. It's not a bad change, but maybe we should make that in the mainline and keep it separate from this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make a pr for that 👍

The reasoning for the change was mainly that I wasn't sure whether the versions for the xml + parser-combinators jars found in {scala_version}.tgz are the same for every minor scala version

native.maven_jar(
name = "scala_proto_rules_protoc_bridge_2_12",
artifact = "com.trueaccord.scalapb:protoc-bridge_2.12:0.3.0-M1",
#server = "scala_proto_deps_maven_server",
Copy link
Member

Choose a reason for hiding this comment

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

let's just remove it entirely vs comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that in twitter_scrooge.bzl a maven_server with bazels mirror is specified, do we want to use that one or just remove all server = ... from this file (I guess maven_jar defaults to maven central)

"scalac", "scalalib", "scalareflect", "scalacompiler",
])

def _declare_scalac_provider(ctx):
Copy link
Member

Choose a reason for hiding this comment

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

this is an interesting approach.

ScalacProvider = provider(
doc = "ScalaProvider",
fields = [
"scalac", "scalalib", "scalareflect", "scalacompiler",
Copy link
Member

Choose a reason for hiding this comment

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

I think going this way vs toolchains i.e. #530 is something we need to think about.

@katre what do you think? This probably lets us sidestep the current toolchain issues blocking us. It also allows us to potentially have more than one build in a single repo (build for both scala 2.11 and 2.12), although I don't see how.

Side note: instead of enumerating the extra jars, can we have something like default_compile_classpath, default_runtime_classpath or something? That way we can control that a bit easier in the different cases (really only macro_library should get reflect by default, and only repl needs scalacompiler in the runtime jars.

Copy link
Member

Choose a reason for hiding this comment

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

Re cross building- maybe we can see if we have multiple ScalacProvider instances then we run the rule (scala_library for example) for each instance. This will create different graphs. Right?

Re your side note- I definitely agree. Add more semantics to it. Though I think that you just demonstrated compile/runtime aren't enough, right? If you do that then where would one define the reflect/scalacompiler for all of their repl targets for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re multiple versions in a single repo:
You'd be able to add multiple scala versions (ScalacProviders) in scala_repositories with custom labels, e.g. scala_2_12_6. Then you could override the default ScalacProvider passed to an invocation of scala_library etc.

However, this would require some rules (e.g. scala_test) that have dependencies that need to be either 2.11/2.12 to be changed to macros (so they can figure out which versions to use). I haven't come up with a better solution to that problem yet at least

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ittaiz re cross building: exactly, I have a version where the main rules are working like that (scala_library, ...) allowing you to build different graphs for different versions.

Decided to create a pr for this simpler version first as getting all rules to work with multiple versions might require some bigger changes

server = "scala_proto_deps_maven_server",
name = "scala_proto_rules_scalapbc_2_11",
artifact = "com.trueaccord.scalapb:scalapbc_2.11:0.6.5",
#server = "scala_proto_deps_maven_server",
Copy link
Contributor

Choose a reason for hiding this comment

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

More comments you might as well remove here and below.

@@ -6,44 +6,78 @@ load(
def specs2_version():
return "3.8.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it makes sense to update ScalaTest since that was necessary to get to a version that had a 2.12 build, but it feels a little odd to leave specs2 on a fairly old version.

Copy link
Member

Choose a reason for hiding this comment

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

definitely agree but:

  1. This PR is big as is.
  2. There is always the question of what should rules_scala default to. Not sure. We (Wix) use latest specs2 version internally by binding our own target

srcjar,
"//external:io_bazel_rules_scala/dependency/thrift/libthrift",
"//external:io_bazel_rules_scala/dependency/thrift/scrooge_core",
"//external:io_bazel_rules_scala/dependency/thrift/util_core",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hurray!

tut_rule/tut.bzl Outdated
native.maven_server(
name = "tut_repositories_maven_server",
url = "https://dl.bintray.com/tpolecat/maven/",
)

native.maven_jar(
name = "io_bazel_rules_scala_org_tpolecat_tut_core",
name = "io_bazel_rules_scala_org_tpolecat_tut_core_2_11",
artifact = scala_mvn_artifact("org.tpolecat:tut-core:0.4.8"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish we could update this—0.4.8 is over a year and a half old, which is a looooong time in the life of a young-ish project like this.

scala/scala.bzl Outdated
@@ -70,6 +64,7 @@ _resolve_deps = {
allow_files = False),
}

# TODO not version specific
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftover, was relevant when trying to use multiple versions, I'll remove

@@ -1,6 +1,6 @@
licenses(["notice"]) # 3-clause BSD

load("//scala:scala.bzl", "scala_library", "scala_library_for_plugin_bootstrapping")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but I don't understand why this change is necessary.

Copy link
Contributor Author

@jhnj jhnj Jun 27, 2018

Choose a reason for hiding this comment

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

Nothing related to this PR, I think I played around with the file when getting tests to pass and noticed the import wasn't used

@jhnj jhnj changed the title [WIP] Support for selecting scala version Support for selecting scala version Jul 14, 2018
@ittaiz
Copy link
Member

ittaiz commented Jul 14, 2018 via email

Copy link
Member

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

a few minor comments, otherwise this looks good.

Also, can you merge the latest master?

artifact = scala_mvn_artifact(
"com.twitter:scalding-date:0.17.0",
default_scala_major_version(),
),
sha1 = "420fb0c4f737a24b851c4316ee0362095710caa5",
Copy link
Member

Choose a reason for hiding this comment

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

yeah, we should refactor these into a bzl function load_test_jars() or something to make it clearer. Don't need to do that in this PR.

_declare_scalac_provider(
name = "scala_default",
default_classpath = [
"@io_bazel_rules_scala_scala_library",
Copy link
Member

Choose a reason for hiding this comment

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

these look weird... this looks like just a string, not a target.... I would expect @foo//:bar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add the full paths

Copy link
Member

Choose a reason for hiding this comment

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

honestly I disagree. This is the style promoted (though not enforced) by *_import_external.
I don't feel strongly enough about it to object but just wanted to add these 2c

Copy link
Member

Choose a reason for hiding this comment

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

@ittaiz what does a raw @foo correspond to? Is it the same as @foo//:foo?

scala/scala.bzl Outdated
@@ -61,7 +56,10 @@ _implicit_deps = {
default = Label("@bazel_tools//tools/jdk:current_java_runtime"),
cfg = "host"),
"_java_runtime": attr.label(
default = Label("@bazel_tools//tools/jdk:current_java_runtime"))
default = Label("@bazel_tools//tools/jdk:current_java_runtime")),
"_scalac": attr.label(
Copy link
Member

Choose a reason for hiding this comment

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

_scalac is a bit confusing, since it includes everything, what about _scala_tools or something?

Also, if we make this public, e.g. use "scala_provider" as the name with a default, users could change it at individual call sites, something that might be useful, not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently there's no point in making it public as we can only have one version of @io_bazel_rules_scala//src/java/io/bazel/rulesscala/scalac:scalac_worker (= only one version of scala) but for the future when we'd support multiple scala versions that's what I'd want to do.

I'll change the name to _scala_provider but keep it private for now

"scala_reflect": "6ba385b450a6311a15c918cf8688b9af9327c6104f0ecbd35933cfcd3095fe04",
}

def scala_mvn_artifact(artifact, major_scala_version):
Copy link
Member

Choose a reason for hiding this comment

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

do you want to give a major_scala_version = extract_major_version(default_scala_version()) variant to avoid breaking people?

"@io_bazel_rules_scala_org_specs2_specs2_common//jar",
"@io_bazel_rules_scala_org_specs2_specs2_core//jar",
"@io_bazel_rules_scala_org_specs2_specs2_matcher//jar",
"@io_bazel_rules_scala_org_scalaz_scalaz_core",
Copy link
Member

Choose a reason for hiding this comment

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

this looks weird to me... I guess it is the default target somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scala_maven_import_external creates the target @io_bazel_rules_scala_org_scalaz_scalaz_core:io_bazel_rules_scala_org_scalaz_scalaz_core and an alias to @io_bazel_rules_scala_org_scalaz_scalaz_core//jar:jar, I can add the full path

@@ -1,20 +1,32 @@
java_binary(
name = "scalac",
name = "scalac_worker",
Copy link
Member

Choose a reason for hiding this comment

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

do we need to change the target name? The standard is to make the default target the name of the directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it back, this change is leftover from when I did it dynamically

@jhnj
Copy link
Contributor Author

jhnj commented Jul 16, 2018

I think github is having some problems at the moment, addressed the comments but my commits don't shown here. Here they are anyways https://github.com/jhnj/rules_scala/commits/johan/single-scala-version-selectable

Should I create a new PR as they're not showing here?

Copy link
Member

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

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

@jhnj would you say that the WORKSPACE.template is the reference snippet to someone who wants to use a different version in their workspace (for example 2.12.4)?

_declare_scalac_provider(
name = "scala_default",
default_classpath = [
"@io_bazel_rules_scala_scala_library",
Copy link
Member

Choose a reason for hiding this comment

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

honestly I disagree. This is the style promoted (though not enforced) by *_import_external.
I don't feel strongly enough about it to object but just wanted to add these 2c

ctx.actions.run(
inputs = ins,
outputs = outs,
executable = ctx.executable._scalac,
executable = scalac_provider.scalac.files_to_run.executable,
Copy link
Member

Choose a reason for hiding this comment

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

load("@io_bazel_rules_scala//scala:scala_cross_version.bzl", "extract_major_version")

scala_version = "${scala_version}"
scala_major_version = extract_major_version(scala_version)
Copy link
Member

Choose a reason for hiding this comment

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

is this leftovers?

scala_version_jar_shas = scala_version_jar_shas,
maven_servers = maven_servers)

scala_extra_jar_shas = {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this also be a parameter like the _default_scala_version_jar_shas? I might be missing something here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As these only need to be specified per major scala version I decided to not to have them as parameters. This means that we'll have to update them to support other major scala versions.

I'd rather leave it like this for now and do another PR to add that functionality

@ittaiz
Copy link
Member

ittaiz commented Jul 16, 2018

@jhnj one last thing- you moved the test code into the test_version, right? if so did you preserve history? I think it's super important...

@jhnj
Copy link
Contributor Author

jhnj commented Jul 16, 2018

@ittaiz WORKSPACE.template could work as a example yes. I'll also add something to the readme about using different versions as well

All the old tests are still run as before, for test_version I copied a subset of the tests to test_version/version_specific_tests_dir and those tests are run for different versions in ./test_version.sh. The subset of the test that I copied don't preserve their history. I'm not sure if it's possible to have them preserve it, if it is I can redo that

@jhnj jhnj force-pushed the johan/single-scala-version-selectable branch from 513ffc7 to b131900 Compare July 16, 2018 23:36
@johnynek
Copy link
Member

👍

@ittaiz do you have any concerns?

I'm ready to (squash) merge. Note, the CLA is signed. @jhnj did this work at Stripe, and we have signed the CLA. For some reason the system is not detecting it.

Copy link
Member

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

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

Well done!

@ittaiz
Copy link
Member

ittaiz commented Jul 17, 2018

@jhnj you have conflicts again :( any chance you can merge master one last time?

@jhnj
Copy link
Contributor Author

jhnj commented Jul 17, 2018

@ittaiz It should be up to date with latest master, git log has 329543d78d2071ef8a54986f4cd2116e64c6c087 (latest commit on master) locally and github says "This branch has no conflicts with the base branch" for me

@jhnj
Copy link
Contributor Author

jhnj commented Jul 17, 2018

And googlebot commented above regarding the cla:
"""
So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

"""

@ittaiz
Copy link
Member

ittaiz commented Jul 17, 2018

screen shot 2018-07-17 at 10 26 01
It was because of the rebase. Changed to squash and it's ok. Merging.

@ittaiz ittaiz merged commit 043ba58 into bazelbuild:master Jul 17, 2018
@ittaiz
Copy link
Member

ittaiz commented Jul 17, 2018

🎉 🎈 well done and thanks!


_scala_maven_import_external(
name = "io_bazel_rules_scala_util_core",
artifact = _scala_mvn_artifact("com.twitter:util-core:18.6.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

@johnynek This PR has a few decent changes into how folks must consider the bindings for both the scala libs, and also the version changed in scrooge. --- should we have some sort of upgrading readme/notes with these?
e.g. if you use bazel deps you need to change X to Y, If you use scrooge you must add the bind section , etc..

Copy link
Member

Choose a reason for hiding this comment

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

@jhnj would you be able to make a short .md file that describes how to set up in the normal case but also how to customize to point to different versions? This often comes up as a question users have but we only have the source code to point to and it can be hard to find for new users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnynek @ianoc There is a short section in the readme about selecting scala version (Selecting Scala version ), I can definitely add a more thorough explanation there (or another md file) if needed.

Regarding the updated thrift version + bazel-deps change, there is currently no versioning for rules_scala, would we want that? We can ofc add upgrading notes for commits that need action from the users, e.g. 76d4ab9f855f78113ee5990a84b0ad55d2417e4a requires you to change X if you're using bazel-deps but I think that could quickly become noisy + it's not very readable.

(sidenote: the bazel-deps change actually came from an earlier PR)

Copy link
Member

Choose a reason for hiding this comment

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

Some users don't use our provided scala_repositories, and instead wire them up from jars found by say bazel-deps. It would be good to add a section on using these rules with bazel-deps (either here, or in the bazel-deps repo).

Copy link
Member

Choose a reason for hiding this comment

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

I think referencing commits is fine. I don't think we have the energy to do a more formal versioning for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the users just bind their own jars, basically replacing this:

native.bind(
      name = "io_bazel_rules_scala/dependency/scalatest/scalatest",
      actual = "@io_bazel_rules_scala//scala/scalatest:scalatest")

nothing should have changed in this PR, the name it's bound to hasn't changed.

There is a example on using jars from scala_repositories with bazel-deps in the bazel-deps readme but can add something here as well

Copy link
Contributor

Choose a reason for hiding this comment

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

This for instance we need to call out would silently change the version of the scrooge generator used by people. It wasn't a custom binding before hand. Thats an important change we would want to call out since it would lead to strange/confusing errors.

(in general changing the default version of the libraries is probably something we want to call out pretty loudly. could easily cause binary incompatibilities or other issues for folks even if it compiles inside their repo)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, adding notes about that 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants