Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose a py_proto_library target for build_event_stream.proto so that way the python programs outside the Bazel repo can consume the BEP #15874

Open
aprotyas opened this issue Jul 13, 2022 · 9 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: feature request

Comments

@aprotyas
Copy link

aprotyas commented Jul 13, 2022

Description of the feature request:

Make the src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto file publicly visible. Alternatively, export the file with exports_files().

What underlying problem are you trying to solve with this feature?

I have a Python tool that needs to ingest the build event stream, so I'm trying to generate a Python protobuf library for the BES.

I'm using the @com_github_grpc_grpc//py_proto_library rule as such:

py_proto_library(
    name = "build_event_stream",
    deps = ["@io_bazel//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_proto"],
)

However, since the proto file of interest is in an external workspace, the generated build_event_stream_proto_pb2.py file has the following content which can't be imported:

from ...io_bazel.src.main.java.com.google.devtools.build.lib.buildeventstream.proto.build_event_stream_pb2 import *

The workaround to this is to declare a filegroup with the proto file of interest, manually build a local proto library, and then list that target as a dependency (I've verified that this approach works with public proto files such as @io_bazel//src/main/protobuf:extra_actions_base.proto).

# This example should work, but doesn't since build_event_stream.proto is not publicly visible

py_proto_library(
    name = "build_event_stream",
    deps = [":build_event_stream_proto"],
)

proto_library(
    name = "build_event_stream_proto",
    srcs = [":build_event_stream_proto_file"],
)

filegroup(
    name = "build_event_stream_proto_file",
    srcs = ["@io_bazel//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream.proto"],
)

Unfortunately, this workaround does not work for this case because build_event_stream.proto is not publicly visible.

Which operating system are you running Bazel on?

Ubuntu 20.04.4 LTS

What is the output of bazel info release?

release 4.2.2

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

Nothing exactly relevant, but there are a bunch of questions about which py_proto_library rule to actually use (I've found 3 implementations...).

Any other information, logs, or outputs that you want to share?

No response

@aprotyas aprotyas changed the title Make build_event_stream.proto public so that Python protobuf libraries are usable Make build_event_stream.proto public so that BES Python protobuf library is usable Jul 13, 2022
@aprotyas
Copy link
Author

aprotyas commented Jul 18, 2022

Having applied the patch below to my Bazel fork, I think it's evident that this is not the right approach here.

diff --git src/main/java/com/google/devtools/build/lib/buildeventstream/proto/BUILD src/main/java/com/google/devtools/build/lib/buildeventstream/proto/BUILD
index 313dccb6e1..8311ef385e 100644
--- src/main/java/com/google/devtools/build/lib/buildeventstream/proto/BUILD
+++ src/main/java/com/google/devtools/build/lib/buildeventstream/proto/BUILD
@@ -4,6 +4,12 @@ load("@rules_proto//proto:defs.bzl", "proto_library")

 package(default_visibility = ["//src:__subpackages__"])

+exports_files([
+    "build_event_stream.proto",
+])
+
 filegroup(
     name = "srcs",
     srcs = glob(["**"]),

While this patch makes build_event_stream.proto visible downstream, it opens another can of worms for the "local filegroup -> local proto lib -> py_proto_lib" approach since that proto file itself pulls in other non-public proto libs (command_line_proto).

To that end, I think requesting for the BES proto file to be made publicly visible may be misguided, and I should instead request for a py_proto_library instance consuming the build_event_stream_proto. Thoughts?

EDIT: Unfortunately, building a py_proto_library (https://github.com/aprotyas/bazel/blob/330e57cbb9904e613d8e91c1af221f15d2fbb3e9/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/BUILD#L38-L44) as suggested^ and consuming it downstream is still problematic because it can't find proto files referred to by build_event_stream.proto. 😢

@comius comius added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc and removed team-Rules-Python Native rules for Python labels Jul 19, 2022
@comius
Copy link
Contributor

comius commented Jul 19, 2022

cc @michaeledgar, BEP is under team-core, right?

@haxorz haxorz added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jul 20, 2022
@haxorz
Copy link
Contributor

haxorz commented Jul 20, 2022

cc @michaeledgar, BEP is under team-core, right?

Yes, it is, so yes we'd be the team responsible for making the proposed change.

But can someone on team-OSS (or someone responsible for how Bazel is used) comment on whether this sort of change is desirable in general? I reckon there are other source files in the Bazel repo that other repos would want to consume; what's the best practice for allowing them / not allowing them to do that? I imagine there are files we would not want to expose externally.

@haxorz
Copy link
Contributor

haxorz commented Jul 20, 2022

On second thought, there's already //src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto with intentionally-public visibility. So that precedent makes me think we don't need to think about setting a precedent ourselves here.

Therefore I think the proposed change is very reasonable.

@haxorz haxorz removed the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jul 20, 2022
@haxorz haxorz changed the title Make build_event_stream.proto public so that BES Python protobuf library is usable Expose a py_proto_library target for build_event_stream.proto so that way the python programs outside the Bazel repo can consume the BEP Jul 20, 2022
@comius
Copy link
Contributor

comius commented Jul 20, 2022

Nothing exactly relevant, but there are a bunch of questions about which py_proto_library rule to actually use (I've found 3 implementations...).

Bazel is using py_proto_library from @com_google_protobuf.

py_proto_library(

The implementation is not perfect, but using that one, should prevent any compatibility issues and it hopefully doesn't have problems crossing repo boundaries.

The Bazel team plans to provide py_proto_library as a native rule in Bazel by the end of September 2022, which should become the default one.

@haxorz haxorz removed the untriaged label Jul 20, 2022
@aprotyas
Copy link
Author

aprotyas commented Jul 20, 2022

The Bazel team plans to provide py_proto_library as a native rule in Bazel by the end of September 2022, which should become the default one.

Looking forward to that!

Bazel is using py_proto_library from @com_google_protobuf.

py_proto_library(

The implementation is not perfect, but using that one, should prevent any compatibility issues and it hopefully doesn't have problems crossing repo boundaries.

I experimented with exposing a py_proto_library target with that rule on my fork, but ingesting this py_proto_library downstream doesn't work for me. Maybe I'm messing something up? Here's what protoc complains about says:

Error message
(14:33:39) ERROR: /home/user/.cache/bazel/_bazel_aprotyas/4a97a70fd3beec2f8ff7ff6fbda19be1/external/io_bazel/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/BUILD:26:17: ProtoCompile external/io_bazel/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream_pb2.py failed: (Exit 1): protoc failed: error executing command
  (cd /home/user/.cache/bazel/_bazel_aprotyas/4a97a70fd3beec2f8ff7ff6fbda19be1/execroot/av && \
  exec env - \
    LANG=C.UTF-8 \
    LC_ALL=C.UTF-8 \
    PATH=/bin:/usr/bin:/usr/local/bin \
  bazel-out/k8-opt-exec-CD513B8/bin/external/com_google_protobuf/protoc '--python_out=bazel-out/k8-opt/bin/external/io_bazel' -Iexternal/io_bazel -Ibazel-out/k8-opt/bin/external/io_bazel external/io_bazel/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto)

. . .


src/main/protobuf/command_line.proto: File not found.
src/main/protobuf/failure_details.proto: File not found.
src/main/protobuf/invocation_policy.proto: File not found.
src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto:19:1: Import "src/main/protobuf/command_line.proto" was not found or had errors.
src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto:20:1: Import "src/main/protobuf/failure_details.proto" was not found or had errors.
src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto:21:1: Import "src/main/protobuf/invocation_policy.proto" was not found or had errors.
src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto:345:3: "blaze.invocation_policy.InvocationPolicy" is not defined.
src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto:484:3: "failure_details.FailureDetail" is not defined.
src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto:538:3: "failure_details.FailureDetail" is not defined.
src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto:809:5: "command_line.CommandLine" is not defined.

[Edit]: I think I need to create py_proto_lib instances of these dependencies and list those in the dep attribute.

@comius
Copy link
Contributor

comius commented Jul 21, 2022

[Edit]: I think I need to create py_proto_lib instances of these dependencies and list those in the dep attribute.

Yes, it seems so. I didn't know that protoc/py_proto_library needs targets transitively (because it's not using an aspect). In this respect grpc/py_proto_library is better, but looking at the implementation, it is not handling repository boundaries correctly. :(

@aprotyas
Copy link
Author

Hi team, any updates on this? I'm a little blocked by this right now. Neither of the two popular py_proto_library rule implementations (@com_github_grpc_grpc, @com_google_protobuf), seem to expose the required target correctly.

@tpudlik
Copy link
Contributor

tpudlik commented Nov 4, 2022

I too became interested in how py_proto_library is coming along, and found it's now a PR: bazelbuild/rules_python#832. Looks pretty close to going in!

@haxorz haxorz added the P2 We'll consider working on this in future. (Assignee optional) label Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: feature request
Projects
None yet
Development

No branches or pull requests

6 participants