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

Add canonical CEL support: dev.cel.expr (rules_python approach) #69

Closed
wants to merge 2 commits into from

Conversation

sergiitk
Copy link
Contributor

@sergiitk sergiitk commented Oct 6, 2023

No description provided.

@sergiitk
Copy link
Contributor Author

sergiitk commented Oct 6, 2023

At the moment this fails because google/cel-spec doesn't provide python proto libraries (see https://dev.azure.com/cncf/xds/_build/results?buildId=151440&view=results):

❯ bazel build //xds/type/v3:*
ERROR: /Users/sergiitk/Development/protos/xds/xds/type/v3/BUILD:5:18: no such target '@dev_cel//proto/cel/expr:checked_proto_py_proto': target 'checked_proto_py_proto' not declared in package 'proto/cel/expr' defined by /private/var/tmp/_bazel_sergiitk/b42f1414e9b2a5df5de7bcf9dd0bd497/external/dev_cel/proto/cel/expr/BUILD.bazel and referenced by '//xds/type/v3:pkg_py_proto'
ERROR: /Users/sergiitk/Development/protos/xds/xds/type/v3/BUILD:5:18: no such target '@dev_cel//proto/cel/expr:syntax_proto_py_proto': target 'syntax_proto_py_proto' not declared in package 'proto/cel/expr' defined by /private/var/tmp/_bazel_sergiitk/b42f1414e9b2a5df5de7bcf9dd0bd497/external/dev_cel/proto/cel/expr/BUILD.bazel and referenced by '//xds/type/v3:pkg_py_proto'
ERROR: Analysis of target '//xds/type/v3:pkg_py_proto' failed; build aborted: Analysis failed

Created google/cel-spec#318 to add the support.

@sergiitk sergiitk force-pushed the cel-expression-new-fields branch 2 times, most recently from aaf69d1 to 7ad233b Compare October 6, 2023 13:55
@sergiitk sergiitk changed the title Add stable CEL support: dev.cel.expr Add canonical CEL support: dev.cel.expr Nov 9, 2023
@sergiitk sergiitk changed the title Add canonical CEL support: dev.cel.expr Add canonical CEL support: dev.cel.expr (rules_python approach) Nov 10, 2023
@sergiitk
Copy link
Contributor Author

Superseded by another approach: #74, #75.

@sergiitk sergiitk closed this Nov 15, 2023
htuch pushed a commit that referenced this pull request Nov 16, 2023
py_proto_library provided by @com_google_protobuf//:protobuf.bzl has been deprecated for a while now:

This is provided for backwards compatibility only. Bazel 5.3 will
introduce support for py_proto_library, which should be used instead.
https://github.com/protocolbuffers/protobuf/blob/32af7d211b85f71920acdfa9b796db008f8c2a45/protobuf.bzl#L642-L644

However, native py_proto_library has never been provided, see bazelbuild/bazel#3935. Instead @rules_python//python:proto.bzl is recommended. I attempted switching to this library, but it's not compatible with @com_google_googleapis's py_proto_library targets, see #69. I found a hacky workaround by using cc_proto_library to generate python targets, but downstream integration into Envoy failed (envoyproxy/envoy#30159).

This PR migrates py_proto_library implementation to to @com_github_grpc_grpc. This implementation is used by @com_google_googleapis's, and, more importantly, uses bazel aspects. Which decouples cncf/xds and Envoy's dependencies from concrete upstream py_proto_library implementations.

This resulted in a significant code improvement of bazel/api_build_system.bzl:

No more custom @com_google_googleapis dependency mapping needed via py_proto_library rules override.
No more hardcoded dependencies _xds_py_proto_library - proto dependency tree is determined from proto_library definitions via Basel aspects.
No more EXTERNAL_PROTO_PY_BAZEL_DEP_MAP dependency map needed - for the same reason.
Similar work in Envoy: envoyproxy/envoy#30834.

Signed-off-by: Sergii Tkachenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant