Aspect-based scrooge implementation

scala/private/common.bzl

@@ -1,10 +1,14 @@
 def write_manifest(ctx):
+  main_class = getattr(ctx.attr, "main_class", None)
+  write_manifest_file(ctx.actions, ctx.outputs.manifest, main_class)
+
+def write_manifest_file(actions, output_file, main_class):
   # TODO(bazel-team): I don't think this classpath is what you want
   manifest = "Class-Path: \n"
-  if getattr(ctx.attr, "main_class", ""):
-    manifest += "Main-Class: %s\n" % ctx.attr.main_class
+  if main_class:
+    manifest += "Main-Class: %s\n" % main_class
 
-  ctx.actions.write(output = ctx.outputs.manifest, content = manifest)
+  actions.write(output = output_file, content = manifest)
 
 def collect_srcjars(targets):
   srcjars = []

scala/private/rule_impls.bzl

@@ -132,25 +132,11 @@ def _expand_location(ctx, flags):
 def _join_path(args, sep = ","):
   return sep.join([f.path for f in args])
 
-def compile_scala(ctx,
-                  target_label,
-                  output,
-                  manifest,
-                  statsfile,
-                  sources,
-                  cjars,
-                  all_srcjars,
-                  buildijar,
-                  transitive_compile_jars,
-                  plugins,
-                  resource_strip_prefix,
-                  resources,
-                  resource_jars,
-                  labels,
-                  in_scalacopts,
-                  print_compile_time,
-                  expect_java_output,
-                  scalac_jvm_flags = []):
+def compile_scala(ctx, target_label, output, manifest, statsfile, sources,
+                  cjars, all_srcjars, buildijar, transitive_compile_jars,
+                  plugins, resource_strip_prefix, resources, resource_jars,
+                  labels, in_scalacopts, print_compile_time, expect_java_output,
+                  scalac_jvm_flags):
   ijar_output_path = ""
   ijar_cmd_path = ""
   if buildijar:
@@ -254,12 +240,13 @@ StatsfileOutput: {statsfile_output}
       output = argfile, content = scalac_args + optional_scalac_args)
 
   outs = [output, statsfile]
+  ins = (compiler_classpath_jars.to_list() + all_srcjars.to_list() +
+         list(sources) + plugins_list + dependency_analyzer_plugin_jars +
+         classpath_resources + resources + resource_jars + [manifest, argfile])
   if buildijar:
     outs.extend([ctx.outputs.ijar])
-  ins = (
-      compiler_classpath_jars.to_list() + all_srcjars.to_list() + list(sources)
-      + plugins_list + dependency_analyzer_plugin_jars + classpath_resources +
-      resources + resource_jars + [manifest, ctx.executable._ijar, argfile])
+    ins.extend([ctx.executable._ijar])
+
   ctx.actions.run(
       inputs = ins,
       outputs = outs,
@@ -567,28 +554,9 @@ def _lib(ctx, non_macro_lib):
       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,
-      # new_target1 -> scala_library -> new_target2. There might be
-      # information that new_target2 needs to get from new_target1,
-      # but we do not want to have to change scala_library to pass
-      # 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.
-      # 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,
     )
 
-def _collect_extra_information(targets):
-  r = []
-  for target in targets:
-    if hasattr(target, "extra_information"):
-      r.extend(target.extra_information)
-  return r
-
 def scala_library_impl(ctx):
   return _lib(ctx, True)
 

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

@@ -1,5 +1,6 @@
 load("//scala:scala.bzl", "scala_binary", "scala_library")
 load("//twitter_scrooge:twitter_scrooge.bzl", "scrooge_scala_library")
