Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions tools/api_proto_plugin/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ py_library(
"type_context.py",
"visitor.py",
],
srcs_version = "PY3",
visibility = ["//visibility:public"],
deps = [
"@com_google_protobuf//:protobuf_python",
Expand All @@ -22,7 +21,6 @@ py_library(
py_library(
name = "utils",
srcs = ["utils.py"],
srcs_version = "PY3",
visibility = ["//visibility:public"],
)

Expand Down
2 changes: 2 additions & 0 deletions tools/proto_format/proto_format.sh
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ bazel build ${BAZEL_BUILD_OPTIONS} --//tools/api_proto_plugin:default_type_db_ta
TOOLS=$(dirname $(dirname $(realpath $0)))
# to satisfy dependency on api_proto_plugin
export PYTHONPATH="$TOOLS"
# Build protoprint for use in proto_sync.py.
bazel build ${BAZEL_BUILD_OPTIONS} //tools/protoxform:protoprint
./tools/proto_format/proto_sync.py "--mode=$1" ${PROTO_TARGETS}

bazel build ${BAZEL_BUILD_OPTIONS} //tools/type_whisperer:api_build_file
Expand Down
59 changes: 44 additions & 15 deletions tools/proto_format/proto_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
# Diff or copy protoxform artifacts from Bazel cache back to the source tree.

import argparse
import functools
import multiprocessing as mp
import os
import pathlib
import re
Expand Down Expand Up @@ -46,7 +48,7 @@

IMPORT_REGEX = re.compile('import "(.*)";')
SERVICE_REGEX = re.compile('service \w+ {')
PACKAGE_REGEX = re.compile('\npackage ([^="]*);')
PACKAGE_REGEX = re.compile('\npackage: "([^"]*)"')
PREVIOUS_MESSAGE_TYPE_REGEX = re.compile(r'previous_message_type\s+=\s+"([^"]*)";')


Expand Down Expand Up @@ -87,20 +89,44 @@ def GetDestinationPath(src):
matches[0])).joinpath(src_path.name.split('.')[0] + ".proto")


def SyncProtoFile(cmd, src, dst_root):
"""Diff or in-place update a single proto file from protoxform.py Bazel cache artifacts."
def GetAbsDestinationPath(dst_root, src):
"""Obtain absolute path from a proto file path combined with destination root.

Creates the parent directory if necessary.

Args:
cmd: 'check' or 'fix'.
dst_root: destination root path.
src: source path.
"""
# Skip empty files, this indicates this file isn't modified in this version.
if os.stat(src).st_size == 0:
return []
rel_dst_path = GetDestinationPath(src)
dst = dst_root.joinpath(rel_dst_path)
dst.parent.mkdir(0o755, True, True)
shutil.copyfile(src, str(dst))
dst.parent.mkdir(0o755, parents=True, exist_ok=True)
return dst


def ProtoPrint(src, dst):
"""Pretty-print FileDescriptorProto to a destination file.

Args:
src: source path for FileDescriptorProto.
dst: destination path for formatted proto.
"""
print('ProtoPrint %s' % dst)
subprocess.check_output([
'bazel-bin/tools/protoxform/protoprint', src,
str(dst),
'./bazel-bin/tools/protoxform/protoprint.runfiles/envoy/tools/type_whisperer/api_type_db.pb_text'
])


def SyncProtoFile(src_dst_pair):
"""Diff or in-place update a single proto file from protoxform.py Bazel cache artifacts."

Args:
src_dst_pair: source/destination path tuple.
"""
rel_dst_path = GetDestinationPath(src_dst_pair[0])
ProtoPrint(*src_dst_pair)
return ['//%s:pkg' % str(rel_dst_path.parent)]


Expand Down Expand Up @@ -250,17 +276,20 @@ def GitStatus(path):


def Sync(api_root, mode, labels, shadow):
pkg_deps = []
with tempfile.TemporaryDirectory() as tmp:
dst_dir = pathlib.Path(tmp).joinpath("b")
paths = []
for label in labels:
pkg_deps += SyncProtoFile(mode, utils.BazelBinPathForOutputArtifact(label, '.active.proto'),
dst_dir)
pkg_deps += SyncProtoFile(
mode,
paths.append(utils.BazelBinPathForOutputArtifact(label, '.active.proto'))
paths.append(
utils.BazelBinPathForOutputArtifact(
label, '.next_major_version_candidate.envoy_internal.proto'
if shadow else '.next_major_version_candidate.proto'), dst_dir)
if shadow else '.next_major_version_candidate.proto'))
src_dst_paths = [
(path, GetAbsDestinationPath(dst_dir, path)) for path in paths if os.stat(path).st_size > 0
]
with mp.Pool() as p:
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to resort to multiprocessing here to get reasonable performance. It isn't much slower than before on my workstation, but it might be slower on less powerful machines. Ultimately, when we move this all to Bazel native, this performance delta won't matter as we won't be rebuilding unchanged files, but that's post 1.14.0.

Copy link
Member

Choose a reason for hiding this comment

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

I think this does affect performance on CI though?

Copy link
Member Author

Choose a reason for hiding this comment

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

CI for format in this run was 12m 52s vs. 8m 59s for a recent PR involving proto changes (https://dev.azure.com/cncf/envoy/_build/results?buildId=36485&view=logs&j=9fd74226-df5b-5f29-39da-76fc341cc763).

It's a regression, but arguably a small fraction of total CI time. Not sure if there is a better way of tackling this without Bazelifying stuff, which is a major project that should happen after the v2 freeze. We need to be able to slice-and-dice the descriptors for the hand edited v3 and combine with v2 upgrade generated shadows somehow.. suggestions welcome :)

Copy link
Member

Choose a reason for hiding this comment

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

Given issues we have had with non-deterministic behavior with multi-threading are you confident this is thread safe / deterministic?

pkg_deps = p.map(SyncProtoFile, src_dst_paths)
SyncBuildFiles(mode, dst_dir)

current_api_dir = pathlib.Path(tmp).joinpath("a")
Expand Down
23 changes: 21 additions & 2 deletions tools/protoxform/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,30 @@ py_binary(
"protoxform.py",
"utils.py",
],
data = ["//:.clang-format"],
python_version = "PY3",
visibility = ["//visibility:public"],
deps = [
"//tools/api_proto_plugin",
"//tools/type_whisperer:api_type_db_proto_py_proto",
"@com_envoyproxy_protoc_gen_validate//validate:validate_py",
"@com_github_cncf_udpa//udpa/annotations:pkg_py_proto",
"@com_google_googleapis//google/api:annotations_py_proto",
"@envoy_api_canonical//envoy/annotations:pkg_py_proto",
],
)

py_binary(
name = "protoprint",
srcs = [
"options.py",
"protoprint.py",
"utils.py",
],
data = [
"//:.clang-format",
"//tools/type_whisperer:api_type_db.pb_text",
],
visibility = ["//visibility:public"],
deps = [
"//tools/type_whisperer",
"//tools/type_whisperer:api_type_db_proto_py_proto",
"@com_envoyproxy_protoc_gen_validate//validate:validate_py",
Expand Down
Loading