Restructure scala provider, add JavaProvider

The key change is that the scala provider has been completely
restructured to match the structure of the JavaProvider. It no longer
tracks exports separately but instead has the following fields:

* outputs = contains all the outputs generated by the current rule
(nothing or ijar and class_jar); however, no rules here actually use it.

* compile_jars = contains all the jars dependent rules should compile
with. Thus, ijar and exports.compile_jars

* transitive_runtime_jars = contains all the jars needed to handle this
target at runtime. Thus, class_jar plus (deps + exports +
runtime_deps).transitive_runtime_jars

The created java provider (used by dependent native java rules) uses
just compile_jars and transitive_runtime_jars.

In general, this change was seamless. The major exception is if you were
specifically relying on only exports being re-exported by specifically
gen_scrooge_srcjar. Beforehand, any jars produced by direct dependencies
were not being exported to dependents but now they are. Note that, if
you were using scrooge_scala_library instead, nothing should change.

Other changes:
* Cleanup an issue where uses of _hamcrest and _junit were not picking
up all potential runtime/compile time jars in the cases where those
labels were retarget to a java_library or java_import instead of just
being a raw file. (Really, need to consider auditing ALL use of raw jars
to potentially be cleaned up.)

* Use depset instead of set. (set docs already say set should be
considered deprecated and just points to depset anyway)

jmh/jmh.bzl

@@ -66,8 +66,7 @@ def _scala_construct_runtime_classpath(deps):
   java_targets = [d.java for d in deps if hasattr(d, "java")]
   files = []
   for scala in scala_targets:
-    files += list(scala.transitive_runtime_deps)
-    files += list(scala.transitive_runtime_exports)
+    files += list(scala.transitive_runtime_jars)
   for java in java_targets:
     files += list(java.transitive_runtime_deps)
   return files

scala/scala.bzl

@@ -76,6 +76,7 @@ def _get_jar_path(paths):
 
 
 def _build_nosrc_jar(ctx, buildijar):
+    # No scala compile needed, so just build a jar of resources
     cp_resources = _add_resources_cmd(ctx, "{out}_tmp".format(
       out=ctx.outputs.jar.path)
     )
@@ -133,8 +134,7 @@ def _collect_plugin_paths(plugins):
     return paths
 
 
-def _compile(ctx, _jars, dep_srcjars, buildijar):
-    jars = _jars
+def _compile(ctx, cjars, dep_srcjars, buildijar):
     ijar_output_path = ""
     ijar_cmd_path = ""
     if buildijar:
@@ -153,7 +153,7 @@ def _compile(ctx, _jars, dep_srcjars, buildijar):
         scalalib=ctx.file._scalalib.path,
         scalacompiler=ctx.file._scalacompiler.path,
         scalareflect=ctx.file._scalareflect.path,
