proto_format: Use aspect to protoprint formatted api#21750
proto_format: Use aspect to protoprint formatted api#21750htuch merged 3 commits intoenvoyproxy:mainfrom
Conversation
proto_format to bazel and refactor to class
proto_format to bazel and refactor to classproto_format to bazel and refactor to class
daed48d to
ae69158
Compare
proto_format to bazel and refactor to classproto_sync to bazel and refactor to class
d4e30de to
435b2d5
Compare
proto_sync to bazel and refactor to classproto_sync to class
cfc302d to
ca34ec4
Compare
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
193ee6b to
a64cc24
Compare
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
proto_sync to class9146144 to
3470614
Compare
b6fd374 to
93affd7
Compare
Signed-off-by: Ryan Northey <ryan@synca.io>
|
this one can be slightly slower in the uncached situation (but only sometimes) - it should be faster in every other situation |
tools/protoprint/protoprint.py
Outdated
| class ProtoprintTraverser: | ||
|
|
||
| def traverse_file(self, file_proto, visitor): | ||
| # This breaks the Visitor contract, ie class props should not be set on individual visitation. |
There was a problem hiding this comment.
this reflects what was being done already in the main function and is i think the reason why this was not implemented as a protoc plugin originally - either way this is necessary without making other changes to traverser/visitor
There was a problem hiding this comment.
Yeah, this is kind of evil. Can you add some explanation of "why" for future readers?
| from bazel_tools.tools.python.runfiles import runfiles | ||
|
|
||
| from tools.api_proto_plugin import annotations, traverse, visitor | ||
| sys.path = [p for p in sys.path if not p.endswith('bazel_tools')] |
There was a problem hiding this comment.
this is the usual hack, as added elsewhere - if you use runfiles you cant import tools without mangling sys.path
There was a problem hiding this comment.
added here because i needed runfiles to get the clang-tidy config - otherwise i generally avoid runfiles if i can
There was a problem hiding this comment.
Can you factor this pattern out into some library so it's clear what is going on?
There was a problem hiding this comment.
not really - its one line and needs to be explicit
this is really nothing new - you and i have added a few of these - and i have removed them whenever i can
There was a problem hiding this comment.
ill add a comment to make it clearer
|
/lgtm deps |
|
no relevant owners for "deps" |
htuch
left a comment
There was a problem hiding this comment.
LGTM modulo this sys.path hack nit.
Signed-off-by: Ryan Northey <ryan@synca.io>
htuch
left a comment
There was a problem hiding this comment.
Thanks - as per discussion on Slack let's land it with comment for later followup.
Commit Message:
This switches from calling protoprint in scripts with python multiproc to using a bazel aspect
Both the
proto_format.shjob and theprotoprint_testare switched to using a prebuilt protoprinted version of the api/test protosAdditional 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:]