rules-proto-grpc / rules_proto_grpc

Bazel rules for building Protobuf and gRPC code and libraries from proto_library targets
https://rules-proto-grpc.com
Apache License 2.0
252 stars 158 forks source link

Support external repositories and generated sources #158

Closed tkoeppe closed 2 months ago

tkoeppe commented 2 years ago

Description

I have seen failures in generated C++ and Python code when using a proto_library with (any of) the following properties:

Could you perhaps check if you have test coverage for those cases?

aaliddell commented 2 years ago

Do you have an example failing repo for either case?

We test the first case pretty comprehensively, since a lot of tests depend on @com_google_protobuf, which contains the well-known protos.

We test the second case here: https://github.com/rules-proto-grpc/rules_proto_grpc/tree/master/test_workspaces/generated_proto

tkoeppe commented 2 years ago

Ah, let me double-check. One error I see are all from when both cases are combined, i.e. my proto has a dependency on a generated proto in a different repository.

Something like this:

proto_library(name = "p1_proto", srcs = ["p1.proto"], deps = ["@side//:q1_proto"])
python_proto_library(name = "p1_python_proto", protos = [":p1_proto"])
py_binary(name = "test", srcs = ["test.py"], deps = [":p1_python_proto"])

And here @side//:q1_proto would have a generated source.

The other error is when the generated proto is a dependency of another proto. In your example, I would try having another proto file depend onproto_lib (i.e. say import "demo.proto"). The error I see is at runtime when importing the outer proto module fails to find the inner one.

aaliddell commented 2 years ago

Ok, thanks, sounds like I need to extend that test workspace and see if I can replicate the issue.

tkoeppe commented 2 years ago

Thanks! I could try and make a complete repro available somehow, but let me know if you get to any problems yourself!

aaliddell commented 2 years ago

I've updated the test workspace to (I think) include the above suggestions and I still can't get it to fail:

This has both static and generated proto in a workspace depending on a generated proto from another workspace. I think I'm going to need that repro repo :smile:

tkoeppe commented 2 years ago

Thank you! OK, one thing at a time. The following use of a generated proto as a dependency of an on-disk proto causes an error for me:

load("@rules_proto_grpc//cpp:defs.bzl", "cpp_proto_library")

genrule(
    name = "p5_proto_gen",
    srcs = ["p5.proto.src"],
    outs = ["p5.proto"],
    cmd = "cp $(<) $(@)",
)

proto_library(
    name = "p5_proto",
    srcs = [":p5.proto"],
)

proto_library(
    name = "p5_consume_proto",
    srcs = ["p5_consume.proto"],
    deps = [":p5_proto"],
)

cpp_proto_library(
    name = "p5_consume_cpp_proto",
    protos = [":p5_consume_proto"],
)

Building:

bazel build :p5_consume_cpp_proto

INFO: Analyzed target //:p5_consume_cpp_proto (21 packages loaded, 621 targets configured).
INFO: Found 1 target...
ERROR: [...]
[generated_proto_repro.tar.gz](https://github.com/rules-proto-grpc/rules_proto_grpc/files/7619758/generated_proto_repro.tar.gz)
/main_repo/BUILD:21:18: Compiling p5_consume_cpp_proto_pb/p5_consume.pb.cc failed: (Exit 1): gcc failed: error executing command /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer '-std=c++0x' -MD -MF ... (remaining 36 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
In file included from bazel-out/k8-fastbuild/bin/p5_consume_cpp_proto_pb/p5_consume.pb.cc:4:
bazel-out/k8-fastbuild/bin/p5_consume_cpp_proto_pb/p5_consume.pb.h:34:10: fatal error: p5.pb.h: No such file or directory
   34 | #include "p5.pb.h"
      |          ^~~~~~~~~
compilation terminated.
Target //:p5_consume_cpp_proto failed to build
Use --verbose_failures to see the command lines of failed build steps.

The on-disk proto p5_consume.proto says import "p5.proto";, and p5.proto is generated by the genrule.

Can you reproduce that error?

I'll try to make a minimal repro for the external repo next. Thanks a lot!

tkoeppe commented 2 years ago

generated_proto_repro.tar.gz

tkoeppe commented 2 years ago

Another issue with dependencies on generated protos arises when using the Python bindings. Extend the above as follows:

python_proto_library(                                                                                                                                                                         
    name = "p5_consume_python_proto",                                                                                                                                                         
    protos = [":p5_consume_proto"],                                                                                                                                                           
)                                                                                                                                                                                             

python_proto_library(                                                                                                                                                                         
    name = "p5_python_proto",                                                                                                                                                                 
    protos = [":p5_proto"],                                                                                                                                                                   
)                                                                                                                                                                                             

py_binary(                                                                                                                                                                                    
    name = "demo",                                                                                                                                                                            
    srcs = ["demo.py"],                                                                                                                                                                       
    deps = [                                                                                                                                                                                  
        ":p5_consume_python_proto",                                                                                                                                                           
        #":p5_python_proto",   # error, this is needed                                                                                                                                        
    ],                                                                                                                                                                                        
)  

demo.py says import p5_consume_pb2, which results in ModuleNotFoundError: No module named 'p5_pb2': It looks like the python_proto_library rule does not add the necessary dependencies. Adding the dependency (and the python_proto_library rule(!)) for p5.proto manually makes this work.

tkoeppe commented 2 years ago

I wonder if these issues also affected the external-repo case I had, so maybe let's concentrate on these two first, and then revisit the external repos afterwards.

github-actions[bot] commented 2 years ago

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further.

aaliddell commented 2 months ago

I think this has been fixed in 5.0.0