Using proto_source_root in proto_library breaks java_grpc_library #4896

haikalpribadi · 2018-10-01T23:50:20Z

Please answer these questions before submitting your issue.

What version of gRPC are you using?

v1.15.0

What did you expect to see?

Say I have 2 proto files sitting in //some-package/

//some-package/A.proto which imports B.proto (without some-package as prefix)
//some-package/B.proto

Then I compile the proto_library in //some-package/BUILD`, like the following:

proto_library(
    name = "some-proto",
    srcs = glob(["*.proto"])
    proto_source_root = "some-package"
)

The above works well because proto_source_root, which allows us to not have to import one proto file into another using the full path relative from WORKSPACE.

As instructed on GRPC docs (https://grpc.io/docs/reference/java/generated-code.html) Next I compile java_proto_library:

java_proto_library(
    name = "java-proto",
    deps = ["some-proto"],
)

This also works fine. But finally, I compile java_grpc_library:

java_grpc_library(
    name = "java-grpc",
    srcs = [":some-proto"],
    deps = [":java-proto"],
)

And here it breaks. java_grpc_library failes to compile with error:

B.proto: file not found.
some-package/A.proto: Import "B.proto" was not found or had errors.

So basically, A.proto cannot find B.proto. However, when I replace the import statement in A.proto with import some-package/B.proto and remove usage of proto_source_root in proto_library everything works, but we really don't want to do this for many reasons.

Does anyone know what could be the cause and how to work around this?

The text was updated successfully, but these errors were encountered:

haikalpribadi · 2018-10-02T15:41:31Z

To help reproduce this problem easily, I've isolated the problem on a branch on my repo: https://github.com/haikalpribadi/grakn/tree/bazel-init-grpc-java-broken-with-proto-source-root

The BUILD file can be found at: https://github.com/haikalpribadi/grakn/blob/bazel-init-grpc-java-broken-with-proto-source-root/client-protocol/proto/BUILD

Once you clone the repo, you just need to run bazel build //client-protocol/proto:client-java-grpc

Thank you!

ejona86 · 2018-10-02T18:25:13Z

The cleanest way to solve this is to use the dsc file/transitive_descriptor_sets from the proto_library plus descriptor_set_in to protoc, as then java_grpc_library no longer has to deal with imports and listing files and means java_grpc_library wouldn't break as proto_library adds new features. The code change for this is trivial, but last time I tried to do that I broke some existing users as the dsc building was stricter. I may try to do that again and figure out exactly where the incompatibility came from.

Although it's probably also fairly easy to add support for proto_source_root directly to java_grpc_library, but I'll need to research it some.

haikalpribadi · 2018-10-02T18:39:41Z

Thanks, @ejona86. Is there any examples on how to use file/transitive_descriptor_sets ?

Although, I do think adding proto_source_root to java_grpc_library (and every other *_grpc_library for that matter) is a more elegant solution. And an even better solution is for java_grpc_library to not need proto_source_root at all, and instead rely on the fact that the proto_library has already encapsulated this information. Is that possible?

ejona86 · 2018-10-02T19:29:12Z

And an even better solution is for java_grpc_library to not need proto_source_root at all, and instead rely on the fact that the proto_library has already encapsulated this information.

This is what I was suggesting with the descriptor set.

Although, I do think adding proto_source_root to java_grpc_library (and every other *_grpc_library for that matter) is a more elegant solution.

For the solution like this, java_grpc_library wouldn't need any additional configuration, it would just be changed to look at proto_library's configuration.

haikalpribadi · 2018-10-02T19:36:15Z

This is what I was suggesting with the descriptor set.

I see! Sorry I didn't get your first point, but I do now. :)

For the solution like this, java_grpc_library wouldn't need any additional configuration, it would just be changed to look at proto_library's configuration.

Do you mean java_grpc_library would still need a new argument but it will have a default value which is what is inside proto_library's configuration?

ejona86 · 2018-10-02T19:47:16Z

Do you mean java_grpc_library would still need a new argument but it will have a default value which is what is inside proto_library's configuration?

No, I was meaning it would use the value directly from the proto_library. Although we're not using aspects in java_grpc_library, that's more of the style of how proto_library is expected to be integrated with.

For reference, here's the type of changes necessary for using the descriptor set. But it is based on the internal copy which is basically slowly being rewritten and I've not yet exported the changes (limited time and I'm "not done yet").

--- a/java/grpc/build_defs.bzl	2018-07-23 17:24:35.000000000 -0700
+++ b/java/grpc/build_defs.bzl	2018-07-25 13:48:29.000000000 -0700
@@ -10,9 +10,6 @@
 

 
-def _create_include_path(include):
-    return "-I{0}={1}".format(include.short_path, include.path)
-
 def _short_path(src):
     return src.short_path
 
@@ -24,7 +21,7 @@
                "same package as consuming rule").format(ctx.label, ctx.attr.srcs[0].label))
 
     srcs = ctx.attr.srcs[0].proto.direct_sources
