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

azymnis · 2017-10-03T19:20:50Z

Problem

scalapb_proto_library will not work with proto_library_targets that are imported from an external repo. The reason is that bazel's proto_library expects proto files to import external files while omitting the external/<repo name>/ bit from the file name. In addition to this java conversions will not work for the final scala library because of the fact that java_proto_library generates a separate external jar for external dependencies that scala_library cannot pull in transitively.

Solution

Two steps:

Pass in the repo_root as well as the proto path for each proto file to be compiled and use a -I flag in protoc to map the actual location of proto files to the one that omits the repo root prefix.
Don't automatically generate the java_proto_library dependency if with_java is true, but expect it to be passed as part of deps.

@johnynek @ittaiz let me know what you think

bazel-io · 2017-10-03T19:20:52Z

Can one of the admins verify this patch?

johnynek · 2017-10-03T23:06:29Z

scala_proto/scala_proto.bzl

@@ -414,7 +414,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 targets or jvm targets that this rule depends on


this can't be any jvm target, right? You are expecting java_proto_library?

Well, any jvm targets that you pass here are going to be passed through as deps to the final output scala_library. Does that seem unreasonable?

johnynek · 2017-10-03T23:06:52Z

src/scala/scripts/ScalaPBGenerator.scala

+      }
+      (file, Paths.get(root, file).toString)
+    }
+    val imports = parsedProtoFiles.map { case (relPath, absolutePath) =>


can you comment what these are doing?

johnynek · 2017-10-03T23:07:22Z

scala_proto/scala_proto.bzl

@@ -414,7 +414,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 targets or jvm 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


to do this you must also depend on the java_proto_library? Is that right? Should we update this comment?

johnynek · 2017-10-03T23:08:31Z

README.md

@@ -498,7 +498,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. All targets of type <code>proto_library</code> are passed to <code>scalapb</code> for compilation. All other targets are used to compile the final output scala library</p>


can these really be any generic target if not proto?

Is it anything that you might need to do the with_java, is that it?

Well the reason I changed this is for with_java but you could potentially use this to pass other dependencies to the output scala_library (though not sure why one would want that).

johnynek · 2017-10-03T23:12:04Z

scala_proto/scala_proto.bzl

-            visibility = visibility,
-        )
-        external_deps.append(java_proto_lib)
+    scala_lib_deps = external_deps + deps


do you not need the transitive compile time dependencies of any of the jvm code?

Also, I think we want to filter this so that we only include the non-proto deps. Otherwise, casual formatting changes to the proto that don't change the compile output will cause a recompilation.

Two questions:

How can I get the transitive compile time deps for a jvm dep?

What is the best way to do this in a macro? Remember that this is not within a rule implementation so deps is just a list of strings. Should I use native.existing_rule? https://docs.bazel.build/versions/master/skylark/lib/native.html#existing_rule

johnynek · 2017-10-03T23:13:36Z

scala_proto/scala_proto.bzl

@@ -308,12 +308,13 @@ 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])


this is making me nervous, but maybe that's the only way to go here.

There was an old doc about fixing these kinds of issues about cross workspace dependencies like 1.5 years ago or so, but I don't think they actually ever did anything about it, unless I'm mistaken:

cc @damienmg I recall there was a plan to put all the paths rooted with the project name. That never shipped right?

If there's a better way to deal with this, I would be happy to do that instead. Frankly I just cribbed this from what the java_proto_library tool is doing

damienmg · 2017-10-03T23:25:38Z

Right Kristina was working on it but that's a hard work. It still has to ship in the following months hopefully.

…

On Tue, Oct 3, 2017, 7:17 PM P. Oscar Boykin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In scala_proto/scala_proto.bzl <#294 (comment)> : > @@ -414,7 +414,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 targets or jvm targets that this rule depends on this can't be any jvm target, right? You are expecting java_proto_library? ------------------------------ In src/scala/scripts/ScalaPBGenerator.scala <#294 (comment)> : > @@ -39,7 +39,20 @@ 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) + } + val imports = parsedProtoFiles.map { case (relPath, absolutePath) => can you comment what these are doing? ------------------------------ In scala_proto/scala_proto.bzl <#294 (comment)> : > @@ -414,7 +414,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 targets or jvm 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 to do this you must also depend on the java_proto_library? Is that right? Should we update this comment? ------------------------------ In README.md <#294 (comment)> : > @@ -498,7 +498,7 @@ generated by the [ScalaPB compiler](https://github.com/scalapb/ScalaPB). <td><code>deps</code></td> <td> <code>List of labels, required</code> - List of proto dependencies that this target depends on. Must be of type <code>proto_library</code> + List of dependencies for this target. All targets of type <code>proto_library</code> are passed to <code>scalapb</code> for compilation. All other targets are used to compile the final output scala library can these really be any generic target if not proto? Is it anything that you might need to do the with_java, is that it? ------------------------------ In scala_proto/scala_proto.bzl <#294 (comment)> : > @@ -445,20 +445,12 @@ 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_lib_deps = external_deps + deps do you not need the transitive compile time dependencies of any of the jvm code? Also, I think we want to filter this so that we only include the non-proto deps. Otherwise, casual formatting changes to the proto that don't change the compile output will cause a recompilation. ------------------------------ In scala_proto/scala_proto.bzl <#294 (comment)> : > @@ -308,12 +308,13 @@ 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]) this is making me nervous, but maybe that's the only way to go here. There was an old doc about fixing these kinds of issues about cross workspace dependencies like 1.5 years ago or so, but I don't think they actually ever did anything about it, unless I'm mistaken: cc @damienmg <https://github.com/damienmg> I recall there was a plan to put all the paths rooted with the project name. That never shipped right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#294 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADjHfxugl0YBjLtjdgtdwE-KNy8ZtY0Nks5sosBrgaJpZM4PsouD> .

azymnis · 2017-10-04T20:35:46Z

@johnynek ok I think this is better: I changed it only accept proto_library and java_proto_library deps and pushed the logic of figuring out jar dependencies into the rule implementation.

Note: I had to modify a couple of scala.bzl methods to be public instead of private (and also deleted a method that could be removed in scrooge). Let me know if that looks ok.

ittaiz · 2017-10-04T21:16:43Z

Haven't had a chance to take a look unfortunately but turning private scala.bzl methods to public sounds like a really bad idea. It turns it to public API which will be hard to change. Can we extract them out to a separate bzl and have it have limited visibility so only internal rules_scala targets can use it? We need to do this refactoring at some point since scala.bzl is a god file

…

On Wed, Oct 4, 2017 at 11:35 PM Argyris Zymnis ***@***.***> wrote: @johnynek <https://github.com/johnynek> ok I think this is better: I changed it only accept proto_library and java_proto_library deps and pushed the logic of figuring out jar dependencies into the rule implementation. Note: I had to modify a couple of scala.bzl methods to be public instead of private (and also deleted a method that could be removed in scrooge). Let me know if that looks ok. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#294 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFw_polJS2_zPP7_qNjHGNvz5g9Ugks5so-wjgaJpZM4PsouD> .

azymnis · 2017-10-04T21:24:06Z

@ittaiz happy to do this refactor. How can I limit a .bzl file to only be accessible from within this repo?

johnynek · 2017-10-04T21:52:22Z

I think this is good.

We already had duplication about collection of jars and many scala/java related code want this.

The alternative is copy-paste.

I'm +1 on this PR.

ittaiz · 2017-10-05T06:02:46Z

Argyris, The full docs are here https://docs.bazel.build/versions/master/be/common-definitions.html but essentially __subpackages__ should be your friend. Oscar, Not sure I understand what you mean. Did you mean you're good without the refactoring? To leave this code duplication in?

…

On Thu, 5 Oct 2017 at 0:52 P. Oscar Boykin ***@***.***> wrote: I think this is good. We already had duplication about collection of jars and many scala/java related code want this. The alternative is copy-paste. I'm +1 on this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#294 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF2B3iy1DmbzffwtzuLymWqztjroBks5so_4XgaJpZM4PsouD> .

johnynek · 2017-10-05T07:32:41Z

@ittaiz how do you apply visibility to .bzl files? I thought that was for rules. Can you give a concrete example?

I am fine with the refactoring he has done as it stands now. If it is easy to mark bazel functions or files with visibility then great, but I didn’t know we could.

ittaiz · 2017-10-05T07:54:55Z

Right... my bad. I mixed up the two. I actually saw an issue around this recently but it's obviously unsolved.

…

On Thu, 5 Oct 2017 at 10:32 P. Oscar Boykin ***@***.***> wrote: @ittaiz <https://github.com/ittaiz> how do you apply visibility to .bzl files? I thought that was for rules. Can you give a concrete example? I am fine with the refactoring he has done as it stands now. If it is easy to mark bazel functions or files with visibility then great, but I didn’t know we could. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#294 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF05DPkBx4yrUz7npYVJGfp9BwI9tks5spIYbgaJpZM4PsouD> .

ittaiz

So I skimmed the PR and it looks good in general but if I'm not mistaken there's no test for the external repo problem right? Can we add one for it?

Btw, sorry for having sparse time for the proto area. I hope to free up some time to dive into it so i can share the review load

azymnis · 2017-10-05T18:00:13Z

@ittaiz yup tests would be good but I think that would require adding an external repo dependency in WORKSPACE. Let me know if I should do that. Should I add a personal repo for this?

johnynek · 2017-10-05T18:17:10Z

@azymnis you can have a nested WORKSPACE I think and use that in tests.

But this is a bit of a yak shave if you ask me. With a nested WORKSPACE I think you have to set up --deleted-packages or something to prevent the outer build from finding it.

azymnis · 2017-10-05T18:19:24Z

@johnynek @ittaiz I'll let you two decide on this. If you feel very strongly that I should add tests for external, I will do so. The current tests at least prove that this doesn't break the non external use cases (which I suspect are much more common). I verified locally that this works for us.

johnynek · 2017-10-05T18:22:16Z

personally I'm happy to merge and add an issue.

When we don't have good examples of how to do a test, I'm -1 on asking contributors to shave a new yak to fix a bug they can observe. If we can test it with existing or simple patterns I would say okay, but having used a nested workspace internally it is a pain, and I don't want to rush to it.

azymnis · 2017-10-05T22:18:27Z

@johnynek seems like travis-ci had another transient download issue

johnynek · 2017-10-05T23:46:14Z

sorry for the latency.

Argyris Zymnis added 2 commits October 3, 2017 10:56

Make sure to extract workspace root for proto files

af859b2

Update tests and docs

8048cc7

googlebot added the cla: yes label Oct 3, 2017

Append deps to the end

dfbda06

johnynek reviewed Oct 3, 2017

View reviewed changes

Argyris Zymnis added 3 commits October 4, 2017 11:56

Targets run properly

5c8955c

Fix comments and fail if needed

433f022

One more comment

60014d4

ittaiz reviewed Oct 5, 2017

View reviewed changes

Update readme

65e53e8

johnynek merged commit 0bac7fe into bazel-contrib:master Oct 5, 2017

Uh oh!

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

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

Uh oh!

Conversation

azymnis commented Oct 3, 2017

Problem

Solution

Uh oh!

bazel-io commented Oct 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damienmg commented Oct 3, 2017 via email

Uh oh!

azymnis commented Oct 4, 2017

Uh oh!

ittaiz commented Oct 4, 2017 via email

Uh oh!

azymnis commented Oct 4, 2017

Uh oh!

johnynek commented Oct 4, 2017

Uh oh!

ittaiz commented Oct 5, 2017 via email

Uh oh!

johnynek commented Oct 5, 2017

Uh oh!

ittaiz commented Oct 5, 2017 via email

Uh oh!

ittaiz left a comment

Choose a reason for hiding this comment

Uh oh!

azymnis commented Oct 5, 2017

Uh oh!

johnynek commented Oct 5, 2017

Uh oh!

azymnis commented Oct 5, 2017

Uh oh!

johnynek commented Oct 5, 2017

Uh oh!

azymnis commented Oct 5, 2017

Uh oh!

johnynek commented Oct 5, 2017

Uh oh!

Uh oh!