Merge pull request #68 from colinmarc/master

Don't allow transitive scala_library dependencies onto the classpath

scala/scala.bzl

@@ -330,7 +330,7 @@ def _lib(ctx, non_macro_lib):
   texp = _collect_jars(ctx.attr.exports)
   scalaattr = struct(outputs = rule_outputs,
                      transitive_runtime_deps = rjars,
-                     transitive_compile_exports = texp.compiletime + cjars,
+                     transitive_compile_exports = texp.compiletime,
                      transitive_runtime_exports = texp.runtime
                      )
   runfiles = ctx.runfiles(

test/A.scala

@@ -4,4 +4,4 @@ object A {
   def main(args: Array[String]) {
     println("4 8 15 16 23 42")
   }
-}
+}

test/BUILD

@@ -65,7 +65,8 @@ scala_library(
     deps = [
         "OtherJavaLib",
         "OtherLib",
-        "MacroTest"
+        "MacroTest",
+        "Exported"
     ],
 )
 
@@ -87,31 +88,10 @@ scala_repl(
     name = "HelloLibTestRepl",
     deps = [":HelloLibTest"])
 
-scala_library(
-    name = "a",
-    srcs = ["A.scala"]
-)
-
-scala_library(
-    name = "b",
-    exports = [":a"],
-)
-
-scala_library(
-    name = "c",
-    deps = [":b"],
-)
-
-scala_library(
-    name = "d",
-    deps = [":c"],
-    srcs = ["D.scala"],
-)
-
 scala_library(
     name = "OtherLib",
     srcs = ["OtherLib.scala"],
-    exports = ["ExportOnly"],  # test of exported target
+    deps = ["ExportOnly"],
 )
 
 # Test of library without src

test/D.scala

@@ -4,4 +4,4 @@ object D {
   def main(args: Array[String]) {
     A.main(args)
   }
-}
+}

test/HelloLibTest.scala

