From 38bffc8e890c0ee3ed0d4640eb3b8c7d1da7157a Mon Sep 17 00:00:00 2001
From: Alex Beal <beala@stripe.com>
Date: Wed, 7 Aug 2019 15:57:26 -0600
Subject: [PATCH 1/2] Fix a regression that breaks scalapb when jvm_flags are
 set on the toolchain

---
 scala/private/rule_impls.bzl                   |  6 +++++-
 test_expect_failure/scalac_jvm_opts/BUILD      | 18 ++++++++++++++++++
 test_expect_failure/scalac_jvm_opts/test.proto |  8 ++++++++
 test_rules_scala.sh                            |  5 +++++
 4 files changed, 36 insertions(+), 1 deletion(-)
 create mode 100644 test_expect_failure/scalac_jvm_opts/test.proto

diff --git a/scala/private/rule_impls.bzl b/scala/private/rule_impls.bzl
index c92542482..da6d5cc4c 100644
--- a/scala/private/rule_impls.bzl
+++ b/scala/private/rule_impls.bzl
@@ -128,7 +128,11 @@ touch {statsfile}
     )
 
 def _expand_location(ctx, flags):
-    return [ctx.expand_location(f, ctx.attr.data) for f in flags]
+    if hasattr(ctx.attr, "data"):
+        data = ctx.attr.data
+    else:
+        data = []
+    return [ctx.expand_location(f, data) for f in flags]
 
 def _join_path(args, sep = ","):
     return sep.join([f.path for f in args])
diff --git a/test_expect_failure/scalac_jvm_opts/BUILD b/test_expect_failure/scalac_jvm_opts/BUILD
index 1b7772ec0..1befa0af9 100644
--- a/test_expect_failure/scalac_jvm_opts/BUILD
+++ b/test_expect_failure/scalac_jvm_opts/BUILD
@@ -1,5 +1,9 @@
 load("//scala:scala_toolchain.bzl", "scala_toolchain")
 load("//scala:scala.bzl", "scala_library")
