Skip to content

proto_format: Prebuild formatted api for proto_sync#21769

Merged
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:proto-sync-ab
Jun 21, 2022
Merged

proto_format: Prebuild formatted api for proto_sync#21769
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:proto-sync-ab

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Jun 18, 2022 •
edited
Loading

Commit Message:
Additional Description:

Currently proto_sync compares 2 "api" directories:

  • an ideal one created from what bazel can see
  • what is actually on the local fs

in the case of the former this can be built hermetically and benefit from bazels caching (and potentially incremental builds)

this PR makes the creation of the "formatted api directory" hermetic and separates it from the run job in which the directories are compared

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 repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jun 18, 2022
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 18, 2022

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 @mattklein123

🐱

Caused by: #21769 was opened by phlax.

see: more, trace.

@phlax phlax marked this pull request as draft June 18, 2022 15:04
@phlax phlax force-pushed the proto-sync-ab branch 4 times, most recently from 4600cbb to 5032ede Compare June 18, 2022 17:37
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jun 18, 2022 •
edited
Loading

~this seems to work pretty reliably in CI breaking/fixing protos and testng ~

~it halves the time to run the proto_format job, perhps more if i wasnt blowing the cache

re bazel cache - im not 100% how it will work currently locally and making changes, i will test further

(approach changed as described below)

@phlax phlax force-pushed the proto-sync-ab branch 5 times, most recently from 0782a8e to 582b419 Compare June 19, 2022 11:24
@phlax phlax changed the title [WIP] proto_format: Separate generation of comparison fs proto_format: Prebuild formatted api for proto_sync Jun 19, 2022
@phlax phlax changed the title proto_format: Prebuild formatted api for proto_sync [WIP] proto_format: Prebuild formatted api for proto_sync Jun 20, 2022
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jun 20, 2022 •
edited
Loading

i tested this out further and realized that only the "ideal" representation of api fs can be prebuilt - the "actual" needs to use what is currently on the fs so cannot be hermetic

prebuilding only the ideal api (ie the one built from what bazel can see) speeds this up a lot in almost every situation (uncached run having the least benefit, and may be slower)

i have also taken this a step further and switched the protoprint jobs that are currently run in a python multiproc pool inside proto_sync.py to use an aspect (#21750)

this seems to speed things up quite a bit - altho in the uncached run it is doing a bit more work as it currently only protoprints changed files, whereas this lets bazel manage it, meaning it has to check all the first time

running protoxform/protoprint against all files there are a few v2 protos that fail formatting - not sure if we would be better just fixing these, or to add an exclusion for them (this is fixed it was an issue with frozen protos)

cc @htuch

@phlax phlax mentioned this pull request Jun 20, 2022
Merged
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jun 20, 2022 •
edited
Loading

running protoxform/protoprint against all files there are a few v2 protos that fail formatting - not sure if we would be better just fixing these, or to add an exclusion for them

after testing this further this seems to be an issue with my impiementation so will debug further - i have for that reason split out the part shifting protoprint to an aspect (to #21750 ) as i think this can/should land more quickly and that PR throws up other issues which i am addressing separately

(^^ fixed but kept in separate PR as there are quite a few changes required to use an aspect)

@phlax phlax changed the title [WIP] proto_format: Prebuild formatted api for proto_sync proto_format: Prebuild formatted api for proto_sync Jun 20, 2022
@phlax phlax marked this pull request as ready for review June 20, 2022 10:22
phlax
phlax commented Jun 20, 2022
View reviewed changes
tools/proto_format/BUILD Outdated
Copy link
Copy Markdown
Member Author

@phlax phlax Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this is pretty annoying - it used to be that to create a pkg_tar and preserve dir structure you just gave it srcs - now to do this you must have 2 rules and this weird strip_prefix invokation

@phlax phlax force-pushed the proto-sync-ab branch 2 times, most recently from fc33f82 to e035dae Compare June 20, 2022 10:58
phlax
phlax commented Jun 20, 2022
View reviewed changes
phlax
phlax commented Jun 20, 2022
View reviewed changes
@phlax
proto_format: Prebuild formatted api for proto_sync
bd988e7 
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jun 21, 2022

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 21, 2022

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21769 (comment) was created by @phlax.

see: more, trace.

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Jun 21, 2022

@phlax would you mind updating the PR description with details about what this change does? Thank you.

/wait-any

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jun 21, 2022

done

mattklein123
mattklein123 approved these changes Jun 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@phlax phlax merged commit 6247cec into envoyproxy:main Jun 21, 2022
Amila-Rukshan pushed a commit to Amila-Rukshan/envoy that referenced this pull request Jun 28, 2022
@phlax @Amila-Rukshan
proto_format: Prebuild formatted api for proto_sync (envoyproxy#21769)
1a9a1e6 
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>
@phlax phlax mentioned this pull request Aug 1, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@mattklein123 mattklein123

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@phlax @mattklein123