proto_format: Optimize and cleanup #26496

phlax · 2023-04-01T16:38:14Z

This PR:

optimizes proto_sync by creating a diff index and only transferring changes
consolidates current proto format algos into a single bazel target
updates headers in api files to point to proto_format.sh as proto_sync will move soon and it doesnt make sense to call it directly
adds an (optional) BAZEL_VOLATILE_DIRTY action env to bust local build stamps on tree change
removes freeze args from proto_format (this is now redundant - v3 forever!)

This uses stamped local build jobs to create a list of local api (git) hashes and a list of local modifications

The normalized api directory also has hashes created for comparison

normalized versions of any mismatched or modified files are downloaded and diffed to ensure the local versions match the normalized ones

this minimizes upload to bazel (only local diff files uploaded), RBE/bazel then processes the changes and gives back the minimum required for comparison (only normalized versions of changed files)

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:]

tools/proto_format/format_api.py

phlax · 2023-04-02T10:14:59Z

There is quite a bit that doesnt work with the proto formatting that is not really fixed by this PR - eg handling proto removal and addition. At least in my tests it breaks the same ways before/after

Mostly what is required is better error handling, there are a lot of assumptions - eg that proto files are valid/well-formed - that dont necessarily stand up, which can have unpredictable consequences, and prevents the tool from being idempotent

repokitteh-read-only · 2023-04-02T10:46:48Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #26496 was synchronize by phlax.

see: more, trace.

tools/proto_format/fetch_normalized_changes.py

phlax · 2023-04-02T13:43:57Z

i have tested a few scenarios

random files on fs

current: get deleted
new: left alone

unwanted proto or BUILD files

removed in both cases

incorrect `api/versioning/BUILD` or `api/BUILD`

current: silently updated (return code 0)
new: diff/fix shown/applied correctly

this depends on what is mismatched - neither way does anything useful in the majority of cases

mismatched proto imports <> BUILD file

depends on what changed - in many cases the tool borks - anything i tested works same before/after

normalizing proto format

both ways work pretty much the same

phlax · 2023-04-02T16:47:16Z

@htuch i should stress that this is a step towards combining proto_format with envoy.code.check the main aim is to minimize the amount of work done on the host machine

phlax · 2023-04-02T17:53:50Z

tools/proto_format/proto_sync.py

+# TODO(phlax): Move all of this code to `envoy.code.check`
+
+
+class Git(object):


by way of explanation - this is here to avoid a dependency on gitpython or similar and to avoid the more expensive way of creating a python git repo and deriving the git hash/headers/diff from that (or shelling out and doing the same)

Why avoid this dependency? I kind of feel there isn't a lot of value in us maintaining this code.

its not just the dependency - it would add as much code and is less efficient - as discussed offline

Ad discussed offline, I think I disagree with us maintaining our own git library and we should either find a more minimal version of what gitpython does or create a separate repository for this and make it its own thing. I'm not including coverage of correctness here in the review as a result.

ok - thanks for not blocking

its a minimal bit of code that will move asap, i believe its superuseful for when you want to create git appliable diffs without having to create repos to do it

ill move it asap

Signed-off-by: Ryan Northey <[email protected]>

htuch

@phlax thanks and I think most of these make sense. Can I ask you to split this into a few PRs? In particular, can you move the super noisy mechanical trivial changes into their own? I prefer the philosophy that PRs should be long and trivial or small and non-trivial, but never long and non-trivial. Thanks.

Signed-off-by: Ryan Northey <[email protected]>

htuch · 2023-04-04T12:11:05Z

tools/proto_format/format_api.py

+
+licenses(["notice"])  # Apache 2
+
+# This tracks active development versions of protos.


I think the biggest cleanup we'll see is to remove the versioning machinery, e.g. things like active / frozen distinction. We don't need to do this in this PR, but I think this will be a major win for simplification and performance.

yep, agreed - i will do that as i go

Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: River Phillips <[email protected]>

phlax force-pushed the format-hashed-api branch 6 times, most recently from 832bf86 to ddbccbb Compare April 1, 2023 17:50

phlax changed the title ~~proto_format: Optimize and cleanup~~ [WIP] proto_format: Optimize and cleanup Apr 1, 2023

phlax marked this pull request as draft April 1, 2023 17:52

phlax force-pushed the format-hashed-api branch 5 times, most recently from 27b2529 to 391acc2 Compare April 2, 2023 09:45

phlax commented Apr 2, 2023

View reviewed changes