-    includes = ctx.attr.srcs[0].proto.transitive_imports
+    descriptor_set_in = ctx.attr.srcs[0].proto.transitive_descriptor_sets
     flavor = ctx.attr.flavor
     if flavor == "normal":
         flavor = ""
@@ -32,11 +29,11 @@
     args = ctx.actions.args()
     args.add(ctx.executable._java_plugin.path, format = "--plugin=protoc-gen-grpc-java=%s")
     args.add("--grpc-java_out={0}:{1}".format(flavor, ctx.outputs.srcjar.path))
-    args.add_all(includes, map_each = _create_include_path)
+    args.add_joined("--descriptor_set_in", descriptor_set_in, join_with = ":")
     args.add_all(srcs, map_each = _short_path)
 
     ctx.action(
-        inputs = depset([ctx.executable._java_plugin] + srcs, transitive = [includes]),
+        inputs = depset([ctx.executable._java_plugin] + srcs, transitive = [descriptor_set_in]),
         outputs = [ctx.outputs.srcjar],
         executable = ctx.executable._protoc,
         arguments = [args],

haikalpribadi · 2018-10-02T20:04:14Z

Alright, I see. It makes to me from a high level, but unfortunately, my 2 weeks old knowledge of Bazel might not be enough for me to submit a PR for this. :/

adibrastegarnia · 2019-01-10T17:14:33Z

Hello,

I have a similar problem but I didn't understand the solution. I used proto source root in my proto_library and it can be compiled fine but when I use grpc_proto_library to generate grpc stubs, it gives import not found and file not found (i.e. it cannot fine the imports in the protobuf that I define the services)

adibrastegarnia · 2019-01-17T00:36:34Z

@ejona86

Eric,

May I ask you what is the status of this change? Can you give me some guidelines If I want to work on it. We need to get it fixed to use for a project.

Thanks,
Adib

ejona86 · 2019-01-17T00:58:27Z

Honestly, for the while my recommendation is "don't do that;" always import from the base directory. This is what Blaze has required of us, for example. So I see this more as a feature request with a possible workaround.

Why is this fix required for a project?

adibrastegarnia · 2019-01-17T01:09:55Z

@ejona86

Let me explain our situation: Please take a look at:
(https://github.com/dnosproject/dnos-core-grpc/blob/master/protobuf/proto/net/ServicesProto.proto)
we use imports like this for example : import "net/link/LinkEvent.proto" if we want to import from LinkEvent.proto in another protobuf message. It works fine if we don't use grpc_proto_lib to generate a grpc stub. If we want to use grpc_proto_lib then the imports should be changed to something like this otherwise it cannot find it (i.e. file not found error): import "protobuf/proto/net/link/LinkEventProto.proto". If you look at the build file we use proto source root in proto_build rules.

https://github.com/dnosproject/dnos-core-grpc/blob/master/protobuf/proto/BUILD

Does that mean we should change our imports or can we still use net/link/LinkEvent.proto?

Is my explanation clear? If not let me know and I will try to explain it in a better way.

ejona86 · 2019-01-17T02:28:06Z

Ah, I understand more. I didn't realize proto_source_ root was available now-a-days. I want to use that internally to make the import easier! (Right now we apply some diffs to just to fix the .proto imports.)

(Yes, I know it was all mentioned clearly earlier, but I had been dealing with various "proto did something interesting in random case X". Instead of investigating what the new option was I was thinking of swapping to the descriptor set approach gets us out of the business of tracking them.)

proto_source_root is available in ProtoInfo, so this should be pretty easy. Basically it would be an additional string concat any place we use _path_ignoring_repository (to add dep.proto.proto_source_root prefix). And adding a test.

I'd be happy to accept a PR; this is something that someone else could figure out semi-easily (unless anything goes wrong!). But I also agree that Skylark/Bazel rules are quite involved. I might be able to get to this next week.

adibrastegarnia · 2019-01-17T04:29:34Z

@ejona86

I am not completely familiar with grpc-java code so I will try but I think you will get it work before I do. Please update me if you fix it so I can get a new version of that to test. Thanks.

ejona86 · 2019-01-17T23:26:13Z

@adibrastegarnia, I tried this out today. It turned out to be "not easy." So don't waste your time trying to figure it out; I am still debugging what's wrong.

adibrastegarnia · 2019-01-17T23:32:42Z

@ejona86
OK. Thanks for letting me know. Please keep me posted if you find a solution (I hope you do).

adibrastegarnia · 2019-01-22T16:56:33Z

@ejona86

Eric, Any updates on this issue?

ejona86 · 2019-01-23T16:49:34Z

No, no updates.

ejona86 · 2019-01-25T17:23:26Z

I dug into this more. I may be missing something simple, but it currently seems mostly unsupportable. It seems like it requires using an aspect, plus some very specific logic. I tried using the transitive descriptors, and they appear not to save any effort with this particular feature.

The only ways this could be implemented would be for ProtoInfo to include more information or for proto_toolchain to open itself up to Skylark implementations.

To give a sampling of how Bazel itself is using this stuff:
https://github.com/bazelbuild/bazel/blob/0d708411/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java#L288
https://github.com/bazelbuild/bazel/blob/0d708411/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java#L698

Part of the problem is that when using proto_source_root you can still do imports based on the full path. (That is, it seems you can take any proto_library and add proto_source_root to it without breaking the build.)

I'll need to have some conversations with the Bazel folks at some point.

adibrastegarnia · 2019-01-25T17:40:19Z

Thanks Eric for the update.

Part of the problem is that when using proto_source_root you can still do

imports based on the full path. (That is, it seems you can take any proto_library and add proto_source_root to it without breaking the build.) This is exactly what I do as a workaround. Do you think should we go for that workaround or wait to get it fixed? Adib

…

On Fri, Jan 25, 2019 at 12:23 PM Eric Anderson ***@***.***> wrote: I dug into this more. I may be missing something simple, but it currently seems mostly unsupportable. It seems like it requires using an aspect, plus some very specific logic. I tried using the transitive descriptors, and they appear not to save any effort with this particular feature. The only ways this could be implemented would be for ProtoInfo to include more information or for proto_toolchain to open itself up to Skylark implementations. To give a sampling of how Bazel itself is using this stuff: https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java#L288 https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java#L698 Part of the problem is that when using proto_source_root you can still do imports based on the full path. (That is, it seems you can take any proto_library and add proto_source_root to it without breaking the build.) I'll need to have some conversations with the Bazel folks at some point. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4896 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJ4q3KMpY1DeI480wMZLjFABH02J-JKZks5vGz2hgaJpZM4XDFay> .

ejona86 · 2019-01-26T04:42:38Z

@adibrastegarnia, I don't really see how that is a "workaround" as it is equivalent to not using proto_source_root. For now, I'd recommend not using proto_source_root. I'm not expecting a quick fix. But I also need to talk with the Bazel folks.

adibrastegarnia · 2019-01-26T05:18:39Z

@ejona86
OK. Please let me know if there is any updates.

ejona86 · 2019-02-13T20:45:16Z

FYI: bazelbuild/bazel#7153 , proto_source_root is being replaced with strip_import_prefix

adibrastegarnia · 2019-02-13T21:45:59Z

Thanks Eric for the update. Glad to hear it gets fixed.

…

On Wed, Feb 13, 2019 at 3:45 PM Eric Anderson ***@***.***> wrote: FYI: bazelbuild/bazel#7153 <bazelbuild/bazel#7153> , proto_source_root is being replaced with strip_import_prefix — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4896 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJ4q3JffoFuzbsFI-G75yBqmc6B6Y_lIks5vNHlxgaJpZM4XDFay> .

ejona86 · 2022-06-06T07:29:06Z

Fixed by #5959

carl-mastrangelo assigned zhangkun83 Oct 2, 2018

zhangkun83 assigned ejona86 and unassigned zhangkun83 Oct 2, 2018

ejona86 added the enhancement label Oct 2, 2018

ejona86 added this to the Next milestone Oct 2, 2018

haikalpribadi mentioned this issue Oct 3, 2018

Using proto_source_root in proto_library breaks <lang>_grpc_[compile|library] pubref/rules_proto#4

Closed

adibrastegarnia mentioned this issue Jan 10, 2019

java_grpc_library- import not found #5225

Closed

ejona86 mentioned this issue Apr 23, 2019

java_grpc_library: enable (strip_)import_prefix in proto dependencies #5621

Closed

ejona86 closed this as completed Jun 6, 2022

ejona86 modified the milestones: Next, 1.23 Jun 6, 2022

github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using proto_source_root in proto_library breaks java_grpc_library #4896

Using proto_source_root in proto_library breaks java_grpc_library #4896

haikalpribadi commented Oct 1, 2018 •

edited

Loading

haikalpribadi commented Oct 2, 2018 •

edited

Loading

ejona86 commented Oct 2, 2018 •

edited

Loading

haikalpribadi commented Oct 2, 2018 •

edited

Loading

ejona86 commented Oct 2, 2018

haikalpribadi commented Oct 2, 2018

ejona86 commented Oct 2, 2018

haikalpribadi commented Oct 2, 2018

adibrastegarnia commented Jan 10, 2019

adibrastegarnia commented Jan 17, 2019

ejona86 commented Jan 17, 2019

adibrastegarnia commented Jan 17, 2019 •

edited

Loading

ejona86 commented Jan 17, 2019

adibrastegarnia commented Jan 17, 2019

ejona86 commented Jan 17, 2019

adibrastegarnia commented Jan 17, 2019

adibrastegarnia commented Jan 22, 2019

ejona86 commented Jan 23, 2019

ejona86 commented Jan 25, 2019 •

edited

Loading

adibrastegarnia commented Jan 25, 2019 via email

ejona86 commented Jan 26, 2019

adibrastegarnia commented Jan 26, 2019

ejona86 commented Feb 13, 2019

adibrastegarnia commented Feb 13, 2019 via email

ejona86 commented Jun 6, 2022

Using proto_source_root in proto_library breaks java_grpc_library #4896

Using proto_source_root in proto_library breaks java_grpc_library #4896

Comments

haikalpribadi commented Oct 1, 2018 • edited Loading

What version of gRPC are you using?

What did you expect to see?

haikalpribadi commented Oct 2, 2018 • edited Loading

ejona86 commented Oct 2, 2018 • edited Loading

haikalpribadi commented Oct 2, 2018 • edited Loading

ejona86 commented Oct 2, 2018

haikalpribadi commented Oct 2, 2018

ejona86 commented Oct 2, 2018

haikalpribadi commented Oct 2, 2018

adibrastegarnia commented Jan 10, 2019

adibrastegarnia commented Jan 17, 2019

ejona86 commented Jan 17, 2019

adibrastegarnia commented Jan 17, 2019 • edited Loading

ejona86 commented Jan 17, 2019

adibrastegarnia commented Jan 17, 2019

ejona86 commented Jan 17, 2019

adibrastegarnia commented Jan 17, 2019

adibrastegarnia commented Jan 22, 2019

ejona86 commented Jan 23, 2019

ejona86 commented Jan 25, 2019 • edited Loading

adibrastegarnia commented Jan 25, 2019 via email

ejona86 commented Jan 26, 2019

adibrastegarnia commented Jan 26, 2019

ejona86 commented Feb 13, 2019

adibrastegarnia commented Feb 13, 2019 via email

ejona86 commented Jun 6, 2022

haikalpribadi commented Oct 1, 2018 •

edited

Loading

haikalpribadi commented Oct 2, 2018 •

edited

Loading

ejona86 commented Oct 2, 2018 •

edited

Loading

haikalpribadi commented Oct 2, 2018 •

edited

Loading

adibrastegarnia commented Jan 17, 2019 •

edited

Loading

ejona86 commented Jan 25, 2019 •

edited

Loading