Skip to content

Conversation

@sergiitk
Copy link
Contributor

DO NOT MERGE

This is an experiment at this moment.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #30159 was opened by sergiitk.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Oct 12, 2023
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @htuch

🐱

Caused by: #30159 was opened by sergiitk.

see: more, trace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know when ready for review. It looks like this is still pointing at a branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - this is still a draft. I'll click "Ready for review" when it's ready.

@sergiitk sergiitk force-pushed the cncf-xds-stable-cel branch from dd46e8e to d9b38de Compare October 30, 2023 22:23
@moderation
Copy link
Contributor

@phlax, @htuch at some point it would be nice to repoint to the correct repo - https://github.com/cncf/xds - and change all the references to udpa. There are a lot of references to the incorrect repo - https://github.com/search?q=repo%3Aenvoyproxy%2Fenvoy+com_github_cncf_udpa&type=code

40:            f.path.startswith("external/com_github_cncf_udpa/xds"))

tools/protoxform/protoxform.bzl
27:                path.short_path.startswith("../com_github_cncf_udpa") or

tools/protodoc/protodoc.bzl
23:                path.short_path.startswith("../com_github_cncf_udpa")

api/bazel/repositories.bzl
29:        name = "com_github_cncf_udpa",

api/bazel/external_proto_deps.bzl
25:    # @com_github_cncf_udpa//xds/type/matcher/v3:pkg_go_proto

api/bazel/repository_locations.bzl
37:    com_github_cncf_udpa = dict(

DO NOT MERGE

Signed-off-by: Sergii Tkachenko <[email protected]>
Signed-off-by: Sergii Tkachenko <[email protected]>
Signed-off-by: Sergii Tkachenko <[email protected]>
@sergiitk sergiitk changed the title deps: Bump com_github_cncf_udpa (cncf/xds) deps: Experiment — adopt canonical CEL - py_proto_library support via rules_python Nov 10, 2023
@sergiitk
Copy link
Contributor Author

sergiitk commented Nov 15, 2023

Superseded by another approach: #30834, #30901.

@sergiitk sergiitk closed this Nov 15, 2023
htuch pushed a commit to cncf/xds 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]>
@sergiitk sergiitk deleted the cncf-xds-stable-cel branch January 26, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants