envoy/api/bazel/api_build_system.bzl requires //docs

[mju]

Title: envoy/api/bazel/api_build_system.bzl requires //docs

Description:
When building an Envoy filter, we needed to use api_proto_library, which tries to grant //docs visibility. See

envoy/api/bazel/api_build_system.bzl

Line 89 in 2447cb7

if visibility == ["//visibility:private"]:

. To us, this is a bug. We are working around by adding a dummy folder and BUILD file or adding visibility = ["//visibility:public"] to the rule. We would like to be able to avoid those hacks.

My guess is that the shell scripts under https://github.com/envoyproxy/envoy/tree/2447cb73ef8f103770dab6b1cb154a3a8f02630e/docs run some bazel commands that need that visibility.

Suggestion I got on how to fix this issue:
"""Get rid of this defaulting behavior in api_proto_library (since it is used in many places external to the envoy_api repo). We could add a wrapper for the API repo that does the defaulting only internally."""

Repro steps:
To reproduce this, try to build https://github.com/envoyproxy/envoy-filter-example/tree/master/http-filter-example as a independent repo.

[mattklein123]

@htuch @kyessenov @lizan @jmillikin-stripe thoughts here?

[lizan]

We can modify api_proto_library to sth similar to envoy_cc_* in https://github.com/envoyproxy/envoy/blob/master/bazel/envoy_build_system.bzl, the api_proto_library could take a parameter repository and when it is used outside of envoy, it will grant visibility to repository + "//docs".

[jmillikin-stripe]

This looks like fallout from merging envoy-api/ into envoy/. I'm not familiar with Envoy's proto_library wrappers or docs build, but I can tell the visibility settings make no sense:

• envoy/docs/ has no BUILD files or targets.
• envoy/docs/build.sh runs bazel build @envoy_api//docs:protos
• api_proto_library() is a macro that sets visibility for one target, but also defines C++ and Python proto library targets that appear hard-coded to be //visibility:public.

My recommendation is to remove the //docs bit from api_proto_library().

[htuch]

@jmillikin-stripe the visibility is for api/docs, we do a trick by which the api/ directory is treated as an external repo, so the path is relative to that. This is done to simplify exporting to the data-plane-api mirror.

I do agree broadly that we should not have api_proto_library contain these api/docs specific workarounds, this is bad. What we should do instead is remove this business logic from api_proto_library and then push this visibility responsibility to the BUILD files (as a BUILD file wide setting) in /api.