build: cleaning up visibility rules#20592
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
requested review from
dio,
ggreenway,
lizan,
mattklein123,
snowp and
wbpcode
as code owners
|
want to land this before the other one :-P |
|
/wait want a working build and thent o land this |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
changed the title
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
/retest |
|
Retrying Azure Pipelines: |
| "-DWASM_USE_CEL_PARSER", | ||
| ], | ||
| }), | ||
| visibility = ["//visibility:public"], |
There was a problem hiding this comment.
This should be reverted to public, since Wasm C++ extensions depend on it.
There was a problem hiding this comment.
I don't think this should be public, as we don't want Envoy core code depending on it. The visibility rules are to prevent dependency leakage upstream (see the associaed issue)
source/extensions/extensions_build_config.bzl allows you to define overrides for downstream visibility changes, so if we're having trouble with import we can override there. Alternately if we want a rule for WASM_VISIBILTY which in envoy defaults to EXTENSION_VISIBILITY but for istio/import can be more broad I'd be happy to add that.
There was a problem hiding this comment.
This is a different situation than extensions. Null VM binaries are linked against the headers @proxy_wasm_cpp_host//:null_lib which are implemented by this library. So to run tests that are not associated with any extension, we need some runtime. Are you suggesting to create an extension for this target that can be exposed to unit tests in downstream repos?
There was a problem hiding this comment.
Where are those binaries linked? I'm still not understanding why the default visibility rules cant' be changed for the use case which is encountering the problem.
cc @phlax @lizan @mattklein123
There was a problem hiding this comment.
The intent here is to actually encourage using Wasm C++ ABI via @proxy_wasm_cpp_host, so hiding the implementation seems like going against it. If you insist, we can add a variable to control visibility, but it seems it's the easiest to just make Wasm library public.
There was a problem hiding this comment.
Sample binary https://github.com/istio/proxy/blob/master/extensions/common/BUILD#L95. It's a test for a library in a Wasm extension.
There was a problem hiding this comment.
I will admit that I don't fully understand the moving pieces here, but are you saying that this is not really an extension but basically an export library that must be linked into null-VM extensions? How would one then link the null VM extension into Envoy itself? Via the normal mechanisms?
Is there some way we could make it easy to just expose this export library (and I would rename to wasm_export_lib or something to make it clear) to consuming extensions? I'm not sure what is possible with bazel.
There was a problem hiding this comment.
Linking the Wasm extension using envoy_extension_cc_test into tests solves the issue. Sorry for the noise. I was confused myself since these tests don't really import anything Envoy except proxy-wasm headers, so it's a bit awkward but not a big deal.
Removing unnecessary public visibility from extensions. Changing the visibility rules of extensions to be visible from contrib and examples. We do this for the specific extensions which need it, but I'm comfortable leaving it as blanket visibility and it will avoid churn as other deps are needed. Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
4 participants
Removing unnecessary public visibility from extensions.
Changing the visibility rules of extensions to be visible from contrib and examples. We do this for the specific extensions which need it, but I'm comfortable leaving it as blanket visibility and it will avoid churn as other deps are needed.