Require JavaInfo on deps, move scrooge srcjar to srcs (#523)

* move scrooge srcjar to srcs

* require JavaInfo, add a test to avoid thrift jars on classpath

* fix strict build, run lint fix

* address review comments

* merge master, fix tests

* use JavaInfo directly

* fix tests, take N

* fix take N+1, update README, make runtime_deps strict

README.md

@@ -104,21 +104,23 @@ In order to make a java rule use this jar file, use the `java_import` rule.
       <td>
         <p><code>List of labels, required</code></p>
         <p>List of Scala <code>.scala</code> source files used to build the
-        library</p>
+        library. These may be .srcjar jar files that contain source code.</p>
       </td>
     </tr>
     <tr>
       <td><code>deps</code></td>
       <td>
         <p><code>List of labels, optional</code></p>
-        <p>List of other libraries to linked to this library target</p>
+        <p>List of other libraries to linked to this library target.
+        These must be jvm targets (scala_library, java_library, java_import, etc...)</p>
       </td>
     </tr>
     <tr>
       <td><code>runtime_deps</code></td>
       <td>
         <p><code>List of labels, optional</code></p>
-        <p>List of other libraries to put on the classpath only at runtime. This is rarely needed in Scala.</p>
+        <p>List of other libraries to put on the classpath only at runtime. This is rarely needed in Scala.
+        These must be jvm targets (scala_library, java_library, java_import, etc...)</p>
       </td>
     </tr>
     <tr>
@@ -127,7 +129,8 @@ In order to make a java rule use this jar file, use the `java_import` rule.
         <p><code>List of labels, optional</code></p>
         <p>List of targets to add to the dependencies of those that depend on this target. Similar
         to the `java_library` parameter of the same name. Use this sparingly as it weakens the
-        precision of the build graph.</p>
+        precision of the build graph.
+        These must be jvm targets (scala_library, java_library, java_import, etc...)</p>
       </td>
     </tr>
     <tr>
@@ -260,14 +263,16 @@ A `scala_binary` requires a `main_class` attribute.
       <td><code>deps</code></td>
       <td>
         <p><code>List of labels, optional</code></p>
-        <p>List of other libraries to linked to this binary target</p>
+        <p>List of other libraries to linked to this binary target.
+        These must be jvm targets (scala_library, java_library, java_import, etc...)</p>
       </td>
     </tr>
     <tr>
       <td><code>runtime_deps</code></td>
       <td>
         <p><code>List of labels, optional</code></p>
-        <p>List of other libraries to put on the classpath only at runtime. This is rarely needed in Scala.</p>
+        <p>List of other libraries to put on the classpath only at runtime. This is rarely needed in Scala.
+        These must be jvm targets (scala_library, java_library, java_import, etc...)</p>
       </td>
     </tr>
     <tr>

scala/BUILD

@@ -17,3 +17,9 @@ toolchain(
     toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
     visibility = ["//visibility:public"],
 )
+
+java_import(
+    name = "bazel_test_runner_deploy",
+    jars = ["@bazel_tools//tools/jdk:TestRunner_deploy.jar"],
+    visibility = ["//visibility:public"],
+)

scala/private/common.bzl

@@ -26,15 +26,14 @@ def _collect_jars_when_dependency_analyzer_is_off(dep_targets):
   runtime_jars = []
 
   for dep_target in dep_targets:
+    # we require a JavaInfo for dependencies
+    # must use java_import or scala_import if you have raw files
     if JavaInfo in dep_target:
       java_provider = dep_target[JavaInfo]
       compile_jars.append(java_provider.compile_jars)
       runtime_jars.append(java_provider.transitive_runtime_jars)
     else:
-      # support http_file pointed at a jar. http_jar uses ijar,
-      # which breaks scala macros
-      compile_jars.append(filter_not_sources(dep_target.files))
-      runtime_jars.append(filter_not_sources(dep_target.files))
+      print("ignored dependency, has no JavaInfo: " + str(dep_target))
 
   return struct(
       compile_jars = depset(transitive = compile_jars),
@@ -49,26 +48,21 @@ def _collect_jars_when_dependency_analyzer_is_on(dep_targets):
   runtime_jars = []
 
   for dep_target in dep_targets:
-    current_dep_compile_jars = None
-    current_dep_transitive_compile_jars = None
-
+    # we require a JavaInfo for dependencies
+    # must use java_import or scala_import if you have raw files
     if JavaInfo in dep_target:
       java_provider = dep_target[JavaInfo]
       current_dep_compile_jars = java_provider.compile_jars
       current_dep_transitive_compile_jars = java_provider.transitive_compile_time_jars
       runtime_jars.append(java_provider.transitive_runtime_jars)
+
+      compile_jars.append(current_dep_compile_jars)
+      transitive_compile_jars.append(current_dep_transitive_compile_jars)
+      add_labels_of_jars_to(jars2labels, dep_target,
+                            current_dep_transitive_compile_jars.to_list(),
+                            current_dep_compile_jars.to_list())
     else:
-      # support http_file pointed at a jar. http_jar uses ijar,
-      # which breaks scala macros
-      current_dep_compile_jars = filter_not_sources(dep_target.files)
-      current_dep_transitive_compile_jars = filter_not_sources(dep_target.files)
-      runtime_jars.append(filter_not_sources(dep_target.files))
-
-    compile_jars.append(current_dep_compile_jars)
-    transitive_compile_jars.append(current_dep_transitive_compile_jars)
-    add_labels_of_jars_to(jars2labels, dep_target,
-                          current_dep_transitive_compile_jars.to_list(),
-                          current_dep_compile_jars.to_list())
+      print("ignored dependency, has no JavaInfo: " + str(dep_target))
 
   return struct(
       compile_jars = depset(transitive = compile_jars),

scala/private/rule_impls.bzl

@@ -457,12 +457,7 @@ def _collect_runtime_jars(dep_targets):
   runtime_jars = []
 
   for dep_target in dep_targets:
-    if JavaInfo in dep_target:
-      runtime_jars.append(dep_target[JavaInfo].transitive_runtime_jars)
-    else:
-      # support http_file pointed at a jar. http_jar uses ijar,
-      # which breaks scala macros
-      runtime_jars.append(filter_not_sources(dep_target.files))
+    runtime_jars.append(dep_target[JavaInfo].transitive_runtime_jars)
 
   return runtime_jars
 
@@ -562,7 +557,9 @@ def _lib(ctx, non_macro_lib):
       # this information through. extra_information allows passing
       # this information through, and it is up to the new_targets
       # to filter and make sense of this information.
-      extra_information=_collect_extra_information(ctx.attr.deps),
+      # unfortunately, we need to see this for scrooge and protobuf to work,
+      # but those are generating srcjar, so they should really come in via srcs
+      extra_information=_collect_extra_information(ctx.attr.deps + ctx.attr.srcs),
       jars_to_labels = jars.jars2labels,
     )
 

scala/scala.bzl

@@ -102,7 +102,7 @@ _common_attrs_for_plugin_bootstrapping = {
     "srcs": attr.label_list(allow_files = [".scala", ".srcjar", ".java"]),
     "deps": attr.label_list(),
     "plugins": attr.label_list(allow_files = [".jar"]),
-    "runtime_deps": attr.label_list(),
+    "runtime_deps": attr.label_list(providers = [[JavaInfo]]),
     "data": attr.label_list(allow_files = True, cfg = "data"),
     "resources": attr.label_list(allow_files = True),
     "resource_strip_prefix": attr.string(),
@@ -219,13 +219,10 @@ _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"),
-        allow_files = True),
+        default = Label("//src/java/io/bazel/rulesscala/scala_test:runner")),
     "_scalatest_reporter": attr.label(
         default = Label("//scala/support:test_reporter")),
 }