+load("//thrift:thrift.bzl", "thrift_library")
 
 scrooge_scala_library(
     name = "scrooge1",
@@ -83,21 +84,12 @@ scrooge_scala_library(
 scrooge_scala_library(
     name = "bare_thrift_scrooge",
     visibility = ["//visibility:public"],
-    deps = [
+    exports = [
         ":scroogebarejar1",
         ":scroogebarejar2",
-        "//test/src/main/scala/scalarules/test/twitter_scrooge/thrift/bare_jar_thrifts",
-    ],
-)
-
-scrooge_scala_library(
-    name = "thrift_with_remote_jar",
-    remote_jars = [
-        "//test/src/main/scala/scalarules/test/twitter_scrooge/thrift/bare_jar_thrifts:barejar_java_import",
     ],
-    visibility = ["//visibility:public"],
     deps = [
-        "//test/src/main/scala/scalarules/test/twitter_scrooge/thrift/bare_jar_thrifts/bare_jar_2",
+        "//test/src/main/scala/scalarules/test/twitter_scrooge/thrift/bare_jar_thrifts",
     ],
 )
 

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

@@ -1,20 +1,23 @@
 load("//thrift:thrift.bzl", "thrift_library")
 
-filegroup(
+java_import(
     name = "barejar",
-    srcs = ["bare-thrift.jar"],
+    jars = ["bare-thrift.jar"],
     visibility = ["//visibility:public"],
 )
 
 java_import(
     name = "barejar_java_import",
-    jars = ["bare-thrift.jar"],
+    jars = ["bare_jar_thrifts_scrooge.jar"],
     visibility = ["//visibility:public"],
 )
 
 thrift_library(
     name = "bare_jar_thrifts",
-    external_jars = [":barejar"],
+    external_jars = [
+        ":barejar",
+        ":barejar_java_import",
+    ],
     visibility = ["//visibility:public"],
     deps = [
         "//test/src/main/scala/scalarules/test/twitter_scrooge/thrift/bare_jar_thrifts/bare_jar_1",

test/src/main/scala/scalarules/test/twitter_scrooge/thrift/bare_jar_thrifts/bare_jar_1/BUILD

@@ -1,13 +1,22 @@
 load("//thrift:thrift.bzl", "thrift_library")
 
-filegroup(
+java_import(
     name = "barejar1",
-    srcs = ["bare-thrift-1.jar"],
+    jars = ["bare-thrift-1.jar"],
+    visibility = ["//visibility:public"],
+)
+
+java_import(
+    name = "bare_jar_1_remote",
+    jars = ["bare_jar_1_scrooge.jar"],
     visibility = ["//visibility:public"],
 )
 
 thrift_library(
     name = "bare_jar_1",
-    external_jars = [":barejar1"],
+    external_jars = [
+        ":barejar1",
+        ":bare_jar_1_remote",
+    ],
     visibility = ["//visibility:public"],
 )

test/src/main/scala/scalarules/test/twitter_scrooge/thrift/bare_jar_thrifts/bare_jar_2/BUILD

@@ -1,13 +1,22 @@
 load("//thrift:thrift.bzl", "thrift_library")
 
-filegroup(
+java_import(
     name = "barejar2",
-    srcs = ["bare-thrift-2.jar"],
+    jars = ["bare-thrift-2.jar"],
+    visibility = ["//visibility:public"],
+)
+
+java_import(
+    name = "bare_jar_2_remote",
+    jars = ["bare_jar_2_scrooge.jar"],
     visibility = ["//visibility:public"],
 )
 
 thrift_library(
     name = "bare_jar_2",
-    external_jars = [":barejar2"],
+    external_jars = [
+        ":barejar2",
+        ":bare_jar_2_remote",
+    ],
     visibility = ["//visibility:public"],
 )

thrift/thrift.bzl

@@ -1,13 +1,21 @@
 """Rules for organizing thrift files."""
 
-_thrift_filetype = FileType([".thrift"])
+load("@io_bazel_rules_scala//thrift:thrift_info.bzl", "ThriftInfo")
 
-ThriftInfo = provider(fields = [
-    "srcs",  # The source files in this rule
-    "transitive_srcs",  # the transitive version of the above
-    "external_jars",  # external jars of thrift files
-    "transitive_external_jars"  # transitive version of the above
-])
+def empty_thrift_info():
+  return ThriftInfo(
+      srcs = depset(),
+      transitive_srcs = depset(),
+      external_jars = depset(),
+      transitive_external_jars = depset())
+
+def merge_thrift_infos(tis):
+  return ThriftInfo(
+      srcs = depset(transitive = [t.srcs for t in tis]),
+      transitive_srcs = depset(transitive = [t.transitive_srcs for t in tis]),
+      external_jars = depset(transitive = [t.external_jars for t in tis]),
+      transitive_external_jars = depset(
+          transitive = [t.transitive_external_jars for t in tis]))
 
 def _common_prefix(strings):
   pref = None
@@ -62,7 +70,6 @@ def _thrift_library_impl(ctx):
     zipper_arg_path = ctx.actions.declare_file(
         "%s_zipper_args" % ctx.outputs.libarchive.path)
     ctx.actions.write(zipper_arg_path, zipper_args)
-    _valid_thrift_deps(ctx.attr.deps)
     # We move the files and touch them so that the output file is a purely deterministic
     # product of the _content_ of the inputs
     cmd = """
@@ -81,6 +88,7 @@ rm -f {out}
         progress_message = "making thrift archive %s (%s files)" %
         (ctx.label, len(src_paths)),
     )
+    srcs_depset = depset([ctx.outputs.libarchive])
   else:
     # we still have to create the output we declared
     ctx.actions.run_shell(
@@ -95,20 +103,28 @@ rm {out}.contents
            zipper = ctx.executable._zipper.path),
         progress_message = "making empty thrift archive %s" % ctx.label,
     )
+    srcs_depset = depset()
+
+  # external jars are references to thrift we depend on,
+  # BUT WE DON'T BUILD. When we build the code, the code can
+  # do a thrift include to this, but we won't generate the source or bytecode.
+  remotes = []
+  for f in ctx.attr.external_jars:
+    remotes.extend(f.files.to_list())
 
   transitive_srcs = depset(
-      [ctx.outputs.libarchive],
-      transitive = _collect_thrift_srcs(ctx.attr.deps))
-  jarfiles = _collect_thrift_external_jars(ctx.attr.deps)
-  for jar in ctx.attr.external_jars:
-    jarfiles.append(depset(jar.files))
-  transitive_external_jars = depset(transitive = jarfiles)
+      transitive = _collect_thrift_srcs(ctx.attr.deps) + [srcs_depset])
+  transitive_external_jars = depset(
+      remotes,
+      transitive = [
+          d[ThriftInfo].transitive_external_jars for d in ctx.attr.deps
+      ])
 
   return [
       ThriftInfo(
-          srcs = ctx.outputs.libarchive,
+          srcs = srcs_depset,
           transitive_srcs = transitive_srcs,
-          external_jars = ctx.attr.external_jars,
+          external_jars = depset(remotes),
           transitive_external_jars = transitive_external_jars,
       )
   ]
@@ -119,17 +135,6 @@ def _collect_thrift_srcs(targets):
     ds.append(target[ThriftInfo].transitive_srcs)
   return ds
 
-def _collect_thrift_external_jars(targets):
-  ds = []
-  for target in targets:
-    ds.append(target[ThriftInfo].transitive_external_jars)
-  return ds
-
-def _valid_thrift_deps(targets):
-  for target in targets:
-    if not ThriftInfo in target:
-      fail("thrift_library can only depend on thrift_library", target)
-
 # Some notes on the raison d'etre of thrift_library vs. code gen specific
 # targets. The idea is to be able to separate concerns -- thrift_library is
 # concerned purely with the ownership and organization of thrift files. It
@@ -141,8 +146,8 @@ def _valid_thrift_deps(targets):
 thrift_library = rule(
     implementation = _thrift_library_impl,
     attrs = {
-        "srcs": attr.label_list(allow_files = _thrift_filetype),
-        "deps": attr.label_list(),
+        "srcs": attr.label_list(allow_files = [".thrift"]),
+        "deps": attr.label_list(providers = [ThriftInfo]),
         #TODO this is not necessarily the best way to do this... the goal
         # is that we want thrifts to be able to be imported via an absolute
         # path. But the thrift files have no clue what part of their path
@@ -160,6 +165,9 @@ thrift_library = rule(
         "absolute_prefix": attr.string(default = '', mandatory = False),
         "absolute_prefixes": attr.string_list(),
         # This is a list of JARs which only contain Thrift files
+        # these files will NOT  be compiled as part of the current target,
+        # but the current thrifts do include these. It should also include
+        # the compiled versions of these thrifts
         "external_jars": attr.label_list(),
         "_zipper": attr.label(
             executable = True,
@@ -168,4 +176,5 @@ thrift_library = rule(
             allow_files = True)
     },
     outputs = {"libarchive": "lib%{name}.jar"},
+    provides = [ThriftInfo],
 )

thrift/thrift_info.bzl

@@ -0,0 +1,6 @@
+ThriftInfo = provider(fields = [
+    "srcs",  # The source files in this rule
+    "transitive_srcs",  # the transitive version of the above
+    "external_jars",  # external jars of thrift files
+    "transitive_external_jars",  # transitive version of the above
+])

tools/bazel.rc

@@ -1,2 +1,2 @@
-build --experimental_local_disk_cache
-build --experimental_local_disk_cache_path=.bazel_cache
+#build --experimental_local_disk_cache
+#build --experimental_local_disk_cache_path=.bazel_cache