-        jars=":".join([j.path for j in jars]),
+        jars=":".join([j.path for j in cjars]),
     )
 
     scalac_args = """
@@ -212,7 +212,7 @@ SourceJars: {srcjars}
     # _jdk added manually since _java doesn't currently setup runfiles
     # _scalac, as a java_binary, should already have it in its runfiles; however,
     # adding does ensure _java not orphaned if _scalac ever was not a java_binary
-    ins = (list(jars) +
+    ins = (list(cjars) +
            list(dep_srcjars) +
            list(srcjars) +
            list(sources) +
@@ -353,71 +353,85 @@ def collect_srcjars(targets):
 
 def _collect_jars(targets):
     """Compute the runtime and compile-time dependencies from the given targets"""  # noqa
-    compile_jars = set()
-    runtime_jars = set()
+    compile_jars = depset()
+    runtime_jars = depset()
     for target in targets:
-        found = False
         if hasattr(target, "scala"):
-            if hasattr(target.scala.outputs, "ijar"):
-                compile_jars += [target.scala.outputs.ijar]
-            compile_jars += target.scala.transitive_compile_exports
-            runtime_jars += target.scala.transitive_runtime_deps
-            runtime_jars += target.scala.transitive_runtime_exports
-            found = True
-        if hasattr(target, "java"):
-            # see JavaSkylarkApiProvider.java,
-            # this is just the compile-time deps
-            # this should be improved in bazel 0.1.5 to get outputs.ijar
-            # compile_jars += [output.ijar for output in target.java.outputs.jars]
-            compile_jars += target.java.transitive_deps
-            runtime_jars += target.java.transitive_runtime_deps
-            found = True
-        if not found:
+            compile_jars += target.scala.compile_jars
+            runtime_jars += target.scala.transitive_runtime_jars
+        elif java_common.provider in target:
+            java_provider = target[java_common.provider]
+            compile_jars += target.java.transitive_deps #java_provider.compile_jars
+            runtime_jars += java_provider.transitive_runtime_jars
+        else:
             # support http_file pointed at a jar. http_jar uses ijar,
             # which breaks scala macros
-            runtime_jars += target.files
             compile_jars += target.files
+            runtime_jars += target.files
 
-    return struct(compiletime = compile_jars, runtime = runtime_jars)
+    return struct(compile_jars = compile_jars, transitive_runtime_jars = runtime_jars)
 
 
 def _lib(ctx, non_macro_lib):
+    # Build up information from dependency-like attributes
+
     # This will be used to pick up srcjars from non-scala library
     # targets (like thrift code generation)
-    srcjars = collect_srcjars(ctx.attr.deps)
-    jars = _collect_jars(ctx.attr.deps)
-    (cjars, rjars) = (jars.compiletime, jars.runtime)
-    write_manifest(ctx)
-    outputs = _compile_or_empty(ctx, cjars, srcjars, non_macro_lib)
-
-    rjars += [ctx.outputs.jar]
-    rjars += _collect_jars(ctx.attr.runtime_deps).runtime
+    deps_srcjars = collect_srcjars(ctx.attr.deps)
+    deps_jars = _collect_jars(ctx.attr.deps)
+    (cjars, transitive_rjars) = (deps_jars.compile_jars, deps_jars.transitive_runtime_jars)
+    transitive_rjars += _collect_jars(ctx.attr.runtime_deps).transitive_runtime_jars
 
-    rjars += [ctx.file._scalalib, ctx.file._scalareflect]
-    if not non_macro_lib:
-        #  macros need the scala reflect jar
-        rjars += [ctx.file._scalareflect]
+    # Add requisite scala libraries (e.g., if going to java)
+    transitive_rjars += [ctx.file._scalalib, ctx.file._scalareflect]
 
-    _build_deployable(ctx, rjars)
-    rule_outputs = struct(ijar=outputs.ijar, class_jar=outputs.class_jar, deploy_jar=ctx.outputs.deploy_jar)
+    # Execute compile actions
+    write_manifest(ctx)
+    outputs = _compile_or_empty(ctx, cjars, deps_srcjars, non_macro_lib)
+    deployable_rjars = transitive_rjars + [outputs.class_jar]
+    _build_deployable(ctx, deployable_rjars)
 
-    texp = _collect_jars(ctx.attr.exports)
-    scalaattr = struct(outputs=rule_outputs,
-                       transitive_runtime_deps=rjars,
-                       transitive_compile_exports=texp.compiletime,
-                       transitive_runtime_exports=texp.runtime
-                       )
+    # Now, need to setup providers for dependents
+    # Notice that transitive_rjars just carries over from dependency analysis
+    # but cjars 'resets'
+    next_cjars = depset([outputs.ijar])  # use ijar, if available, for future compiles
+    transitive_rjars += [outputs.class_jar]  # need true jar at runtime
 
-    # Note that rjars already transitive so don't really
-    # need to use transitive_files with _get_all_runfiles
+    # Using transitive_files since deployable_rjars a depset and avoiding linearization
     runfiles = ctx.runfiles(
-        files=list(rjars),
-        collect_data=True)
+        transitive_files = deployable_rjars,
+        collect_data = True,
+    )
+
+    # Add information from exports (is key that AFTER all build actions/runfiles analysis)
+    # Notice that this is compile_jars is intentionally transitive for exports
+    exports_jars = _collect_jars(ctx.attr.exports)
+    next_cjars += exports_jars.compile_jars
+    transitive_rjars += exports_jars.transitive_runtime_jars
+
+    rule_outputs = struct(
+        ijar = outputs.ijar,
+        class_jar = outputs.class_jar,
+        deploy_jar = ctx.outputs.deploy_jar,
+    )
+    # Note that, internally, rules only care about compile_jars and transitive_runtime_jars
+    # in a similar manner as the java_library and JavaProvider
+    scalaattr = struct(
+        outputs = rule_outputs,
+        compile_jars = next_cjars,
+        transitive_runtime_jars = transitive_rjars,
+    )
+    # TODO(twigg): Linearization is concerning here.
+    java_provider = java_common.create_provider(
+        compile_time_jars = scalaattr.compile_jars.to_list(),
+        runtime_jars = scalaattr.transitive_runtime_jars.to_list(),
+    )
 
     return struct(
-        files=set([ctx.outputs.jar]),  # Here is the default output
-        scala=scalaattr,
-        runfiles=runfiles,
+        files = depset([ctx.outputs.jar]),  # Here is the default output
+        scala = scalaattr,
+        providers = [java_provider],
+        runfiles = runfiles,
         # This is a free monoid given to the graph for the purpose of
         # extensibility. This is necessary when one wants to create
         # new targets which want to leverage a scala_library. For example,
@@ -461,38 +475,39 @@ def _scala_binary_common(ctx, cjars, rjars):
       class_jar=outputs.class_jar,
       deploy_jar=ctx.outputs.deploy_jar,
   )
-  scalaattr = struct(outputs = rule_outputs,
-                     transitive_runtime_deps = rjars,
-                     transitive_compile_exports = set(),
-                     transitive_runtime_exports = set()
-                     )
+  scalaattr = struct(
+      outputs = rule_outputs,
+      compile_jars = depset([outputs.class_jar]),
+      transitive_runtime_jars = rjars,
+  )
   return struct(
       files=set([ctx.outputs.executable]),
       scala = scalaattr,
       runfiles=runfiles)
 
 def _scala_binary_impl(ctx):
-  jars = _collect_jars(ctx.attr.deps)
-  (cjars, rjars) = (jars.compiletime, jars.runtime)
+  deps_jars = _collect_jars(ctx.attr.deps)
+  (cjars, transitive_rjars) = (deps_jars.compile_jars, deps_jars.transitive_runtime_jars)
   cjars += [ctx.file._scalareflect]
-  rjars += [ctx.outputs.jar, ctx.file._scalalib, ctx.file._scalareflect]
-  rjars += _collect_jars(ctx.attr.runtime_deps).runtime
+  transitive_rjars += [ctx.outputs.jar, ctx.file._scalalib, ctx.file._scalareflect]
+  transitive_rjars += _collect_jars(ctx.attr.runtime_deps).transitive_runtime_jars
+
   _write_launcher(
       ctx = ctx,
-      rjars = rjars,
+      rjars = transitive_rjars,
       main_class = ctx.attr.main_class,
       jvm_flags = ctx.attr.jvm_flags,
       args = "",
       run_before_binary = "",
       run_after_binary = "",
   )
-  return _scala_binary_common(ctx, cjars, rjars)
+  return _scala_binary_common(ctx, cjars, transitive_rjars)
 
 def _scala_repl_impl(ctx):
-  jars = _collect_jars(ctx.attr.deps)
-  rjars = jars.runtime
+  deps_jars = _collect_jars(ctx.attr.deps)
+  rjars = deps_jars.transitive_runtime_jars
   rjars += [ctx.file._scalalib, ctx.file._scalareflect, ctx.file._scalacompiler]
-  rjars += _collect_jars(ctx.attr.runtime_deps).runtime
+  rjars += _collect_jars(ctx.attr.runtime_deps).transitive_runtime_jars
 
   args = " ".join(ctx.attr.scalacopts)
   _write_launcher(
@@ -533,16 +548,16 @@ def _scala_test_impl(ctx):
         )
     deps = ctx.attr.deps
     deps += [ctx.attr._scalatest_reporter]
-    jars = _collect_jars(deps)
-    (cjars, rjars) = (jars.compiletime, jars.runtime)
+    deps_jars = _collect_jars(deps)
+    (cjars, rjars) = (deps_jars.compile_jars, deps_jars.transitive_runtime_jars)
     cjars += [ctx.file._scalareflect, ctx.file._scalatest, ctx.file._scalaxml]
     rjars += [ctx.outputs.jar,
               ctx.file._scalalib,
               ctx.file._scalareflect,
               ctx.file._scalatest,
               ctx.file._scalaxml
               ]
-    rjars += _collect_jars(ctx.attr.runtime_deps).runtime
+    rjars += _collect_jars(ctx.attr.runtime_deps).transitive_runtime_jars
 
     args = " ".join([
         "-R \"{path}\"".format(path=ctx.outputs.jar.short_path),
@@ -572,14 +587,14 @@ def _scala_junit_test_impl(ctx):
     if (not(ctx.attr.prefixes) and not(ctx.attr.suffixes)):
       fail("Setting at least one of the attributes ('prefixes','suffixes') is required")
     deps = ctx.attr.deps + [ctx.attr._suite]
-    jars = _collect_jars(deps)
-    (cjars, rjars) = (jars.compiletime, jars.runtime)
+    deps_jars = _collect_jars(deps)
+    (cjars, rjars) = (deps_jars.compile_jars, deps_jars.transitive_runtime_jars)
     junit_deps = [ctx.file._junit,ctx.file._hamcrest]
     cjars += junit_deps
     rjars += [ctx.outputs.jar,
               ctx.file._scalalib
               ] + junit_deps
-    rjars += _collect_jars(ctx.attr.runtime_deps).runtime
+    rjars += _collect_jars(ctx.attr.runtime_deps).transitive_runtime_jars
     test_suite = _gen_test_suite_flags_based_on_prefixes_and_suffixes(ctx, ctx.outputs.jar)
     launcherJvmFlags = ["-ea", test_suite.archiveFlag, test_suite.prefixesFlag, test_suite.suffixesFlag, test_suite.printFlag]
     _write_launcher(

twitter_scrooge/twitter_scrooge.bzl

@@ -153,13 +153,13 @@ def _gen_scrooge_srcjar_impl(ctx):
     arguments=["--jvm_flag=%s" % flag for flag in ctx.attr.jvm_flags] + ["@" + argfile.path],
   )
 
-  jars = _collect_scalaattr(ctx.attr.deps)
+  deps_jars = _collect_scalaattr(ctx.attr.deps)
 
-  scalaattr = struct(outputs = None,
-                     transitive_runtime_deps = jars.transitive_runtime_deps,
-                     transitive_compile_exports = jars.transitive_compile_exports,
-                     transitive_runtime_exports = jars.transitive_runtime_exports,
-                     )
+  scalaattr = struct(
+      outputs = None,
+      compile_jars = deps_jars.compile_jars,
+      transitive_runtime_jars = deps_jars.transitive_runtime_jars,
+  )
 
   transitive_srcjars = collect_srcjars(ctx.attr.deps) + collect_extra_srcjars(ctx.attr.deps)
 
@@ -177,22 +177,19 @@ def _gen_scrooge_srcjar_impl(ctx):
     )],
   )
 
+# TODO(twigg): Use one in scala.bzl ...
 def _collect_scalaattr(targets):
-  transitive_runtime_deps = set()
-  transitive_compile_exports = set()
-  transitive_runtime_exports = set()
+  compile_jars = depset()
+  transitive_runtime_jars = depset()
+
   for target in targets:
     if hasattr(target, "scala"):
-      transitive_runtime_deps += target.scala.transitive_runtime_deps
-      transitive_compile_exports += target.scala.transitive_compile_exports
-      if hasattr(target.scala.outputs, "ijar"):
-        transitive_compile_exports += [target.scala.outputs.ijar]
-      transitive_runtime_exports += target.scala.transitive_runtime_exports
+      compile_jars += target.scala.compile_jars
+      transitive_runtime_jars += target.scala.transitive_runtime_jars
 
   return struct(
-    transitive_runtime_deps = transitive_runtime_deps,
-    transitive_compile_exports = transitive_compile_exports,
-    transitive_runtime_exports = transitive_runtime_exports,
+    compile_jars = compile_jars,
+    transitive_runtime_jars = transitive_runtime_jars,
   )
 
 scrooge_scala_srcjar = rule(
@@ -204,7 +201,7 @@ scrooge_scala_srcjar = rule(
         #     is saying that we have a jar with a bunch
         #     of thrifts that we want to depend on. Seems like
         #     that should be a concern of thrift_library? we have
-        #     it here through becuase we need to show that it is
+        #     it here through because we need to show that it is
         #     "covered," as well as needing the thrifts to
         #     do the code gen.
         "remote_jars": attr.label_list(),
@@ -229,9 +226,10 @@ def scrooge_scala_library(name, deps=[], remote_jars=[], jvm_flags=[], visibilit
         visibility = visibility,
     )
 
+    # deps information is via srcjar
     scala_library(
         name = name,
-        deps = deps + remote_jars + [
+        deps = remote_jars + [
             srcjar,
             "//external:io_bazel_rules_scala/dependency/thrift/libthrift",
             "//external:io_bazel_rules_scala/dependency/thrift/scrooge_core"