@@ -476,7 +473,8 @@ _scala_junit_test_attrs = {
             "//external:io_bazel_rules_scala/dependency/hamcrest/hamcrest_core")
     ),
     "_bazel_test_runner": attr.label(
-        default = Label("@bazel_tools//tools/jdk:TestRunner_deploy.jar"),
+        default = Label(
+            "@io_bazel_rules_scala//scala:bazel_test_runner_deploy"),
         allow_files = True),
 }
 _scala_junit_test_attrs.update(_launcher_template)

src/java/io/bazel/rulesscala/scala_test/BUILD

@@ -1,4 +1,4 @@
-java_binary(
+java_library(
     name = "runner",
     srcs = ["Runner.java"],
     visibility = ["//visibility:public"],

test/src/main/scala/scalarules/test/sources_jars_in_deps/BUILD

@@ -4,6 +4,6 @@ scala_library(
     name = "source_jar_not_oncp",
     srcs = ["ReferCatsImplicits.scala"],
     deps = [
-        "@org_typelevel__cats_core//jar:file",
+        "@org_typelevel__cats_core//jar",
     ],
 )

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

@@ -190,3 +190,13 @@ scala_binary(
     main_class = "scalarules.test.twitter_scrooge.BareThrifts",
     deps = [":bare_thrift_scrooge"],
 )
+
+sh_test(
+    name = "libthrift2_a_not_on_classpath",
+    srcs = ["grep.sh"],
+    args = [
+        "libthrift2_a",
+        "$(location :justscrooges)",
+    ],
+    data = [":justscrooges"],
+)

test/src/main/scala/scalarules/test/twitter_scrooge/grep.sh

@@ -0,0 +1,13 @@
+#!/bin/sh
+
+
+# this is used by a sh_test to test that a string is NOT
+# found in a file. This is used for instance to check if
+# stray jars are not making it onto the classpath
+
+if grep -q $1 $2 ; then
+  echo "ERROR: found $1"
+  exit 1
+else
+  exit 0
+fi

test_expect_failure/missing_direct_deps/external_deps/BUILD

@@ -31,5 +31,5 @@ scala_library(
     srcs = [
         "B.scala",
     ],
-    deps = ["@com_google_guava_guava_21_0_with_file//jar:file"],
+    deps = ["@com_google_guava_guava_21_0_with_file//jar"],
 )

test_expect_failure/missing_direct_deps/internal_deps/custom-jvm-rule.bzl

@@ -12,8 +12,7 @@ def _custom_jvm_impl(ctx):
 def _collect(deps):
   transitive_compile_jars = depset()
   for dep_target in deps:
-    transitive_compile_jars += dep_target[
-        java_common.provider].transitive_compile_time_jars
+    transitive_compile_jars += dep_target[JavaInfo].transitive_compile_time_jars
   return transitive_compile_jars
 
 custom_jvm = rule(

test_rules_scala.sh

@@ -159,7 +159,7 @@ test_scala_library_expect_failure_on_missing_direct_external_deps_jar() {
 }
 
 test_scala_library_expect_failure_on_missing_direct_external_deps_file_group() {
-  dependenecy_target='@com_google_guava_guava_21_0_with_file//jar:file'
+  dependenecy_target='@com_google_guava_guava_21_0_with_file//jar:jar'
   test_target='test_expect_failure/missing_direct_deps/external_deps:transitive_external_dependency_user_file_group'
 
   test_scala_library_expect_failure_on_missing_direct_deps $dependenecy_target $test_target

twitter_scrooge/twitter_scrooge.bzl

@@ -289,12 +289,11 @@ def scrooge_scala_library(name,
       with_finagle = with_finagle,
   )
 
-  # deps from macro invocation would come via srcjar
-  # however, retained to make dependency analysis via aspects easier
   scala_library(
       name = name,
+      srcs = [srcjar],
       deps = deps + remote_jars + [
-          srcjar, "//external:io_bazel_rules_scala/dependency/thrift/libthrift",
+          "//external:io_bazel_rules_scala/dependency/thrift/libthrift",
           "//external:io_bazel_rules_scala/dependency/thrift/scrooge_core"
       ],
       exports = deps + remote_jars + [