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.

Sign up for GitHub

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
Merged

Support for selecting scala version #544

Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
67 commits
Select commit Hold shift + click to select a range
d27e73f
WIP compile using worker supplied from provider
johan-stripe Jun 13, 2018
abae2b2
Add scalareflect + scalacompiler to [ScalaWorker]
johan-stripe Jun 14, 2018
f670436
All rules using [ScalaWorker]
johan-stripe Jun 14, 2018
50deaf3
Create [ScalaWorker] using new_scala_repository
johan-stripe Jun 14, 2018
b9f1b22
Use filegroup for scalac worker files
johan-stripe Jun 14, 2018
2aa315c
Specify scala version and label in new_scala_repository
johan-stripe Jun 14, 2018
27d3af0
Merge branch 'master' into johan/cross-build
johan-stripe Jun 14, 2018
0e5c5dd
Run formatter
johan-stripe Jun 14, 2018
72aba58
Run scalatest for both 2.11/2.12
johan-stripe Jun 16, 2018
25132c9
Update scalatest to 3.0.5 + remove defaults from ScalaWorker
johan-stripe Jun 18, 2018
d8edfb6
Symlink scalac_worker sources to repo_rule workspace
johan-stripe Jun 18, 2018
cf37e0f
Resolve scalatest_reporter version in rule
johan-stripe Jun 19, 2018
5b2a053
Use new_scala_repository for default 2.11/2.12
johan-stripe Jun 19, 2018
2dc260f
scala_repositories takes scala versions as parameters
johan-stripe Jun 19, 2018
94d0ab1
improve naming
johan-stripe Jun 19, 2018
a47af87
Use single scala version
johan-stripe Jun 21, 2018
52bdef1
Fix runfiles not being generated
johan-stripe Jun 21, 2018
a125428
Support multiple scala versions for tut and scrooge
johan-stripe Jun 22, 2018
dd28058
Format .bzl and BUILD files
johan-stripe Jun 22, 2018
e6aff25
Tests passing with 2.11
johan-stripe Jun 22, 2018
e600177
Make scala_proto work with 2.12
johan-stripe Jun 23, 2018
927e5f8
Support specs2 for both 2.11/2.12
jhnj Jun 26, 2018
22f216f
Don't pass scalatest dependencies through provider
jhnj Jun 26, 2018
87dad40
Pass _scalac through _implicit_deps
jhnj Jun 26, 2018
d45943d
Remove temp test directory
jhnj Jun 26, 2018
990cb9d
Remove debugging println
jhnj Jun 26, 2018
57449bb
Cleanup
jhnj Jun 27, 2018
adeff15
Run formatter
jhnj Jun 27, 2018
2bd3825
Use inputs from resolve_command
jhnj Jun 27, 2018
e1846f5
Combine scalatest+scalactic to single target
jhnj Jun 27, 2018
5d67d4d
Add utils
jhnj Jun 27, 2018
0dc4361
Increase scala_mvn_artifact usage, add shas
jhnj Jun 27, 2018
a66d27a
Add default_major_scala_version()
jhnj Jun 28, 2018
8b48a1e
Add scala_jar_shas dict
jhnj Jun 28, 2018
1648bd4
Fix aspect test
jhnj Jun 28, 2018
e1e7cfb
Merge branch 'master' into johan/single-scala-version-selectable
jhnj Jun 30, 2018
b819162
Use scala_maven_import_external for all imports
jhnj Jul 2, 2018
b6c5806
Add util_core to scrooge, fix aspect test
jhnj Jul 2, 2018
ed646b4
Supply shas to scala_library,compiler and reflect
jhnj Jul 2, 2018
bc36a86
namespace scala_default
jhnj Jul 3, 2018
ae407ac
Scala default version to 2.11.12
jhnj Jul 3, 2018
f5e5ff6
Lookup scala_extra_jar_shas[major_version] once, renaming
jhnj Jul 3, 2018
5f26b2b
tut server_urls as parameter
jhnj Jul 3, 2018
8a7b120
Pass ScalacProvider as paramater
jhnj Jul 3, 2018
7b36b2d
Replace pass classpaths instead of jars
jhnj Jul 3, 2018
dc9eb43
linting
jhnj Jul 3, 2018
a72372c
Lookup scala_jar_shas[major_version] only once
jhnj Jul 3, 2018
260fd7f
Merge branch 'master' into johan/single-scala-version-selectable
jhnj Jul 3, 2018
829705d
Run version specific tests in their own workspace
jhnj Jul 4, 2018
8abd79f
remove more non version-specific tests
jhnj Jul 4, 2018
2252748
Remove version from scala labels fix tests
jhnj Jul 5, 2018
b43be39
Use default_runtime_classpath for macros, fix aspect test
jhnj Jul 5, 2018
2db2ada
Run more tests in test_version.sh
jhnj Jul 5, 2018
f0c262c
Update naming, default_compile_classpath -> base_compile_classpath
jhnj Jul 5, 2018
78cb80e
Use base_classpath instead of base_compile.. + base_runtime...
jhnj Jul 5, 2018
6c05916
Add scala_reflect to default_classpath
jhnj Jul 5, 2018
50cbd54
scala_version and shas as on tuple paramter
jhnj Jul 5, 2018
e1b12ba
Reduce amount of version specific junit tests
jhnj Jul 5, 2018
4774178
Run version specific tests
jhnj Jul 5, 2018
4661265
Run linter
jhnj Jul 5, 2018
a834ba5
Remove dynamic creation of scala_default ScalacProvider
jhnj Jul 11, 2018
e518edc
PR comments, clearer naming
jhnj Jul 16, 2018
16767e3
Merge branch 'master' into johan/single-scala-version-selectable
jhnj Jul 16, 2018
25db5f6
Use ctx.attr._scala_provider instead of ..attr.scalalib, run linter
jhnj Jul 16, 2018
20ee544
Address comments, clean up WORKSPACE.template
jhnj Jul 16, 2018
c9dffbf
Remove unused code
jhnj Jul 16, 2018
b131900
Add section about different scala versions to the readme
jhnj Jul 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions WORKSPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,21 @@ load("//scala:scala_cross_version.bzl", "scala_mvn_artifact")
# test adding a scala jar:
maven_jar(
name = "com_twitter__scalding_date",
artifact = scala_mvn_artifact("com.twitter:scalding-date:0.17.0"),
artifact = scala_mvn_artifact("com.twitter:scalding-date:0.17.0", "2.11"),
Copy link
Member

Choose a reason for hiding this comment

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

thought (which can be deferred to later PR):
WDYT about:
artifact = scala_mvn_artifact("com.twitter:scalding-date:0.17.0", default_scala_major_version),
where default_scala_major_version is defined as extract_major_version_underscore(default_scala_version())
Will ease changing the default version for rules_scala when we'll need it

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.

Seems like the shas need to become a map of scala version -> artifact -> sha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that these maven_jars are just for tests and as they always use default_scala_version() (which is hardcoded) we shouldn't need a map here. The shas only need to be changed if we change default_scala_version(). Or should we add a map just to make the fact that they need to be changed more explicit?

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.

)

# For testing that we don't include sources jars to the classpath
maven_jar(
name = "org_typelevel__cats_core",
artifact = scala_mvn_artifact("org.typelevel:cats-core:0.9.0"),
artifact = scala_mvn_artifact("org.typelevel:cats-core:0.9.0", "2.11"),
sha1 = "b2f8629c6ec834d8b6321288c9fe77823f1e1314",
)

# test of a plugin
maven_jar(
name = "org_psywerx_hairyfotr__linter",
artifact = scala_mvn_artifact("org.psywerx.hairyfotr:linter:0.1.13"),
artifact = scala_mvn_artifact("org.psywerx.hairyfotr:linter:0.1.13", "2.11"),
sha1 = "e5b3e2753d0817b622c32aedcb888bcf39e275b4",
)

Expand Down
31 changes: 22 additions & 9 deletions scala/private/rule_impls.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
# limitations under the License.
"""Rules for supporting the Scala language."""
load("@io_bazel_rules_scala//scala:scala_toolchain.bzl", "scala_toolchain")
load("@io_bazel_rules_scala//scala:providers.bzl", "create_scala_provider")
load(
"@io_bazel_rules_scala//scala:providers.bzl",
"create_scala_provider",
_ScalacProvider = "ScalacProvider")
load(
":common.bzl",
"add_labels_of_jars_to",
Expand Down Expand Up @@ -240,18 +243,25 @@ StatsfileOutput: {statsfile_output}
ctx.actions.write(
output = argfile, content = scalac_args + optional_scalac_args)

scalac_provider = ctx.attr._scalac[_ScalacProvider]
Copy link
Member

Choose a reason for hiding this comment

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

@johnynek given your work trying to make compile_scala more explicit and less ctx dependent; Would you rather this be a parameter?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I’d prefer a parameter right keep function cleaner. Eventually I’d like to not accept ctx and instead only actions.

scalac_inputs, _, scalac_input_manifests = ctx.resolve_command(
tools = [scalac_provider.scalac])

outs = [ctx.outputs.jar, ctx.outputs.statsfile]
if buildijar:
outs.extend([ctx.outputs.ijar])
ins = (compiler_classpath_jars.to_list() + dep_srcjars.to_list() +
list(srcjars) + list(sources) + ctx.files.srcs + ctx.files.plugins +
dependency_analyzer_plugin_jars + classpath_resources +
ctx.files.resources + ctx.files.resource_jars + ctx.files._java_runtime
+ [ctx.outputs.manifest, ctx.executable._ijar, argfile])
ins = (
compiler_classpath_jars.to_list() + dep_srcjars.to_list() +
list(srcjars) + list(sources) + ctx.files.srcs + ctx.files.plugins +
dependency_analyzer_plugin_jars + classpath_resources +
ctx.files.resources + ctx.files.resource_jars + ctx.files._java_runtime +
[ctx.outputs.manifest, ctx.executable._ijar, argfile] + scalac_inputs)

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.

can you take this from the tuple output (command) of the resolve_command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't work, command is ["/bin/bash", "-c", ""] not an executable

Copy link
Member

Choose a reason for hiding this comment

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

@laurentlb is this the expected behavior? Sounds strange we can get the inputs and manifests from resolve_command but not the executable. (@jhnj this comment isn't blocking the merge)

Copy link
Contributor

Choose a reason for hiding this comment

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

@c-parsons

Copy link
Member

Choose a reason for hiding this comment

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

ping @laurentlb @c-parsons

input_manifests = scalac_input_manifests,
mnemonic = "Scalac",
progress_message = "scala %s" % ctx.label,
execution_requirements = {"supports-workers": "1"},
Expand Down Expand Up @@ -486,7 +496,8 @@ def _collect_jars_from_common_ctx(ctx, extra_deps = [],
dependency_analyzer_is_off = is_dependency_analyzer_off(ctx)

# Get jars from deps
auto_deps = [ctx.attr._scalalib, ctx.attr._scalareflect]
scalac_provider = ctx.attr._scalac[_ScalacProvider]
auto_deps = [scalac_provider.scalalib, scalac_provider.scalareflect]
Copy link
Member

Choose a reason for hiding this comment

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

This could directly be given in the toolchain/provider.

deps_jars = collect_jars(ctx.attr.deps + auto_deps + extra_deps,
dependency_analyzer_is_off)
(cjars, transitive_rjars, jars2labels,
Expand Down Expand Up @@ -641,7 +652,8 @@ def scala_binary_impl(ctx):
def scala_repl_impl(ctx):
# need scala-compiler for MainGenericRunner below
jars = _collect_jars_from_common_ctx(
ctx, extra_runtime_deps = [ctx.attr._scalacompiler])
ctx,
extra_runtime_deps = [ctx.attr._scalac[_ScalacProvider].scalacompiler])
(cjars, transitive_rjars) = (jars.compile_jars, jars.transitive_runtime_jars)

args = " ".join(ctx.attr.scalacopts)
Expand Down Expand Up @@ -689,6 +701,7 @@ def _scala_test_flags(ctx):
def scala_test_impl(ctx):
if len(ctx.attr.suites) != 0:
print("suites attribute is deprecated. All scalatest test suites are run")

jars = _collect_jars_from_common_ctx(
ctx,
extra_runtime_deps = [
Expand Down
33 changes: 33 additions & 0 deletions scala/providers.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,36 @@ def create_scala_provider(ijar, class_jar, compile_jars,
transitive_runtime_jars = transitive_runtime_jars,
transitive_exports = [] #needed by intellij plugin
)

ScalacProvider = provider(
doc = "ScalaProvider",
fields = [
"scalac",
"scalalib",
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 we should consider instead default_compile_classpath, default_runtime_classpath, default_repl_classpath, and default_macro_classpath. Then we use exports to set up what we want, but that is not hard wired to specific jars.

Copy link
Contributor Author

@jhnj jhnj Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In _collect_jars_from_common_ctx:

Would the idea be to have default_compile_classpath etc. as a depset
( depset([<source file external/io_bazel_rules_scala_scala_compiler_2_11_12/scala-compiler-2.11.12.jar>, ...], order = "preorder") )
and combine that with the result of collect_jars or just have default_compile_classpath be a list of labels that is passed to collect_jars (basically replacing auto_deps)?

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 attr.label_list is convenient.

"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.

return [
ScalacProvider(
scalac = ctx.attr.scalac,
scalalib = ctx.attr.scalalib,
scalareflect = ctx.attr.scalareflect,
scalacompiler = ctx.attr.scalacompiler,
)
]

declare_scalac_provider = rule(
implementation = _declare_scalac_provider,
attrs = {
"scalac": attr.label(
executable = True,
cfg = "host",
allow_files = True,
mandatory = True),
"scalalib": attr.label(mandatory = True, allow_files = True),
"scalareflect": attr.label(mandatory = True, allow_files = True),
"scalacompiler": attr.label(mandatory = True, allow_files = True),
"scalaxml": attr.label(mandatory = True, allow_files = True)
})
162 changes: 85 additions & 77 deletions scala/scala.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,17 @@ load(
_scala_junit_test_impl = "scala_junit_test_impl",
)

load(
"@io_bazel_rules_scala//scala:providers.bzl",
_ScalacProvider = "ScalacProvider",
)

load(
"@io_bazel_rules_scala//scala:scala_cross_version.bzl",
_new_scala_repository = "new_scala_repository",
_extract_major_version_underscore = "extract_major_version_underscore",
_default_scala_version = "default_scala_version")

load(
"@io_bazel_rules_scala//specs2:specs2_junit.bzl",
_specs2_junit_dependencies = "specs2_junit_dependencies")
Expand All @@ -28,23 +39,6 @@ _implicit_deps = {
cfg = "host",
default = Label("@bazel_tools//tools/jdk:ijar"),
allow_files = True),
"_scalac": attr.label(
executable = True,
cfg = "host",
default = Label("//src/java/io/bazel/rulesscala/scalac"),
allow_files = True),
"_scalalib": attr.label(
default = Label(
"//external:io_bazel_rules_scala/dependency/scala/scala_library"),
allow_files = True),
"_scalacompiler": attr.label(
default = Label(
"//external:io_bazel_rules_scala/dependency/scala/scala_compiler"),
allow_files = True),
"_scalareflect": attr.label(
default = Label(
"//external:io_bazel_rules_scala/dependency/scala/scala_reflect"),
allow_files = True),
"_zipper": attr.label(
executable = True,
cfg = "host",
Expand All @@ -56,7 +50,9 @@ _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

default = Label("@scala_default"), providers = [_ScalacProvider])
Copy link
Member

Choose a reason for hiding this comment

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

I’d like to namespace this io_bazel_rules_scala...

}

# Single dep to allow IDEs to pickup all the implicit dependencies.
Expand Down Expand Up @@ -219,12 +215,11 @@ _scala_test_attrs = {
"full_stacktraces": attr.bool(default = True),
"_scalatest": attr.label(
default = Label(
"//external:io_bazel_rules_scala/dependency/scalatest/scalatest"),
allow_files = True),
"//external:io_bazel_rules_scala/dependency/scalatest/scalatest")),
"_scalatest_runner": attr.label(
executable = True,
cfg = "host",
default = Label("//src/java/io/bazel/rulesscala/scala_test:runner.jar"),
executable = True,
allow_files = True),
"_scalatest_reporter": attr.label(
default = Label("//scala/support:test_reporter")),
Expand Down Expand Up @@ -257,56 +252,60 @@ scala_repl = rule(
toolchains = ['@io_bazel_rules_scala//scala:toolchain_type'],
)

_SCALA_BUILD_FILE = """
# scala.BUILD
java_import(
name = "scala-xml",
jars = ["lib/scala-xml_2.11-1.0.5.jar"],
visibility = ["//visibility:public"],
)
def scala_repositories(scala_version = _default_scala_version()):
major_version_underscore = _extract_major_version_underscore(scala_version)

java_import(
name = "scala-parser-combinators",
jars = ["lib/scala-parser-combinators_2.11-1.0.4.jar"],
visibility = ["//visibility:public"],
)
_new_scala_repository("scala_default", scala_version)

java_import(
name = "scala-library",
jars = ["lib/scala-library.jar"],
visibility = ["//visibility:public"],
)

java_import(
name = "scala-compiler",
jars = ["lib/scala-compiler.jar"],
visibility = ["//visibility:public"],
)

java_import(
name = "scala-reflect",
jars = ["lib/scala-reflect.jar"],
visibility = ["//visibility:public"],
)
"""

def scala_repositories():
native.new_http_archive(
name = "scala",
strip_prefix = "scala-2.11.11",
sha256 =
"12037ca64c68468e717e950f47fc77d5ceae5e74e3bdca56f6d02fd5bfd6900b",
url = "https://downloads.lightbend.com/scala/2.11.11/scala-2.11.11.tgz",
build_file_content = _SCALA_BUILD_FILE,
# scalatest has macros, note http_jar is invoking ijar
Copy link
Member

Choose a reason for hiding this comment

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

@johnynek should I open an issue about changing these http_jar to use scala_maven_import_external?

native.http_jar(
name = "scalatest_2_11",
url =
"http://central.maven.org/maven2/org/scalatest/scalatest_2.11/3.0.5/scalatest_2.11-3.0.5.jar",
sha256 = "2aafeb41257912cbba95f9d747df9ecdc7ff43f039d35014b4c2a8eb7ed9ba2f"
)
native.http_jar(
name = "scalactic_2_11",
url =
"http://central.maven.org/maven2/org/scalactic/scalactic_2.11/3.0.5/scalactic_2.11-3.0.5.jar",
sha256 = "84723064f5716f38990fe6e65468aa39700c725484efceef015771d267341cf2"
)

# scalatest has macros, note http_jar is invoking ijar
native.http_jar(
name = "scalatest",
name = "scalatest_2_12",
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

"f198967436a5e7a69cfd182902adcfbcb9f2e41b349e1a5c8881a2407f615962",
"http://central.maven.org/maven2/org/scalatest/scalatest_2.12/3.0.5/scalatest_2.12-3.0.5.jar",
sha256 = "b416b5bcef6720da469a8d8a5726e457fc2d1cd5d316e1bc283aa75a2ae005e5"
)
native.http_jar(
name = "scalactic_2_12",
url =
"http://central.maven.org/maven2/org/scalactic/scalactic_2.12/3.0.5/scalactic_2.12-3.0.5.jar",
sha256 = "57e25b4fd969b1758fe042595112c874dfea99dca5cc48eebe07ac38772a0c41"
)
native.http_jar(
name = "scala_xml_2_11",
url =
"http://central.maven.org/maven2/org/scala-lang/modules/scala-xml_2.11/1.0.5/scala-xml_2.11-1.0.5.jar",
sha256 = "767e11f33eddcd506980f0ff213f9d553a6a21802e3be1330345f62f7ee3d50f"
)
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

url =
"http://central.maven.org/maven2/org/scala-lang/modules/scala-xml_2.12/1.0.5/scala-xml_2.12-1.0.5.jar",
sha256 = "035015366f54f403d076d95f4529ce9eeaf544064dbc17c2d10e4f5908ef4256"
)
native.http_jar(
name = "scala_parser_combinators_2_11",
url =
"http://central.maven.org/maven2/org/scala-lang/modules/scala-parser-combinators_2.11/1.0.4/scala-parser-combinators_2.11-1.0.4.jar",
sha256 = "0dfaafce29a9a245b0a9180ec2c1073d2bd8f0330f03a9f1f6a74d1bc83f62d6"
)
native.http_jar(
name = "scala_parser_combinators_2_12",
url =
"http://central.maven.org/maven2/org/scala-lang/modules/scala-parser-combinators_2.12/1.0.4/scala-parser-combinators_2.12-1.0.4.jar",
sha256 = "282c78d064d3e8f09b3663190d9494b85e0bb7d96b0da05994fe994384d96111"
)

native.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.

if we don't use maven_jar then we don't need maven_server anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That maven_server is still used by

  native.maven_jar(
      name = "scalac_rules_protobuf_java",
      artifact = "com.google.protobuf:protobuf-java:3.1.0",
      sha1 = "e13484d9da178399d32d2d27ee21a77cfb4b7873",
      server = "scalac_deps_maven_server",
  )

Could remove it if we're happy just using maven central instead of the bazel mirror

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 keep it. Maybe we should drop the maven jar entirely but that’s a different story

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 keep it. Maybe we should drop the maven jar entirely but that’s a different story

Expand Down Expand Up @@ -356,28 +355,37 @@ def scala_repositories():
actual = "@scalac_rules_commons_io//jar")

native.bind(
name = "io_bazel_rules_scala/dependency/scala/parser_combinators",
actual = "@scala//:scala-parser-combinators")
name = "io_bazel_rules_scala/dependency/scalatest/scalatest_library",
actual = "@scalatest_{}//jar".format(major_version_underscore))
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 want a utility for this? something like scala_external("scalatest", provider) => "@$repo_name_$ver//jar".format(...)

Maybe not, 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.

If this way of selecting the dependencies sound reasonable to you I can definitely add some kind of utility to make it simpler

Copy link
Member

Choose a reason for hiding this comment

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

To me this sounds reasonable. what do you think? ;) @johnynek wdyt?


native.bind(
name = "io_bazel_rules_scala/dependency/scala/scala_compiler",
actual = "@scala//:scala-compiler")
name = "io_bazel_rules_scala/dependency/scalactic/scalactic",
actual = "@scalactic_{}//jar".format(major_version_underscore))

native.bind(
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 you only need this bind. Why do you need to bind scalactic and scalatest_library as well?

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/scalatest:scalatest needs to export the correct versions of scalatest/scalactic and here the binds are used to select which versions it exports. I think I'll do it with an if statement instead, that's probably clearer

name = "io_bazel_rules_scala/dependency/scala/scala_library",
actual = "@scala//:scala-library")
name = "io_bazel_rules_scala/dependency/scalatest/scalatest",
actual = "@io_bazel_rules_scala//scala/scalatest:scalatest")

native.bind(
name = "io_bazel_rules_scala/dependency/scala/scala_reflect",
actual = "@scala//:scala-reflect")
name = "io_bazel_rules_scala/dependency/scala/scala_xml",
actual = "@scala_xml_{}//jar".format(major_version_underscore))

native.bind(
name = "io_bazel_rules_scala/dependency/scala/scala_xml",
actual = "@scala//:scala-xml")
name = "io_bazel_rules_scala/dependency/scala/parser_combinators",
actual = "@scala_parser_combinators_{}//jar".format(
major_version_underscore))

native.bind(
name = "io_bazel_rules_scala/dependency/scalatest/scalatest",
actual = "@scalatest//jar")
name = "io_bazel_rules_scala/dependency/scala/scala_compiler",
actual = "@scala_default_imports//:scala-compiler")

native.bind(
name = "io_bazel_rules_scala/dependency/scala/scala_library",
actual = "@scala_default_imports//:scala-library")

native.bind(
name = "io_bazel_rules_scala/dependency/scala/scala_reflect",
actual = "@scala_default_imports//:scala-reflect")

def _sanitize_string_for_usage(s):
res_array = []
Expand Down
Loading