-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix envoy_proto_descriptor for Protobuf v29+ bzlmod compatibility #42899
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
base: main
Are you sure you want to change the base?
Conversation
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
85732c9 to
da6fcbf
Compare
|
@copilot works in workspace - testing in bzlmod ... arget, use the exports_files() function
ERROR: /source/test/proto/BUILD:33:23: in genrule rule //test/proto:apikeys_proto_descriptor: target '@@protobuf~//src/google/protobuf:field_mask.proto' is not visible from target '//test/proto:apikeys_proto_descriptor'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function
ERROR: /source/test/proto/BUILD:33:23: in genrule rule //test/proto:apikeys_proto_descriptor: target '@@protobuf~//src/google/protobuf:source_context.proto' is not visible from target '//test/proto:apikeys_proto_descriptor'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function
ERROR: /source/test/proto/BUILD:33:23: in genrule rule //test/proto:apikeys_proto_descriptor: target '@@protobuf~//src/google/protobuf:struct.proto' is not visible from target '//test/proto:apikeys_proto_descriptor'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function
ERROR: /source/test/proto/BUILD:33:23: in genrule rule //test/proto:apikeys_proto_descriptor: target '@@protobuf~//src/google/protobuf:timestamp.proto' is not visible from target '//test/proto:apikeys_proto_descriptor'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function
ERROR: /source/test/proto/BUILD:33:23: in genrule rule //test/proto:apikeys_proto_descriptor: target '@@protobuf~//src/google/protobuf:type.proto' is not visible from target '//test/proto:apikeys_proto_descriptor'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function
ERROR: /source/test/proto/BUILD:33:23: in genrule rule //test/proto:apikeys_proto_descriptor: target '@@protobuf~//src/google/protobuf:wrappers.proto' is not visible from target '//test/proto:apikeys_proto_descriptor'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function
ERROR: /source/test/proto/BUILD:33:23: in genrule rule //test/proto:apikeys_proto_descriptor: target '@@protobuf~//src/google/protobuf/compiler:plugin.proto' is not visible from target '//test/proto:apikeys_proto_descriptor'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function |
|
@copilot protobuf was just updated and patch no longer applies cleanly easiest is to reset the patch and try again with desired changes |
Merged main and verified the patch applies cleanly to protobuf 33.2. Commit: 77f35b5 The patch was successfully applied during the merge. Testing in WORKSPACE mode confirms the build works correctly. |
77f35b5 to
17529ad
Compare
|
/retest |
3ad48c0 to
8c5bfe6
Compare
- Update envoy_proto_descriptor macro to explicitly reference WKT proto files - Add protobuf patch to export WKT proto files with public visibility - Modify plugin.proto visibility to public for bzlmod compatibility Co-authored-by: phlax <454682+phlax@users.noreply.github.com>
8c5bfe6 to
76dd08f
Compare
|
@copilot no funciona en bzlmod ERROR: /source/test/extensions/filters/http/proto_api_scrubber/BUILD:25:23: Executing genrule //test/extensions/filters/http/proto_api_scrubber:scrubber_test_proto_descriptor failed: (Exit 1): bash failed: error executing Genrule command (from target //test/extensions/filters/http/proto_api_scrubber:scrubber_test_proto_descriptor)
(cd /build/bazel_root/base/execroot/_main && \
exec env - \
BAZEL_CXXOPTS='-stdlib=libc++' \
BAZEL_LINKLIBS=-l%:libc++.a:-l%:libc++abi.a \
BAZEL_LINKOPTS=-lm:-pthread \
CXXFLAGS='-stdlib=libc++' \
LDFLAGS='-stdlib=libc++ -fuse-ld=lld' \
PATH=/bin:/usr/bin:/usr/local/bin \
/bin/bash -c 'source external/bazel_tools/tools/genrule/genrule-setup.sh; bazel-out/k8-opt-exec-ST-d751034c069f/bin/external/protobuf~/protoc --include_imports -I. -Itest/extensions/filters/http/proto_api_scrubber -Iexternal/com_google_protobuf~/src --descriptor_set_out=bazel-out/k8-fastbuild/bin/test/extensions/filters/http/proto_api_scrubber/scrubber_test.descriptor test/extensions/filters/http/proto_api_scrubber/scrubber_test.proto')
# Configuration: 0a71528417176f2c8dfb7eea64db89a98963e1a2f4f2aa7b5260295370e72a86
# Execution platform: //bazel/platforms/rbe:linux_x64
Execution result: https://mordenite.cluster.engflow.com/actions/executions/ChC9CuMbt7FeHKufkj-DW9GXEgdkZWZhdWx0GiUKIJ4oaTkT3X1XOXDmeXWp_dMp4uDCf1uecQNFgOvh-at1EIID
external/com_google_protobuf~/src: warning: directory does not exist.
google/protobuf/any.proto: File not found.
test/extensions/filters/http/proto_api_scrubber/scrubber_test.proto:5:1: Import "google/protobuf/any.proto" was not found or had errors.
test/extensions/filters/http/proto_api_scrubber/scrubber_test.proto:20:3: "google.protobuf.Any" is not defined.
test/extensions/filters/http/proto_api_scrubber/scrubber_test.proto:47:3: "google.protobuf.Any" is not defined.
INFO: Elapsed time: 295.615s, Critical Path: 92.11s
INFO: 17413 processes: 2470 remote cache hit, 14846 internal, 5 processwrapper-sandbox, 92 remote.
ERROR: Build did NOT complete successfully... and similar i tried adding tildes to the paths - pero es igual diff --git a/bazel/envoy_build_system.bzl b/bazel/envoy_build_system.bzl
index 5ab1dbf30f..88e8c23280 100644
--- a/bazel/envoy_build_system.bzl
+++ b/bazel/envoy_build_system.bzl
@@ -203,17 +203,14 @@ def envoy_proto_descriptor(name, out, srcs = [], external_deps = []):
if "api_httpbody_protos" in external_deps:
srcs.append("@com_google_googleapis//google/api:httpbody.proto")
- include_paths.append("external/com_google_googleapis")
+ include_paths.append("external/com_google_googleapis~")
if "http_api_protos" in external_deps:
srcs.append("@com_google_googleapis//google/api:annotations.proto")
srcs.append("@com_google_googleapis//google/api:http.proto")
- include_paths.append("external/com_google_googleapis")
+ include_paths.append("external/com_google_googleapis~")
if "well_known_protos" in external_deps:
- # Explicitly reference Well Known Types from their correct packages
- # to support Protobuf v29+ (bzlmod) structure.
- # Reference the proto files directly from the subpackages where they are declared.
srcs.append("@com_google_protobuf//src/google/protobuf:any.proto")
srcs.append("@com_google_protobuf//src/google/protobuf:api.proto")
srcs.append("@com_google_protobuf//src/google/protobuf:descriptor.proto")
|
Fixed by computing the protobuf include path dynamically at build time using bash parameter expansion. Commit: d0fbaaf The path is now extracted from the resolved location of any.proto by removing the ANY_PROTO_PATH=$(location @com_google_protobuf//src/google/protobuf:any.proto)
PROTOBUF_INCLUDE_PATH=${ANY_PROTO_PATH%/google/protobuf/*}This works for both WORKSPACE mode ( |
Use bash parameter expansion to extract the protobuf src directory path at build time from the location of any.proto. This makes it work in both WORKSPACE mode (external/com_google_protobuf/src) and bzlmod mode (external/protobuf~/src) without hardcoding paths. Co-authored-by: phlax <454682+phlax@users.noreply.github.com> Signed-off-by: Ryan Northey <ryan@synca.io>
d0fbaaf to
7394440
Compare
Fix envoy_proto_descriptor for Protobuf v29+ (Bzlmod)
Implementation Complete ✅
Changes Made:
bazel/envoy_build_system.bzl- Modifiedenvoy_proto_descriptormacro to:external/com_google_protobuf/src) and bzlmod (external/protobuf~/src) modesbazel/protobuf.patch- Added exports_files for WKT protos with public visibility and updated plugin.proto visibilityTesting:
//test/proto:apikeys_proto_descriptor//test/proto:bookstore_proto_descriptor//test/extensions/filters/http/proto_api_scrubber:scrubber_test_proto_descriptorWhat was fixed:
In Protobuf v29+, the build structure changed with bzlmod support. The old aliases
@com_google_protobuf//:well_known_type_protosand@com_google_protobuf//:descriptor_proto_srcsresolve to file targets in subpackages that are not visible to external targets, causing build failures when using bzlmod.The fix explicitly references the proto files from their correct subpackages and patches the protobuf BUILD files to export these files with public visibility. Additionally, it uses dynamic path resolution to compute the protobuf include directory at build time, making it compatible with both WORKSPACE and bzlmod modes.
Technical Details:
The key innovation is using bash parameter expansion in the genrule to extract the protobuf src directory at build time:
This removes the
/google/protobuf/*suffix from the resolved path, leaving us with the correct include directory regardless of whether we're in WORKSPACE mode (external/com_google_protobuf/src) or bzlmod mode (external/protobuf~/src), without hardcoding repository names.Original prompt
This pull request was created from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.