Make sure that scala proto files are generated correctly for external proto dependencies (#294)

* Make sure to extract workspace root for proto files

* Update tests and docs

* Append deps to the end

* Targets run properly

* Fix comments and fail if needed

* One more comment

* Update readme

Author: azymnis

README.md

@@ -7,6 +7,7 @@
     <li><a href="#scala_library">scala_library/scala_macro_library</a></li>
     <li><a href="#scala_binary">scala_binary</a></li>
     <li><a href="#scala_test">scala_test</a></li>
+    <li><a href="#scalapb_proto_library">scalapb_proto_library</a></li>
   </ul>
 </div>
 
@@ -467,10 +468,20 @@ thrift_library(name, srcs, deps, absolute_prefix, absolute_prefixes)
   </tbody>
 </table>
 
+<a name="scalapb_proto_library"></a>
 ## scalapb_proto_library
 
+You first need to add the following to your WORKSPACE file:
+
+```python
+load("@io_bazel_rules_scala//scala_proto:scala_proto.bzl", "scala_proto_repositories")
+scala_proto_repositories()
+```
+
+Then you can import `scalapb_proto_library` in any BUILD file like this:
+
 ```python
-load("//scala_proto:scala_proto.bzl", "scalapb_proto_library")
+load("@io_bazel_rules_scala//scala_proto:scala_proto.bzl", "scalapb_proto_library")
 scalapb_proto_library(name, deps, with_grpc, with_java, with_flat_package, with_single_line_to_string)
 ```
 
@@ -498,7 +509,7 @@ generated by the [ScalaPB compiler](https://github.com/scalapb/ScalaPB).
       <td><code>deps</code></td>
       <td>
         <p><code>List of labels, required</code></p>
-        <p>List of proto dependencies that this target depends on. Must be of type <code>proto_library</code></p>
+        <p>List of dependencies for this target. Must either be of type <code>proto_library</code> or <code>java_proto_library</code> (allowed only if <code>with_java</code> is enabled) </p>
       </td>
     </tr>
     <tr>
@@ -512,7 +523,7 @@ generated by the [ScalaPB compiler](https://github.com/scalapb/ScalaPB).
       <td><code>with_java</code></td>
       <td>
         <p><code>boolean; optional (default False)</code></p>
-        <p>Enables generation of converters to and from java protobuf bindings</p>
+        <p>Enables generation of converters to and from java protobuf bindings. If you set this to <code>True</code> make sure that you pass the corresponding <code>java_proto_library</code> target in <code>deps</code></p>
       </td>
     </tr>
     <tr>

scala/scala.bzl

@@ -513,7 +513,7 @@ def _collect_jars_when_dependency_analyzer_is_on(dep_targets):
     jars2labels = jars2labels,
     transitive_compile_jars = transitive_compile_jars)
 
-def _collect_jars(dep_targets, dependency_analyzer_is_off = True):
+def collect_jars(dep_targets, dependency_analyzer_is_off = True):
     """Compute the runtime and compile-time dependencies from the given targets"""  # noqa
 
     if dependency_analyzer_is_off:
@@ -541,10 +541,10 @@ def _collect_jars_from_common_ctx(ctx, extra_deps = [], extra_runtime_deps = [])
 
     # Get jars from deps
     auto_deps = [ctx.attr._scalalib, ctx.attr._scalareflect]
-    deps_jars = _collect_jars(ctx.attr.deps + auto_deps + extra_deps, dependency_analyzer_is_off)
+    deps_jars = collect_jars(ctx.attr.deps + auto_deps + extra_deps, dependency_analyzer_is_off)
     (cjars, transitive_rjars, jars2labels, transitive_compile_jars) = (deps_jars.compile_jars, deps_jars.transitive_runtime_jars, deps_jars.jars2labels, deps_jars.transitive_compile_jars)
 
-    runtime_dep_jars =  _collect_jars(ctx.attr.runtime_deps + extra_runtime_deps, dependency_analyzer_is_off)
+    runtime_dep_jars =  collect_jars(ctx.attr.runtime_deps + extra_runtime_deps, dependency_analyzer_is_off)
     transitive_rjars += runtime_dep_jars.transitive_runtime_jars
 
     if not dependency_analyzer_is_off:
@@ -555,7 +555,7 @@ def _collect_jars_from_common_ctx(ctx, extra_deps = [], extra_runtime_deps = [])
 def _format_full_jars_for_intellij_plugin(full_jars):
     return [struct (class_jar = jar, ijar = None) for jar in full_jars]
 
-def _create_java_provider(ctx, scalaattr, transitive_compile_time_jars):
+def create_java_provider(ctx, scalaattr, transitive_compile_time_jars):
     # This is needed because Bazel >=0.6.0 requires ctx.actions and a Java
     # toolchain. Fortunately, the same change that added this requirement also
     # added this field to the Java provider so we can use it to test which
@@ -609,7 +609,7 @@ def _lib(ctx, non_macro_lib):
     # Add information from exports (is key that AFTER all build actions/runfiles analysis)
     # Since after, will not show up in deploy_jar or old jars runfiles
     # Notice that compile_jars is intentionally transitive for exports
-    exports_jars = _collect_jars(ctx.attr.exports)
+    exports_jars = collect_jars(ctx.attr.exports)
     next_cjars += exports_jars.compile_jars
     transitive_rjars += exports_jars.transitive_runtime_jars
 
@@ -630,7 +630,7 @@ def _lib(ctx, non_macro_lib):
         transitive_exports = [] #needed by intellij plugin
     )
 
-    java_provider = _create_java_provider(ctx, scalaattr, jars.transitive_compile_jars)
+    java_provider = create_java_provider(ctx, scalaattr, jars.transitive_compile_jars)
 
     return struct(
         files = depset([ctx.outputs.jar]),  # Here is the default output
@@ -694,7 +694,7 @@ def _scala_binary_common(ctx, cjars, rjars, transitive_compile_time_jars, jars2l
       transitive_exports = [] #needed by intellij plugin
   )
 
-  java_provider = _create_java_provider(ctx, scalaattr, transitive_compile_time_jars)
+  java_provider = create_java_provider(ctx, scalaattr, transitive_compile_time_jars)
 
   return struct(
       files=depset([ctx.outputs.executable]),
@@ -771,7 +771,7 @@ def _scala_test_impl(ctx):
       jars.transitive_compile_jars, jars.jars2labels)
     # _scalatest is an http_jar, so its compile jar is run through ijar
     # however, contains macros, so need to handle separately
-    scalatest_jars = _collect_jars([ctx.attr._scalatest]).transitive_runtime_jars
+    scalatest_jars = collect_jars([ctx.attr._scalatest]).transitive_runtime_jars
     cjars += scalatest_jars
     transitive_rjars += scalatest_jars
 

scala_proto/scala_proto.bzl

@@ -1,5 +1,7 @@
 load("//scala:scala.bzl",
-  "scala_library")
+  "scala_library",
+  "collect_jars",
+  "create_java_provider")
 
 def scala_proto_repositories():
     native.maven_server(
@@ -308,12 +310,26 @@ def scala_proto_repositories():
     )
 
 def _colon_paths(data):
-  return ':'.join([f.path for f in data])
+  return ':'.join(["{root},{path}".format(root=f.owner.workspace_root, path=f.path) for f in data])
 
 def _gen_proto_srcjar_impl(ctx):
     acc_imports = depset()
+
+    proto_deps, java_proto_lib_deps = [], []
     for target in ctx.attr.deps:
-        acc_imports += target.proto.transitive_sources
+        if hasattr(target, 'proto'):
+            proto_deps.append(target)
+            acc_imports += target.proto.transitive_sources
+        else:
+            java_proto_lib_deps.append(target)
+
+    if not ctx.attr.with_java and len(java_proto_lib_deps) > 0:
+        fail("cannot have java_proto_library dependencies with with_java is False")
+
+    if ctx.attr.with_java and len(java_proto_lib_deps) == 0:
+        fail("must have a java_proto_library dependency if with_java is True")
+
+    deps_jars = collect_jars(java_proto_lib_deps)
 
     # Command line args to worker cannot be empty so using padding
     flags = ["-"]
@@ -345,7 +361,15 @@ def _gen_proto_srcjar_impl(ctx):
     srcjarsattr = struct(
         srcjar = ctx.outputs.srcjar,
     )
+    scalaattr = struct(
+      outputs = None,
+      compile_jars =  deps_jars.compile_jars,
+      transitive_runtime_jars = deps_jars.transitive_runtime_jars,
+    )
+    java_provider = create_java_provider(ctx, scalaattr, depset())
     return struct(
+        scala = scalaattr,
+        providers = [java_provider],
         srcjars=srcjarsattr,
         extra_information=[struct(
           srcjars=srcjarsattr,
@@ -357,7 +381,7 @@ scalapb_proto_srcjar = rule(
     attrs={
         "deps": attr.label_list(
             mandatory=True,
-            allow_rules=["proto_library"]
+            allow_rules=["proto_library", "java_proto_library"]
         ),
         "with_grpc": attr.bool(default=False),
         "with_java": attr.bool(default=False),
@@ -414,7 +438,7 @@ Example:
 
 Args:
     name: A unique name for this rule
-    deps: Proto library targets that this rule depends on (must be of type proto_library)
+    deps: Proto library or java proto library (if with_java is True) targets that this rule depends on
     with_grpc: Enables generation of grpc service bindings for services defined in deps
     with_java: Enables generation of converters to and from java protobuf bindings
     with_flat_package: When true, ScalaPB will not append the protofile base name to the package name
@@ -446,19 +470,9 @@ def scalapb_proto_library(
 
     external_deps = list(SCALAPB_DEPS + GRPC_DEPS if (with_grpc) else SCALAPB_DEPS)
 
-    if with_java:
-        java_proto_lib = name + "_java_lib"
-        native.java_proto_library(
-            name = java_proto_lib,
-            deps = deps,
-            visibility = visibility,
-        )
-        external_deps.append(java_proto_lib)
-
     scala_library(
         name = name,
-        srcs = [srcjar],
-        deps = external_deps,
+        deps = [srcjar] + external_deps,
         exports = external_deps,
         visibility = visibility,
     )

src/scala/scripts/ScalaPBGenerator.scala

@@ -39,7 +39,25 @@ class ScalaPBGenerator extends Processor {
 
   def processRequest(args: java.util.List[String]) {
     val jarOutput = args.get(0)
-    val protoFiles = args.get(1).split(':').toList
+    val parsedProtoFiles = args.get(1).split(':').toList.map { rootAndFile =>
+      val parsed = rootAndFile.split(',')
+      val root = parsed(0)
+      val file = if (root.isEmpty) {
+        parsed(1)
+      } else {
+        parsed(1).substring(root.length + 1)
+      }
+      (file, Paths.get(root, file).toString)
+    }
+    // This will map the absolute path of a given proto file
+    // to a relative path that does not contain the repo prefix.
+    // This is to match the expected behavior of
+    // proto_library and java_proto_library where proto files
+    // can import other proto files using only the relative path
+    val imports = parsedProtoFiles.map { case (relPath, absolutePath) =>
+      s"-I${relPath}=${absolutePath}"
+    }
+    val protoFiles = parsedProtoFiles.map(_._2)
     val flagOpt = args.get(2) match {
       case "-" => None
       case s => Some(s.drop(2))
@@ -48,7 +66,7 @@ class ScalaPBGenerator extends Processor {
     val tmp = Paths.get(Option(System.getProperty("java.io.tmpdir")).getOrElse("/tmp"))
     val scalaPBOutput = Files.createTempDirectory(tmp, "bazelscalapb")
     val flagPrefix = flagOpt.map(_ + ":").getOrElse("")
-    val scalaPBArgs = s"--scala_out=${flagPrefix}${scalaPBOutput}" :: protoFiles
+    val scalaPBArgs = s"--scala_out=${flagPrefix}${scalaPBOutput}" :: (imports ++ protoFiles)
     val config = ScalaPBC.processArgs(scalaPBArgs.toArray)
     val code = ProtocBridge.runWithGenerators(
       protoc = a => com.github.os72.protocjar.Protoc.runProtoc(config.version +: a.toArray),

test/proto/BUILD

@@ -28,9 +28,14 @@ scalapb_proto_library(
     visibility = ["//visibility:public"],
 )
 
+java_proto_library(
+    name = "test_proto_java_lib",
+    deps = [":test2", "//test/proto2:test"],
+)
+
 scalapb_proto_library(
     name = "test_proto_java_conversions",
-    deps = [":test2", "//test/proto2:test"],
+    deps = [":test2", "//test/proto2:test", ":test_proto_java_lib"],
     with_java = True,
     with_flat_package = True,
     visibility = ["//visibility:public"],

twitter_scrooge/twitter_scrooge.bzl

@@ -4,7 +4,8 @@ load("//scala:scala.bzl",
   "scala_mvn_artifact",
   "scala_library",
   "write_manifest",
-  "collect_srcjars")
+  "collect_srcjars",
+  "collect_jars")
 
 def twitter_scrooge():
   native.maven_server(
@@ -177,7 +178,7 @@ def _gen_scrooge_srcjar_impl(ctx):
     arguments=["--jvm_flag=%s" % flag for flag in ctx.attr.jvm_flags] + ["@" + argfile.path],
   )
 
-  deps_jars = _collect_jars(ctx.attr.deps)
+  deps_jars = collect_jars(ctx.attr.deps)
 
   scalaattr = struct(
       outputs = None,
@@ -201,22 +202,6 @@ def _gen_scrooge_srcjar_impl(ctx):
     )],
   )
 
-# TODO(twigg): Use the one in scala.bzl?
-def _collect_jars(targets):
-  compile_jars = depset()
-  transitive_runtime_jars = depset()
-
-  for target in targets:
-    if java_common.provider in target:
-      java_provider = target[java_common.provider]
-      compile_jars += java_provider.compile_jars
-      transitive_runtime_jars += java_provider.transitive_runtime_jars
-
-  return struct(
-    compile_jars = compile_jars,
-    transitive_runtime_jars = transitive_runtime_jars,
-  )
-
 scrooge_scala_srcjar = rule(
     _gen_scrooge_srcjar_impl,
     attrs={