tools/proto_format/format_api.py Show resolved Hide resolved

phlax force-pushed the format-hashed-api branch from 391acc2 to f5c1fec Compare April 2, 2023 09:57

phlax force-pushed the format-hashed-api branch from f5c1fec to 94eb5f1 Compare April 2, 2023 10:46

repokitteh-read-only bot added v2-freeze api labels Apr 2, 2023

repokitteh-read-only bot assigned htuch Apr 2, 2023

phlax force-pushed the format-hashed-api branch 2 times, most recently from a27d4cf to 3d9da51 Compare April 2, 2023 11:26

phlax commented Apr 2, 2023

View reviewed changes

tools/proto_format/fetch_normalized_changes.py Outdated Show resolved Hide resolved

phlax force-pushed the format-hashed-api branch 5 times, most recently from d66536a to d332976 Compare April 2, 2023 12:24

phlax force-pushed the format-hashed-api branch 3 times, most recently from 40c6bb3 to 739487f Compare April 2, 2023 16:26

phlax changed the title ~~[WIP] proto_format: Optimize and cleanup~~ proto_format: Optimize and cleanup Apr 2, 2023

phlax marked this pull request as ready for review April 2, 2023 16:39

phlax force-pushed the format-hashed-api branch from 739487f to 363cda6 Compare April 2, 2023 16:41

phlax commented Apr 2, 2023

View reviewed changes

proto_format: Optimize and cleanup

7195c15

Signed-off-by: Ryan Northey <[email protected]>

phlax force-pushed the format-hashed-api branch from 363cda6 to 7195c15 Compare April 3, 2023 20:52

htuch suggested changes Apr 4, 2023

View reviewed changes

revert-header-changes

1c6e771

Signed-off-by: Ryan Northey <[email protected]>

htuch reviewed Apr 4, 2023

View reviewed changes

htuch approved these changes Apr 4, 2023

View reviewed changes

phlax enabled auto-merge (squash) April 4, 2023 12:14

phlax merged commit bd346d5 into envoyproxy:main Apr 4, 2023

RiverPhillips pushed a commit to RiverPhillips/envoy that referenced this pull request Apr 7, 2023

proto_format: Optimize and cleanup (envoyproxy#26496)

71eaee4

Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: River Phillips <[email protected]>

phlax mentioned this pull request Apr 10, 2023

bazel/ci: Cleanups/fix/update #26630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proto_format: Optimize and cleanup #26496

proto_format: Optimize and cleanup #26496

phlax commented Apr 1, 2023 •

edited

Loading

phlax commented Apr 2, 2023 •

edited

Loading

repokitteh-read-only bot commented Apr 2, 2023

phlax commented Apr 2, 2023 •

edited

Loading

phlax commented Apr 2, 2023

phlax Apr 2, 2023 •

edited

Loading

htuch Apr 4, 2023

phlax Apr 4, 2023

htuch Apr 4, 2023

phlax Apr 4, 2023

htuch left a comment

htuch Apr 4, 2023

phlax Apr 4, 2023

		# TODO(phlax): Move all of this code to `envoy.code.check`


		class Git(object):


		licenses(["notice"]) # Apache 2

		# This tracks active development versions of protos.

proto_format: Optimize and cleanup #26496

proto_format: Optimize and cleanup #26496

Conversation

phlax commented Apr 1, 2023 • edited Loading

phlax commented Apr 2, 2023 • edited Loading

repokitteh-read-only bot commented Apr 2, 2023

phlax commented Apr 2, 2023 • edited Loading

random files on fs

unwanted proto or BUILD files

incorrect api/versioning/BUILD or api/BUILD

mismatched proto imports <> BUILD file

normalizing proto format

phlax commented Apr 2, 2023

phlax Apr 2, 2023 • edited Loading

Choose a reason for hiding this comment

htuch Apr 4, 2023

Choose a reason for hiding this comment

phlax Apr 4, 2023

Choose a reason for hiding this comment

htuch Apr 4, 2023

Choose a reason for hiding this comment

phlax Apr 4, 2023

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

htuch Apr 4, 2023

Choose a reason for hiding this comment

phlax Apr 4, 2023

Choose a reason for hiding this comment

phlax commented Apr 1, 2023 •

edited

Loading

phlax commented Apr 2, 2023 •

edited

Loading

phlax commented Apr 2, 2023 •

edited

Loading

incorrect `api/versioning/BUILD` or `api/BUILD`

phlax Apr 2, 2023 •

edited

Loading