Skip to content

added scala_toolchain with scalacopts support #364

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

Merged
merged 17 commits into from
Jan 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
59 changes: 52 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,13 @@ http_archive(
load("@io_bazel_rules_scala//scala:scala.bzl", "scala_repositories")
scala_repositories()
```
To use a particular tag, use the tagged number in `tag = ` and omit the `commit` attribute.
Note that these plugins are still evolving quickly, as is bazel, so you may need to select
the version most appropriate for you.
In addition, you **must** register `scala_toolchain` - To register default empty toolcahin simply add those lines to `WORKSPACE` file:
```python

load("@io_bazel_rules_scala/scala:toolchains.bzl", "scala_register_toolchains")
scala_register_toolchains()
```
[read more here](#scala_toolchain)

Then in your BUILD file just add the following so the rules will be available:
```python
Expand Down Expand Up @@ -543,9 +547,50 @@ generated by the [ScalaPB compiler](https://github.com/scalapb/ScalaPB).
</tbody>
</table>

## scala_toolchain
Scala toolchain allows you to define global configuration to all scala targets.
Currently the only option that can be set is `scalacopts` but the plan is to expand it to other options as well.

**some scala_toolchain must be registered!**
### Several options to configure scala_toolchain:
#### A) Use default scala_toolchain:
In your workspace file add the following lines:
```python
# WORKSPACE
# register default scala toolchain
load("@io_bazel_rules_scala/scala:toolchains.bzl", "scala_register_toolchains")
scala_register_toolchains()
```
#### B) Defining your own scala_toolchain requires 2 steps:
1. Add your own definition to scala_toolchain to a `BUILD` file:
```python
# //toolchains/BUILD
load("//scala:scala_toolchain.bzl", "scala_toolchain")

scala_toolchain(
name = "my_toolchain_impl",
scalacopts = ["-Ywarn-unused"],
visibility = ["//visibility:public"]
)

toolchain(
name = "my_scala_toolchain",
toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
toolchain = "my_toolchain_impl",
visibility = ["//visibility:public"]
)
```
2. register your custom toolchain from `WORKSPACE`:
```python
# WORKSPACE
# ...
register_toolchains("//toolchains:my_scala_toolchain")
```


## [Experimental] Using strict-deps
Bazel pushes towards explicit and minimial dependencies to keep BUILD file higene and allow for targets to refactor their dependencies without fear of downstream breaking.
Currently rules_scala does this at the cost of having cryptic `scalac` errors when one mistakenly depends on a transitive dependency or, as more often the case for some, a transitive dependency is needed to [please scalac](https://github.com/scalacenter/advisoryboard/blob/master/proposals/009-improve-direct-dependency-experience.md) itself.
Bazel pushes towards explicit and minimial dependencies to keep BUILD file higene and allow for targets to refactor their dependencies without fear of downstream breaking.
Currently rules_scala does this at the cost of having cryptic `scalac` errors when one mistakenly depends on a transitive dependency or, as more often the case for some, a transitive dependency is needed to [please scalac](https://github.com/scalacenter/advisoryboard/blob/master/proposals/009-improve-direct-dependency-experience.md) itself.
To learn more about the motivation of strict-deps itself you can visit this Bazel blog [post](https://blog.bazel.build/2017/06/28/sjd-unused_deps.html) on the subject.

To use it just add `--strict_java_deps=WARN|ERROR` to your `bazel` invocation.
Expand All @@ -556,15 +601,15 @@ Target '//some_package:transitive_dependency' is used but isn't explicitly decla
You can use the following buildozer command:
buildozer 'add deps //some_package:transitive_dependency' //some_other_package:transitive_dependency_user
```
Note that if you have `buildozer` installed you can just run the last line and have it automatically apply the fix for you.
Note that if you have `buildozer` installed you can just run the last line and have it automatically apply the fix for you.

**Caveats:**
<ul>
<li>Extra builds- when strict-deps is on the transitive dependencies are inputs to the compilation action which means you can potentially have more build triggers for changes the cross the ijar boundary </li>
<li>Label propagation- since label of targets are needed for the clear message and since it's not currently supported by JavaInfo from bazel we manually propagate it. This means that the error messages have a significantly lower grade if you don't use one of the scala rules or scala_import (since they don't propagate these labels)</li>
<li>javac outputs incorrect targets due to a problem we're tracing down. Practically we've noticed it's pretty trivial to understand the correct target (i.e. it's almost a formatting problem) </li>
</ul>

Note: Currently strict-deps is protected by a feature toggle but we're strongly considering making it the default behavior as `java_*` rules do.

## Building from source
Expand Down
3 changes: 3 additions & 0 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,6 @@ filegroup(
)
"""
)

load("@io_bazel_rules_scala//scala:toolchains.bzl","scala_register_toolchains")
scala_register_toolchains()
21 changes: 21 additions & 0 deletions scala/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
load("//scala:scala_toolchain.bzl", "scala_toolchain")

toolchain_type(
name = "toolchain_type",
visibility = ["//visibility:public"]
)


scala_toolchain(
name = 'default_toolchain_impl',
scalacopts = [],
visibility = ["//visibility:public"]
)


toolchain(
name = 'default_toolchain',
toolchain_type = '@io_bazel_rules_scala//scala:toolchain_type',
toolchain = ':default_toolchain_impl',
visibility = ["//visibility:public"]
)
29 changes: 21 additions & 8 deletions scala/scala.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

load("//specs2:specs2_junit.bzl", "specs2_junit_dependencies")
load(":scala_cross_version.bzl", "scala_version", "scala_mvn_artifact")
load("@io_bazel_rules_scala//scala:scala_toolchain.bzl", "scala_toolchain")

_jar_filetype = FileType([".jar"])
_java_filetype = FileType([".java"])
_scala_filetype = FileType([".scala"])
Expand Down Expand Up @@ -183,6 +185,9 @@ CurrentTarget: {current_target}

compiler_classpath = ":".join([j.path for j in compiler_classpath_jars])

toolchain = ctx.toolchains['@io_bazel_rules_scala//scala:toolchain_type']
scalacopts = toolchain.scalacopts + ctx.attr.scalacopts

scalac_args = """
Classpath: {cp}
EnableIjar: {enableijar}
Expand All @@ -205,7 +210,7 @@ DependencyAnalyzerMode: {dependency_analyzer_mode}
""".format(
out=ctx.outputs.jar.path,
manifest=ctx.outputs.manifest.path,
scala_opts=",".join(ctx.attr.scalacopts),
scala_opts=",".join(scalacopts),
print_compile_time=ctx.attr.print_compile_time,
plugin_arg=plugin_arg,
cp=compiler_classpath,
Expand Down Expand Up @@ -963,7 +968,8 @@ scala_library = rule(
implementation=_scala_library_impl,
attrs=_scala_library_attrs,
outputs=library_outputs,
fragments = ["java"]
fragments = ["java"],
toolchains = ['@io_bazel_rules_scala//scala:toolchain_type'],
)

# the scala compiler plugin used for dependency analysis is compiled using `scala_library`.
Expand All @@ -978,7 +984,8 @@ scala_library_for_plugin_bootstrapping = rule(
implementation=_scala_library_impl,
attrs= _scala_library_for_plugin_bootstrapping_attrs,
outputs=library_outputs,
fragments = ["java"]
fragments = ["java"],
toolchains = ['@io_bazel_rules_scala//scala:toolchain_type'],
)

_scala_macro_library_attrs = {
Expand All @@ -993,7 +1000,8 @@ scala_macro_library = rule(
implementation=_scala_macro_library_impl,
attrs= _scala_macro_library_attrs,
outputs= common_outputs,
fragments = ["java"]
fragments = ["java"],
toolchains = ['@io_bazel_rules_scala//scala:toolchain_type'],
)

_scala_binary_attrs = {
Expand All @@ -1008,7 +1016,8 @@ scala_binary = rule(
attrs= _scala_binary_attrs,
outputs= common_outputs,
executable=True,
fragments = ["java"]
fragments = ["java"],
toolchains = ['@io_bazel_rules_scala//scala:toolchain_type'],
)

_scala_test_attrs = {
Expand All @@ -1030,7 +1039,8 @@ scala_test = rule(
outputs= common_outputs,
executable=True,
test=True,
fragments = ["java"]
fragments = ["java"],
toolchains = ['@io_bazel_rules_scala//scala:toolchain_type'],
)

_scala_repl_attrs = {}
Expand All @@ -1043,7 +1053,8 @@ scala_repl = rule(
attrs= _scala_repl_attrs,
outputs= common_outputs,
executable=True,
fragments = ["java"]
fragments = ["java"],
toolchains = ['@io_bazel_rules_scala//scala:toolchain_type'],
)

SCALA_BUILD_FILE = """
Expand Down Expand Up @@ -1210,7 +1221,8 @@ scala_junit_test = rule(
attrs= _scala_junit_test_attrs,
outputs= common_outputs,
test=True,
fragments = ["java"]
fragments = ["java"],
toolchains = ['@io_bazel_rules_scala//scala:toolchain_type']
)

def scala_specs2_junit_test(name, **kwargs):
Expand All @@ -1220,3 +1232,4 @@ def scala_specs2_junit_test(name, **kwargs):
suite_label = Label("//src/java/io/bazel/rulesscala/specs2:specs2_test_discovery"),
suite_class = "io.bazel.rulesscala.specs2.Specs2DiscoveredTestSuite",
**kwargs)

12 changes: 12 additions & 0 deletions scala/scala_toolchain.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
def _scala_toolchain_impl(ctx):
toolchain = platform_common.ToolchainInfo(
scalacopts = ctx.attr.scalacopts,
)
return [toolchain]

scala_toolchain = rule(
_scala_toolchain_impl,
attrs = {
'scalacopts': attr.string_list(),
Copy link
Contributor

Choose a reason for hiding this comment

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

could we move the scala_library and scala_compiler jars here as well? Since, ultimately, that is how we will be able to switch from 2.11 to 2.12, etc...

Also, we might have default plugins here (since scala native and scala_js use plugins).

}
)
3 changes: 3 additions & 0 deletions scala/toolchains.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@

def scala_register_toolchains():
native.register_toolchains("@io_bazel_rules_scala//scala:default_toolchain")
21 changes: 21 additions & 0 deletions test_expect_failure/scalacopts_from_toolchain/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
load("//scala:scala_toolchain.bzl", "scala_toolchain")
load("//scala:scala.bzl", "scala_library")

scala_toolchain(
name = "failing_toolchain_impl",
scalacopts = ["-Ywarn-unused"],
visibility = ["//visibility:public"]
)

toolchain(
name = "failing_scala_toolchain",
toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
toolchain = "failing_toolchain_impl",
visibility = ["//visibility:public"]
)

scala_library(
name = "failing_build",
srcs = ["ClassWithUnused.scala"],
scalacopts = ["-Xfatal-warnings"]
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package test_expect_failure.scalacopts_from_toolchain

class ClassWithUnused(name:String){
def talk():String = {
val unusedValue = "I am not used :-("
s"hello $name"
}
}
2 changes: 1 addition & 1 deletion test_intellij_aspect.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ test_intellij_aspect() {
cd intellij && git fetch && git pull
fi
git checkout "${intellij_git_tag}"
bazel test --test_output=errors --override_repository io_bazel_rules_scala="${rules_scala_dir}" //aspect/testing/tests/src/com/google/idea/blaze/aspect/scala/...
bazel test --test_output=errors --override_repository io_bazel_rules_scala="${rules_scala_dir}" --extra_toolchains=@io_bazel_rules_scala//scala:default_toolchain //aspect/testing/tests/src/com/google/idea/blaze/aspect/scala/...
}

dir=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
Expand Down
5 changes: 5 additions & 0 deletions test_rules_scala.sh
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,10 @@ test_scala_import_expect_failure_on_missing_direct_deps_warn_mode() {
test_expect_failure_or_warning_on_missing_direct_deps_with_expected_message "${expected_message1}" ${test_target} "--strict_java_deps=warn" "ne" "${expected_message2}"
}

test_scalaopts_from_scala_toolchain() {
action_should_fail build --extra_toolchains="//test_expect_failure/scalacopts_from_toolchain:failing_scala_toolchain" //test_expect_failure/scalacopts_from_toolchain:failing_build
}

dir=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
# shellcheck source=./test_runner.sh
. "${dir}"/test_runner.sh
Expand Down Expand Up @@ -735,3 +739,4 @@ $runner test_scala_library_expect_failure_on_missing_direct_deps_warn_mode_java
$runner test_scala_library_expect_better_failure_message_on_missing_transitive_dependency_labels_from_other_jvm_rules
$runner test_scala_import_expect_failure_on_missing_direct_deps_warn_mode
$runner bazel build "test_expect_failure/missing_direct_deps/internal_deps/... --strict_java_deps=warn"
$runner test_scalaopts_from_scala_toolchain