Remove dependency to @com_google_protobuf//src/google/protobuf/compiler:retention without visibility

[mering]

Description:

Envoy API depends on the target @com_google_protobuf//src/google/protobuf/compiler:retention which does not grant visibility outside of Protobuf itself:

envoy/api/bazel/cc_proto_descriptor_library/BUILD

Line 40 in d1f6f3c

"@com_google_protobuf//src/google/protobuf/compiler:retention",

In the WORKSPACE model, this is patched out via

envoy/bazel/protobuf.patch

Lines 116 to 128 in d1f6f3c

	diff --git a/src/google/protobuf/compiler/BUILD.bazel b/src/google/protobuf/compiler/BUILD.bazel
	index 3f5624d5f..e194b172f 100644
	--- a/src/google/protobuf/compiler/BUILD.bazel
	+++ b/src/google/protobuf/compiler/BUILD.bazel
	@@ -517,7 +517,7 @@ cc_library(
	srcs = ["retention.cc"],
	hdrs = ["retention.h"],
	strip_include_prefix = "/src",
	- visibility = ["//src/google/protobuf:__subpackages__"],
	+ visibility = ["//visibility:public"],
	deps = [
	"//src/google/protobuf",
	"//src/google/protobuf:port",

Downstream patches are not allowed with Bzlmod as this breaks automatic dependency resolution (see #34355 for a WIP PR adding Bzlmod support). We need to find a way to make envoy compatible with upstream Protobuf without patches or upstream necessary patches.

This is blocking addition of Bzlmod support. Bzlmod support was introduced as experimental in Bazel 5, fully introduced in Bazel 6, defaulted in Bazel 7, WORKSPACE support is disabled by default in Bazel 8 and will be removed in Bazel 9.

[phlax]

Downstream patches are not allowed with Bzlmod

@mering is this correct? my understanding is that upstream deps can be patched in much the same way as they can with workspace

[mering]

@phlax overrides (which allow patches) can only be used in the root module or more specifically are ignored in dependent modules (see docs) as they break dependency resolution.

Imagine A depends on B and C which both depend on D (diamond problem). Now imagine both B and C patching D (differently). There is no way to resolve this problem automatically. One could depend on the same dependency twice (just giving it a different name) but one quickly runs into ORD violations in this case. Only A can solve this (root module) by manually combining all patches of all transitive dependencies. This defeats the whole purpose of Bzlmod with automatic dependency resolution as one ends up managing transitive dependencies in the root module again as one was doing in WORKSPACE.

On the other hand, this encourages upstreaming patches such that everyone can benefit from them. Furthermore, this reduces downstream maintenance significantly and makes (transitive) dependency updates much smoother. Combined with the joint community maintenance of BUILD files (for non Bazel dependencies) in the BCR, the maintenance reduction is significant.

[phlax]

Now imagine both B and C patching D (differently). There is no way to resolve this problem automatically

for sure we have a lot of patches and regularly have to resolve their shortcomings

trying to understand the distinction of the root module ...

so eg if envoy deps on envoy_api, and envoy_api on protobuf - then any patches that envoy_api applies to protobuf would not happen automatically - envoy would need to explicitly declare the protobuf dep and add the patches that envoy_api has

is that not the same for workspace?

[phlax]

hmm - actually thinking about it - in workspace if we call envoy_api_deps() or somesuch that would apply the patches

i think ive got it - so from our pov, as long as we are happy to carry whatever patches directly it wouldnt matter so much, but from our downstreams pov it would make life more difficult (assuming they previously just called envoy_deps() or somesuch)

[mering]

trying to understand the distinction of the root module ...

so eg if envoy deps on envoy_api, and envoy_api on protobuf - then any patches that envoy_api applies to protobuf would not happen automatically - envoy would need to explicitly declare the protobuf dep and add the patches that envoy_api has

is that not the same for workspace?

Yes this is correct. In WORKSPACE, every Bazel workspace (similar to root modules, basically where you run your Bazel commands) has to declare all dependencies including all transitive ones. As this adds up quite quickly, people started using macros to declare dependencies based on the definition from another repo (for example envoy_deps()). The problems of finding a suitable transitive dependency which fulfills the need of all consumers is usually a big mess, very time consuming and often requires adding even more patches on top.

So in your example, not only envoy as root module would need to add such patches but also every other dependency depending on envoy_api (like gRPC including all of their dependents) would have to add these patches. Also this is a blocker of adding this dependency to BCR for easy consumption of users. So at least envoy_api should work without patches.

[phlax]

tbh - i think eliminating all of our existing patches and any future need to ever require them is a losing battle

patches are generally regarded as a last resort but for so many things they are 100% necessary (without maintaining a fork or somesuch)

protobuf in particular is going to be more/less impossible to do that with

[mering]

For envoy_api the Protobuf visibility issue, which this issue is about, is the only patch (outside of this repo) required to make it build with Bzlmod.

[phlax]

not so familiar with BCR yet, so not clear how that works there - but i think, in general, we will need some way of documenting and making explicit any patches that we require

[mering]

You cannot upload to BCR if you require patches for your dependencies.

[mering]

So for this specific case, I see the following options:

1. Upstream the patch
2. Remove the need to depend on the retention target
3. Fork the used functionality

[phlax]

yeah was just wondering similar - it seems its the StripSourceRetentionOptions fun that is being used - perhaps upstream can be encouraged to move that to somewhere that is exported

[phlax]

altho in @keith 's PR he seems to have just removed the bazel dep - not sure why that didnt fail

[mering]

altho in @keith 's PR he seems to have just removed the bazel dep - not sure why that didnt fail

There just doesn't seem to be any users of that macro creating such a target within /api/.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.