+load(
+    "//scala_proto:scala_proto.bzl",
+    "scalapb_proto_library",
+)
 
 scala_toolchain(
     name = "failing_toolchain_impl",
@@ -41,3 +45,17 @@ scala_library(
     # the `failing_scala_toolchain` is used.
     scalac_jvm_flags = ["-Xmx1G"],
 )
+
+proto_library(
+    name = "test",
+    srcs = ["test.proto"],
+    visibility = ["//visibility:public"],
+)
+
+# This is a regression test for a bug that broke compiling scalapb targets when
+# `scalac_jvm_flags` was set on the toolchain.
+scalapb_proto_library(
+    name = "proto",
+    visibility = ["//visibility:public"],
+    deps = [":test"],
+)
\ No newline at end of file
diff --git a/test_expect_failure/scalac_jvm_opts/test.proto b/test_expect_failure/scalac_jvm_opts/test.proto
new file mode 100644
index 000000000..0884e1d05
--- /dev/null
+++ b/test_expect_failure/scalac_jvm_opts/test.proto
@@ -0,0 +1,8 @@
+syntax = "proto2";
+
+option java_package = "test.proto";
+
+message TestResponse1 {
+  optional uint32 c = 1;
+  optional bool d = 2;
+}
diff --git a/test_rules_scala.sh b/test_rules_scala.sh
index 9304097cd..a5f92eb92 100755
--- a/test_rules_scala.sh
+++ b/test_rules_scala.sh
@@ -837,6 +837,10 @@ test_scalac_jvm_flags_on_target_overrides_toolchain_passes() {
   bazel build --extra_toolchains="//test_expect_failure/scalac_jvm_opts:failing_scala_toolchain" //test_expect_failure/scalac_jvm_opts:empty_overriding_build
 }
 
+test_scalac_jvm_flags_work_with_scalapb() {
+  bazel build --extra_toolchains="//test_expect_failure/scalac_jvm_opts:passing_scala_toolchain" //test_expect_failure/scalac_jvm_opts:proto
+}
+
 test_scala_test_jvm_flags_from_scala_toolchain_fails() {
   action_should_fail test --extra_toolchains="//test_expect_failure/scala_test_jvm_flags:failing_scala_toolchain" //test_expect_failure/scala_test_jvm_flags:empty_test
 }
@@ -1131,6 +1135,7 @@ $runner scala_pb_library_targets_do_not_have_host_deps
 $runner test_scalac_jvm_flags_on_target_overrides_toolchain_passes
 $runner test_scalac_jvm_flags_from_scala_toolchain_passes
 $runner test_scalac_jvm_flags_from_scala_toolchain_fails
+$runner test_scalac_jvm_flags_work_with_scalapb
 $runner test_scala_test_jvm_flags_on_target_overrides_toolchain_passes
 $runner test_scala_test_jvm_flags_from_scala_toolchain_passes
 $runner test_scala_test_jvm_flags_from_scala_toolchain_fails
\ No newline at end of file

From 4b4913b963d6f12335cc5a3399815701647d242f Mon Sep 17 00:00:00 2001
From: Alex Beal <beala@stripe.com>
Date: Wed, 7 Aug 2019 17:36:02 -0600
Subject: [PATCH 2/2] Move passing manual tests to new directory

---
 manual_test/README.md                         |  1 +
 manual_test/scala_test_jvm_flags/BUILD        | 43 +++++++++++++
 .../scala_test_jvm_flags/EmptyTest.scala      |  9 +++
 manual_test/scalac_jvm_opts/BUILD             | 61 +++++++++++++++++++
 manual_test/scalac_jvm_opts/Empty.scala       |  3 +
 .../scalac_jvm_opts/test.proto                |  0
 .../scala_test_jvm_flags/BUILD                | 10 +--
 test_expect_failure/scalac_jvm_opts/BUILD     | 36 -----------
 test_rules_scala.sh                           | 10 +--
 9 files changed, 123 insertions(+), 50 deletions(-)
 create mode 100644 manual_test/README.md
 create mode 100644 manual_test/scala_test_jvm_flags/BUILD
 create mode 100644 manual_test/scala_test_jvm_flags/EmptyTest.scala
 create mode 100644 manual_test/scalac_jvm_opts/BUILD
 create mode 100644 manual_test/scalac_jvm_opts/Empty.scala
 rename {test_expect_failure => manual_test}/scalac_jvm_opts/test.proto (100%)

diff --git a/manual_test/README.md b/manual_test/README.md
new file mode 100644
index 000000000..acaefadf9
--- /dev/null
+++ b/manual_test/README.md
@@ -0,0 +1 @@
+This directory contains tests that require extra setup such as extra bazel flags.
\ No newline at end of file
diff --git a/manual_test/scala_test_jvm_flags/BUILD b/manual_test/scala_test_jvm_flags/BUILD
new file mode 100644
index 000000000..1012d6dd1
--- /dev/null
+++ b/manual_test/scala_test_jvm_flags/BUILD
@@ -0,0 +1,43 @@
+load("//scala:scala_toolchain.bzl", "scala_toolchain")
+load("//scala:scala.bzl", "scala_test")
+
+scala_toolchain(
+    name = "failing_toolchain_impl",
+    # This will fail because 1M isn't enough
+    scala_test_jvm_flags = ["-Xmx1M"],
+    visibility = ["//visibility:public"],
+)
+
+toolchain(
+    name = "failing_scala_toolchain",
+    toolchain = "failing_toolchain_impl",
+    toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
+    visibility = ["//visibility:public"],
+)
+
+scala_toolchain(
+    name = "passing_toolchain_impl",
+    # This will pass because 1G is enough
+    scala_test_jvm_flags = ["-Xmx1G"],
+    visibility = ["//visibility:public"],
+)
+
+toolchain(
+    name = "passing_scala_toolchain",
+    toolchain = "passing_toolchain_impl",
+    toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
+    visibility = ["//visibility:public"],
+)
+
+scala_test(
+    name = "empty_test",
+    srcs = ["EmptyTest.scala"],
+)
+
+scala_test(
+    name = "empty_overriding_test",
+    srcs = ["EmptyTest.scala"],
+    # This overrides the option passed in on the toolchain, and should BUILD, even if
+    # the `failing_scala_toolchain` is used.
+    jvm_flags = ["-Xmx1G"],
+)
diff --git a/manual_test/scala_test_jvm_flags/EmptyTest.scala b/manual_test/scala_test_jvm_flags/EmptyTest.scala
new file mode 100644
index 000000000..d1fbfc7a0
--- /dev/null
+++ b/manual_test/scala_test_jvm_flags/EmptyTest.scala
@@ -0,0 +1,9 @@
+package test_expect_failure.scala_test_jvm_flags
+
+import org.scalatest.FunSuite
+
+class EmptyTest extends FunSuite {
+    test("empty test") {
+        assert(true)
+    }
+}
\ No newline at end of file
diff --git a/manual_test/scalac_jvm_opts/BUILD b/manual_test/scalac_jvm_opts/BUILD
new file mode 100644
index 000000000..a9ec0370b
--- /dev/null
+++ b/manual_test/scalac_jvm_opts/BUILD
@@ -0,0 +1,61 @@
+load("//scala:scala_toolchain.bzl", "scala_toolchain")
+load("//scala:scala.bzl", "scala_library")
+load(
+    "//scala_proto:scala_proto.bzl",
+    "scalapb_proto_library",
+)
+
+scala_toolchain(
+    name = "failing_toolchain_impl",
+    # This will fail because 1M isn't enough
+    scalac_jvm_flags = ["-Xmx1M"],
+    visibility = ["//visibility:public"],
+)
+
+toolchain(
+    name = "failing_scala_toolchain",
+    toolchain = "failing_toolchain_impl",
+    toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
+    visibility = ["//visibility:public"],
+)
+
+scala_toolchain(
+    name = "passing_toolchain_impl",
+    # This will pass because 1G is enough
+    scalac_jvm_flags = ["-Xmx1G"],
+    visibility = ["//visibility:public"],
+)
+
+toolchain(
+    name = "passing_scala_toolchain",
+    toolchain = "passing_toolchain_impl",
+    toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
+    visibility = ["//visibility:public"],
+)
+
+scala_library(
+    name = "empty_build",
+    srcs = ["Empty.scala"],
+)
+
+scala_library(
+    name = "empty_overriding_build",
+    srcs = ["Empty.scala"],
+    # This overrides the option passed in on the toolchain, and should BUILD, even if
+    # the `failing_scala_toolchain` is used.
+    scalac_jvm_flags = ["-Xmx1G"],
+)
+
+proto_library(
+    name = "test",
+    srcs = ["test.proto"],
+    visibility = ["//visibility:public"],
+)
+
+# This is a regression test for a bug that broke compiling scalapb targets when
+# `scalac_jvm_flags` was set on the toolchain.
+scalapb_proto_library(
+    name = "proto",
+    visibility = ["//visibility:public"],
+    deps = [":test"],
+)
\ No newline at end of file
diff --git a/manual_test/scalac_jvm_opts/Empty.scala b/manual_test/scalac_jvm_opts/Empty.scala
new file mode 100644
index 000000000..691dbdd9b
--- /dev/null
+++ b/manual_test/scalac_jvm_opts/Empty.scala
@@ -0,0 +1,3 @@
+package test_expect_failure.scalac_jvm_opts
+
+class Empty
\ No newline at end of file
diff --git a/test_expect_failure/scalac_jvm_opts/test.proto b/manual_test/scalac_jvm_opts/test.proto
similarity index 100%
rename from test_expect_failure/scalac_jvm_opts/test.proto
rename to manual_test/scalac_jvm_opts/test.proto
diff --git a/test_expect_failure/scala_test_jvm_flags/BUILD b/test_expect_failure/scala_test_jvm_flags/BUILD
index f2bb261c2..34bb75d2b 100644
--- a/test_expect_failure/scala_test_jvm_flags/BUILD
+++ b/test_expect_failure/scala_test_jvm_flags/BUILD
@@ -32,12 +32,4 @@ toolchain(
 scala_test(
     name = "empty_test",
     srcs = ["EmptyTest.scala"],
-)
-
-scala_test(
-    name = "empty_overriding_test",
-    srcs = ["EmptyTest.scala"],
-    # This overrides the option passed in on the toolchain, and should BUILD, even if
-    # the `failing_scala_toolchain` is used.
-    jvm_flags = ["-Xmx1G"],
-)
+)
\ No newline at end of file
diff --git a/test_expect_failure/scalac_jvm_opts/BUILD b/test_expect_failure/scalac_jvm_opts/BUILD
index 1befa0af9..9eeca2daf 100644
--- a/test_expect_failure/scalac_jvm_opts/BUILD
+++ b/test_expect_failure/scalac_jvm_opts/BUILD
@@ -12,13 +12,6 @@ scala_toolchain(
     visibility = ["//visibility:public"],
 )
 
-scala_toolchain(
-    name = "passing_toolchain_impl",
-    # This will pass because 1G is enough
-    scalac_jvm_flags = ["-Xmx1G"],
-    visibility = ["//visibility:public"],
-)
-
 toolchain(
     name = "failing_scala_toolchain",
     toolchain = "failing_toolchain_impl",
@@ -26,36 +19,7 @@ toolchain(
     visibility = ["//visibility:public"],
 )
 
-toolchain(
-    name = "passing_scala_toolchain",
-    toolchain = "passing_toolchain_impl",
-    toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
-    visibility = ["//visibility:public"],
-)
-
 scala_library(
     name = "empty_build",
     srcs = ["Empty.scala"],
-)
-
-scala_library(
-    name = "empty_overriding_build",
-    srcs = ["Empty.scala"],
-    # This overrides the option passed in on the toolchain, and should BUILD, even if
-    # the `failing_scala_toolchain` is used.
-    scalac_jvm_flags = ["-Xmx1G"],
-)
-
-proto_library(
-    name = "test",
-    srcs = ["test.proto"],
-    visibility = ["//visibility:public"],
-)
-
-# This is a regression test for a bug that broke compiling scalapb targets when
-# `scalac_jvm_flags` was set on the toolchain.
-scalapb_proto_library(
-    name = "proto",
-    visibility = ["//visibility:public"],
-    deps = [":test"],
 )
\ No newline at end of file
diff --git a/test_rules_scala.sh b/test_rules_scala.sh
index a5f92eb92..9c2d41a59 100755
--- a/test_rules_scala.sh
+++ b/test_rules_scala.sh
@@ -830,15 +830,15 @@ test_scalac_jvm_flags_from_scala_toolchain_fails() {
 }
 
 test_scalac_jvm_flags_from_scala_toolchain_passes() {
-  bazel build --extra_toolchains="//test_expect_failure/scalac_jvm_opts:passing_scala_toolchain" //test_expect_failure/scalac_jvm_opts:empty_build
+  bazel build --extra_toolchains="//manual_test/scalac_jvm_opts:passing_scala_toolchain" //manual_test/scalac_jvm_opts:empty_build
 }
 
 test_scalac_jvm_flags_on_target_overrides_toolchain_passes() {
-  bazel build --extra_toolchains="//test_expect_failure/scalac_jvm_opts:failing_scala_toolchain" //test_expect_failure/scalac_jvm_opts:empty_overriding_build
+  bazel build --extra_toolchains="//manual_test/scalac_jvm_opts:failing_scala_toolchain" //manual_test/scalac_jvm_opts:empty_overriding_build
 }
 
 test_scalac_jvm_flags_work_with_scalapb() {
-  bazel build --extra_toolchains="//test_expect_failure/scalac_jvm_opts:passing_scala_toolchain" //test_expect_failure/scalac_jvm_opts:proto
+  bazel build --extra_toolchains="//manual_test/scalac_jvm_opts:passing_scala_toolchain" //manual_test/scalac_jvm_opts:proto
 }
 
 test_scala_test_jvm_flags_from_scala_toolchain_fails() {
@@ -846,11 +846,11 @@ test_scala_test_jvm_flags_from_scala_toolchain_fails() {
 }
 
 test_scala_test_jvm_flags_from_scala_toolchain_passes() {
-  bazel test --extra_toolchains="//test_expect_failure/scala_test_jvm_flags:passing_scala_toolchain" //test_expect_failure/scala_test_jvm_flags:empty_test
+  bazel test --extra_toolchains="//manual_test/scala_test_jvm_flags:passing_scala_toolchain" //manual_test/scala_test_jvm_flags:empty_test
 }
 
 test_scala_test_jvm_flags_on_target_overrides_toolchain_passes() {
-  bazel test --extra_toolchains="//test_expect_failure/scala_test_jvm_flags:failing_scala_toolchain" //test_expect_failure/scala_test_jvm_flags:empty_overriding_test
+  bazel test --extra_toolchains="//manual_test/scala_test_jvm_flags:failing_scala_toolchain" //manual_test/scala_test_jvm_flags:empty_overriding_test
 }
 
 test_unused_dependency_checker_mode_set_in_rule() {