@@ -22,7 +22,7 @@ object TestUtil {
 
 class ScalaSuite extends FlatSpec {
   "HelloLib" should "call scala" in {
-    assert(HelloLib.getOtherLibMessage("hello").equals("hello scala!"))
+    assert(HelloLib.getOtherLibMessage("hello").equals("hello you all, everybody. I am Runtime"))
   }
 }
 

test/OtherLib.scala

@@ -16,9 +16,5 @@ package scala.test
 
 // It is just to show how a Scala library can depend on another Scala library.
 object OtherLib {
-  def getMessage(): String = {
-    // This won't compile because Exported is exported, not a dep:
-    // Exported.message
-    "scala!"
-  }
+  def getMessage(): String = Exported.message
 }

test/src/main/scala/scala/test/disappearing_class/ClassProvider.scala

@@ -1,8 +1,3 @@
 package scala.test
 
-object ClassProvider {
-  def dissappearingClassMethod: String = "testContents"
-}
-
-
 object BackgroundNoise{}

test/src/main/scala/scala/test/twitter_scrooge/BUILD

@@ -50,6 +50,7 @@ scala_library(
   name = "justscrooge1",
   srcs = ["JustScrooge1.scala"],
   deps = [":scrooge1"],
+  exports = [":scrooge1"]
 )
 
 scala_library(
@@ -68,6 +69,7 @@ scala_library(
   name = "justscrooge3",
   srcs = ["JustScrooge3.scala"],
   deps = [":scrooge3"],
+  exports = [":scrooge3"],
 )
 
 scala_library(
@@ -107,7 +109,7 @@ scala_binary(
 
 scala_library(
   name = "allscrooges",
-  deps = [
+  exports = [
     ":scrooge1",
     ":scrooge2_a",
     ":scrooge2_b",
@@ -120,4 +122,4 @@ scala_binary(
     srcs = ["JustScrooge1.scala"],
     deps = [":allscrooges"],
     main_class = "scala.test.twitter_scrooge.JustScrooge1",
-)
+)

test_expect_failure/disappearing_class/ClassProvider.scala

@@ -0,0 +1,8 @@
+package scala.test
+
+object ClassProvider {
+  def dissappearingClassMethod: String = "testContents"
+}
+
+
+object BackgroundNoise{}

test_expect_failure/transitive/java_to_scala/A.scala

@@ -0,0 +1,7 @@
+package example
+
+object A {
+  def foo {
+    println("hi")
+  }
+}

test_expect_failure/transitive/java_to_scala/BUILD

@@ -0,0 +1,24 @@
+load("//scala:scala.bzl", "scala_library", "scala_export_to_java")
+
+scala_library(
+    name = "a",
+    srcs = ["A.scala"]
+)
+
+scala_export_to_java(
+    name = "b",
+    exports = [":a"],
+    runtime_deps = []
+)
+
+java_library(
+    name = "c",
+    srcs = ["C.java"],
+    deps = [":b"],
+)
+
+java_library(
+    name = "d",
+    srcs = ["D.java"],
+    deps = [":c"],
+)

test_expect_failure/transitive/java_to_scala/C.java

@@ -0,0 +1,3 @@
+package example;
+
+public class C {}

test_expect_failure/transitive/java_to_scala/D.java

@@ -0,0 +1,7 @@
+package example;
+
+public class D {
+    public static void main(String[] args) {
+        A.foo();
+    }
+}

test_expect_failure/transitive/scala_to_java/A.java

@@ -0,0 +1,7 @@
+package example;
+
+public class A {
+  public static void foo() {
+    System.out.println("yeah?");
+  }
+}

test_expect_failure/transitive/scala_to_java/BUILD

@@ -0,0 +1,22 @@
+load("//scala:scala.bzl", "scala_library")
+
+java_library(
+    name = "a",
+    srcs = ["A.java"]
+)
+
+scala_library(
+    name = "b",
+    exports = [":a"],
+)
+
+scala_library(
+    name = "c",
+    deps = [":b"],
+)
+
+scala_library(
+    name = "d",
+    deps = [":c"],
+    srcs = ["D.scala"],
+)

test_expect_failure/transitive/scala_to_java/D.scala

@@ -0,0 +1,7 @@
+package example
+
+object D {
+  def main(args: Array[String]) {
+    A.foo
+  }
+}

test_expect_failure/transitive/scala_to_scala/A.scala

@@ -0,0 +1,7 @@
+package example
+
+object A {
+  def foo {
+    println("hi")
+  }
+}

test_expect_failure/transitive/scala_to_scala/BUILD

@@ -0,0 +1,22 @@
+load("//scala:scala.bzl", "scala_library")
+
+scala_library(
+    name = "a",
+    srcs = ["A.scala"]
+)
+
+scala_library(
+    name = "b",
+    exports = [":a"],
+)
+
+scala_library(
+    name = "c",
+    deps = [":b"],
+)
+
+scala_library(
+    name = "d",
+    deps = [":c"],
+    srcs = ["D.scala"],
+)

test_expect_failure/transitive/scala_to_scala/D.scala

@@ -0,0 +1,7 @@
+package example
+
+object D {
+  def main(args: Array[String]) {
+    A.foo
+  }
+}

test_run.sh

@@ -3,13 +3,13 @@
 set -e
 
 test_disappearing_class() {
-  git checkout test/src/main/scala/scala/test/disappearing_class/ClassProvider.scala
-  bazel build test/src/main/scala/scala/test/disappearing_class:uses_class
-  echo -e "package scala.test\n\nobject BackgroundNoise{}" > test/src/main/scala/scala/test/disappearing_class/ClassProvider.scala
+  git checkout test_expect_failure/disappearing_class/ClassProvider.scala
+  bazel build test_expect_failure/disappearing_class:uses_class
+  echo -e "package scala.test\n\nobject BackgroundNoise{}" > test_expect_failure/disappearing_class/ClassProvider.scala
   set +e
-  bazel build test/src/main/scala/scala/test/disappearing_class:uses_class
+  bazel build test_expect_failure/disappearing_class:uses_class
   RET=$?
-  git checkout test/src/main/scala/scala/test/disappearing_class/ClassProvider.scala
+  git checkout test_expect_failure/disappearing_class/ClassProvider.scala
   if [ $RET -eq 0 ]; then
     echo "Class caching at play. This should fail"
     exit 1
@@ -18,14 +18,39 @@ test_disappearing_class() {
 }
 
 test_build_is_identical() {
-  bazel build test/... 
+  bazel build test/...
   md5sum bazel-bin/test/*.jar > hash1
   bazel clean
-  bazel build test/... 
+  bazel build test/...
   md5sum bazel-bin/test/*.jar > hash2
   diff hash1 hash2
 }
 
+test_transitive_deps() {
+  set +e
+
+  bazel build test_expect_failure/transitive/scala_to_scala/...
+  if [ $? -eq 0 ]; then
+    echo "'bazel build test_expect_failure/transitive/scala_to_scala/...' should have failed."
+    exit 1
+  fi
+
+  bazel build test_expect_failure/transitive/java_to_scala/...
+  if [ $? -eq 0 ]; then
+    echo "'bazel build test_expect_failure/transitive/java_to_scala/...' should have failed."
+    exit 1
+  fi
+
+  bazel build test_expect_failure/transitive/scala_to_java/...
+  if [ $? -eq 0 ]; then
+    echo "'bazel build test_transitive_deps/scala_to_java/...' should have failed."
+    exit 1
+  fi
+
+  set -e
+  exit 0
+}
+
 test_repl() {
   echo "import scala.test._; HelloLib.printMessage(\"foo\")" | bazel-bin/test/HelloLibRepl | grep "foo scala" &&
   echo "import scala.test._; TestUtil.foo" | bazel-bin/test/HelloLibTestRepl | grep "bar" &&
@@ -43,5 +68,6 @@ bazel build test/... \
   && (find -L ./bazel-testlogs -iname "*.xml" | xargs -n1 xmllint > /dev/null) \
   && test_disappearing_class \
   && test_build_is_identical \
+  && test_transitive_deps \
   && test_repl \
   && echo "all good"

twitter_scrooge/twitter_scrooge.bzl

@@ -74,7 +74,7 @@ def _assert_set_is_subset(left, right):
       missing += [l]
   if len(missing) > 0:
     fail('scrooge_srcjar target must depend on scrooge_srcjar targets sufficient to ' +
-         'cover the transitive graph of thrift files. Uncovered sources: ' + missing)
+         'cover the transitive graph of thrift files. Uncovered sources: ' + str(missing))
 
 def _path_newline(data):
   return '\n'.join([f.path for f in data])
@@ -201,23 +201,25 @@ scrooge_scala_srcjar = rule(
 )
 
 def scrooge_scala_library(name, deps=[], remote_jars=[], jvm_flags=[], visibility=None):
-  scrooge_scala_srcjar(
-    name = name + '_srcjar',
-    deps = deps,
-    remote_jars = remote_jars,
-    visibility = visibility,
-  )
-  scala_library(
-    name = name,
-    deps = remote_jars + [
-      name + '_srcjar',
-      "@libthrift//jar",
-      "@scrooge_core//jar",
-    ],
-    exports = [
-      "@libthrift//jar",
-      "@scrooge_core//jar",
-    ],
-    jvm_flags = jvm_flags,
-    visibility = visibility,
-  )
+    srcjar = name + '_srcjar'
+    scrooge_scala_srcjar(
+        name = srcjar,
+        deps = deps,
+        remote_jars = remote_jars,
+        visibility = visibility,
+    )
+
+    scala_library(
+        name = name,
+        deps = deps + remote_jars + [
+            srcjar,
+            "@libthrift//jar",
+            "@scrooge_core//jar"
+        ],
+        exports = deps + remote_jars + [
+            "@libthrift//jar",
+            "@scrooge_core//jar",
+        ],
+        jvm_flags = jvm_flags,
+        visibility = visibility,
+    )