Repository: bazelbuild/bazel
Issue: Add support for scalac and jvm_flags in scala.bzl

It would be nice to be able to pass these through, as one can for `java_library` and `java_binary`.


Issue comments:
I'm sorry to dig up this issue from the history but I'm trying to introduce [jvm_flags](https://bazel.build/versions/master/docs/be/java.html#java_binary.jvm_flags) to rules_scala and I could use a bit of understanding about the motivation of this issue.
This change added the ability to configure the `scalac` options **and** JVM flags of `scalac`. I'm not sure why one would need configuring the JVM flags of `scalac`. I've looked hard but can't find a corollary option in the Java Rules.
AFAIU `jvm_flags` in the [java rules](https://bazel.build/versions/master/docs/be/java.html#java_binary) only apply to the binaries and are flags for the executable. `java_library` doesn't support that and only supports `javacopts`. 
I can see 3 options:
1. This behavior exists in the JavaRules under a different name which I did not find.
2. It doesn't exist in JavaRules and it was an honest mistake.
3. It doesn't exist in JavaRules but has a valid use-case (which I'd like then to understand to document it).

Any inputs would be appreciated.
cc @johnynek @damienmg @ulfjack for visibility and maybe additional knowledge
I think it is irrelevant in java because the standard compiler is not
written in Java, is it? Unlike scala where the we run the scala compiler on
a JVM, and sometimes we may need to tune memory issues (for instance we
have had issues with thrift targets with tons of files which are rarely
compiled).

I think this should be rarely needed, but that and just plain carelessness
resulted in the state we have today I think.

To do it again, I think we would want scalac_jvm_arg or something to
signify options to scalac. We may want to set some default ones with a
scala_toolchain rule if we can make one.
On Sat, Apr 29, 2017 at 19:35 Ittai Zeidman <notifications@github.com>
wrote:

> I'm sorry to dig up this issue from the history but I'm trying to
> introduce jvm_flags
> <https://bazel.build/versions/master/docs/be/java.html#java_binary.jvm_flags>
> to rules_scala and I could use a bit of understanding about the motivation
> of this issue.
> This change added the ability to configure the scalac options *and* JVM
> flags of scalac. I'm not sure why one would need configuring the JVM
> flags of scalac. I've looked hard but can't find a corollary option in
> the Java Rules.
> AFAIU jvm_flags in the java rules
> <https://bazel.build/versions/master/docs/be/java.html#java_binary> only
> apply to the binaries and are flags for the executable. java_library
> doesn't support that and only supports javacopts.
> I can see 3 options:
>
>    1. This behavior exists in the JavaRules under a different name which
>    I did not find.
>    2. It doesn't exist in JavaRules and it was an honest mistake.
>    3. It doesn't exist in JavaRules but has a valid use-case (which I'd
>    like then to understand to document it).
>
> Any inputs would be appreciated.
> cc @johnynek <https://github.com/johnynek> @damienmg
> <https://github.com/damienmg> @ulfjack <https://github.com/ulfjack> for
> visibility and maybe additional knowledge
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/bazelbuild/bazel/issues/457#issuecomment-298212635>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEJdiiVHPW-egWJMzREXPxChxhH7nUrks5r1B2OgaJpZM4F-Hje>
> .
>

ok fair enough. I see your point. In that case I'll continue like we discussed with scalac_jvm_flags to keep with the current convention of jvm_flags but to prefix it with scalac to signal it's flags for it.
@johnynek why do we pass then the jvm_flags to javac? We use `-J`/`--jvm_flag` to pass it to javac itself. So we currently overload the same attribute with 3 responsibilities:
1. Scalac jvm flags
2. Javac jvm flags
3. Target exectubale jvm flags
Clearly 3 doesn't belong with 1 and 2. Should 1 and 2 be the same attribute? I think they should but a second opinion would be great here.

Pull request review comments:

Patch that resolved the issue:
diff --git a/tools/build_defs/scala/scala.bzl b/tools/build_defs/scala/scala.bzl
index 222573db8f68da..9618592f688ee2 100644
--- a/tools/build_defs/scala/scala.bzl
+++ b/tools/build_defs/scala/scala.bzl
@@ -25,13 +25,15 @@ def _compile(ctx, jars):
   cmd = """
 set -e
 mkdir -p {out}_tmp
-{scalac} -classpath "{jars}" $@ -d {out}_tmp
+{scalac} {scala_opts} {jvm_flags}  -classpath "{jars}" $@ -d {out}_tmp
 # Make jar file deterministic by setting the timestamp of files
 touch -t 198001010000 $(find .)
 jar cmf {manifest} {out} -C {out}_tmp .
 """
   cmd = cmd.format(
       scalac=_scalac_path,
+      scala_opts = " ".join(ctx.attr.scalacopts),
+      jvm_flags = " ".join(["-J" + flag for flag in ctx.attr.jvm_flags]),
       out=ctx.outputs.jar.path,
       manifest=ctx.outputs.manifest.path,
       jars=":".join([j.path for j in jars]))
@@ -119,6 +121,8 @@ scala_library = rule(
           non_empty=True),
       "deps": attr.label_list(),
       "data": attr.label_list(allow_files=True, cfg=DATA_CFG),
+      "scalacopts": attr.string_list(),
+      "jvm_flags": attr.string_list(),
       },
   outputs={
       "jar": "%{name}_deploy.jar",
@@ -135,6 +139,8 @@ scala_binary = rule(
           non_empty=True),
       "deps": attr.label_list(),
       "data": attr.label_list(allow_files=True, cfg=DATA_CFG),
+      "scalacopts":attr.string_list(),
+      "jvm_flags": attr.string_list(),
       },
   outputs={
       "jar": "%{name}_deploy.jar",


Test patch: