Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: remove v2 dependencies from v3 API protos. #18817

Merged
merged 5 commits into from
Oct 29, 2021

Conversation

htuch
Copy link
Member

@htuch htuch commented Oct 28, 2021

This should reduce the binary size, which is particularly important for Envoy Mobile. Looking at a
local opt build with debug symbols, I'm seeing a drop from ~400MB to ~380MB, so maybe 5% saving. @Reflejo indicates that optimized Envoy Mobile without symbols is observing ~20% improvement.

Related to #10943

Risk level: Low
Testing: bazel query deps to confirm no more v2 API deps.

Signed-off-by: Harvey Tuch htuch@google.com

This should reduce the binary size, which is particularly important for Envoy Mobile. Looking at a
local opt build with debug symbols, I'm seeing a drop from ~400MB to ~380MB, so maybe 5% saving.

Related to envoyproxy#10943

Risk level: Low
Testing: bazel query deps to confirm no more v2 API deps.

Signed-off-by: Harvey Tuch <htuch@google.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18817 was opened by htuch.

see: more, trace.

Copy link
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.

Awesome! Check CI?

/wait

@phlax
Copy link
Member

phlax commented Oct 28, 2021

not sure why this has triggered a flake8 error

  File "/build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/k8-fastbuild/bin/tools/code_format/python_check.runfiles/base_pip3_pypi__flake8/flake8/formatting/base.py", line 187, in _write
    sys.stdout.buffer.write(output.encode() + self.newline.encode())
AttributeError: '_io.StringIO' object has no attribute 'buffer'

ill investigate further...

@phlax
Copy link
Member

phlax commented Oct 28, 2021

ill investigate further...

this is the problem we are hitting - or at least the best explanation

tholo/pytest-flake8#81

unrelated to this PR, but the error message is most unhelpful

we can either downgrade flake8 package or add a workaround

@htuch in the meantime running flake8 directly, shows the problem that will unblock CI

$ pip3 install flake8
$ flake8 .
./tools/proto_format/proto_sync.py:300:62: E261 at least two spaces before inline comment
./tools/proto_format/proto_sync.py:300:63: E262 inline comment should start with '# '

Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
@mattklein123
Copy link
Member

CI still seems broken. :(

/wait

Copy link
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.

Yay!

htuch pushed a commit that referenced this pull request Oct 29, 2021
Fixes issue discussed here #18817 (comment)

Signed-off-by: Ryan Northey <ryan@synca.io>
@htuch htuch enabled auto-merge (squash) October 29, 2021 20:24
jasonmillerstackpath pushed a commit to jasonmillerstackpath/envoy that referenced this pull request Oct 29, 2021
Fixes issue discussed here envoyproxy#18817 (comment)

Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: phlax <phlax@users.noreply.github.com>
@htuch htuch merged commit 53fca61 into envoyproxy:main Oct 29, 2021
@htuch htuch deleted the remove-v2-api-refs branch October 29, 2021 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants