Fix unused proto import warnings in api #13401

cheister · 2020-10-06T02:44:20Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: api: fix unused proto import warnings
Additional Description: Fixing "warning: Import ... but not used" warnings from protoc
Risk Level: Low
Testing: manually built protos
Docs Changes: None
Release Notes: None

repokitteh-read-only · 2020-10-06T02:44:28Z

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/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13401 was opened by cheister.

see: more, trace.

rojkov · 2020-10-06T07:16:16Z

To fix the CI failure you need to generate the API shadow with ./tools/proto_format/proto_format.sh fix first and include it in the PR with git add api/ generated_api_shadow/. Ideally after running the unit tests.

phlax · 2020-10-06T07:19:20Z

great @cheister i started to do this, but didnt get very far. (see #13284)

ill close my PR in favour of yours

it might be a bit more complex than it first seems, i hit some errors removing some of the imports, but i might just have been a bit sloppy about how i did it. It will be good to see if tests pass

htuch

Looks great. Please follow the advice in the comments to fix format. Also, I think it would be great having a CI check to ensure that we don't regress after these improvements. Would you be able to add this or maybe just file an issue if you don't have cycles? Thanks.
/wait

cheister · 2020-10-07T00:21:50Z

Thanks, I didn't know about ./tools/proto_format/proto_format.sh fix, I just ran this and updated the PR.

cheister · 2020-10-07T00:23:33Z

A CI check would be nice. The easiest thing would be protocolbuffers/protobuf#3980 if it was done. I probably won't have any time soon to write something for CI.

cheister · 2020-10-07T06:49:14Z

I can't tell if this error is related to this PR or not?

bazel-out/k8-opt/bin/source/extensions/filters/http/grpc_json_transcoder/_virtual_includes/json_transcoder_filter_lib/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h:120:55: error: no member named 'api' in namespace 'google'
                                        const google::api::HttpRule& http_rule,
                                              ~~~~~~~~^
1 error generated.

phlax · 2020-10-07T06:54:13Z

I can't tell if this error is related to this PR or not?

i hit similar problems - ie errors that were hard to track - so i started removing the lines one by one - which was why it was taking so long

github-actions · 2020-12-09T02:58:10Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

htuch · 2020-12-09T22:31:41Z

@cheister anything we can do to unblock?

cheister · 2021-01-19T06:32:29Z

So I started looking at this again and now I'm getting an error when I try to run

FORCE_PROTO_FORMAT=yes ./tools/proto_format/proto_format.sh fix

on the latest main branch. It fails when it runs

bazel build --remote_download_outputs=all --//tools/api_proto_plugin:default_type_db_target=@envoy_api_canonical//versioning:active_protos @envoy_api_canonical//versioning:active_protos --aspects //tools/protoxform:protoxform.bzl%protoxform_aspect --output_groups=proto

and gives the error

  File "/Users/cheister/.cache/bazel/9d68f171b1e2407d88fb2358d1b40dce/sandbox/darwin-sandbox/662/execroot/envoy/bazel-out/darwin-opt-exec-2B5CBBC6/bin/tools/protoxform/protoxform.runfiles/envoy/tools/protoxform/migrate.py", line 14, in <module>
    from google.api import annotations_pb2
ModuleNotFoundError: No module named 'google.api'
--api_proto_plugin_out: protoc-gen-api_proto_plugin: Plugin failed with status code 1.
Aspect //tools/protoxform:protoxform.bzl%protoxform_aspect of @envoy_api_canonical//versioning:active_protos failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 2.957s, Critical Path: 1.00s
INFO: 9 processes: 9 internal.
FAILED: Build did NOT complete successfully

I'm not sure why migrate.py can't find google.api since @com_google_googleapis//google/api:annotations_py_proto is a dependency of the protoxform py_binary in tools/protoxform/BUILD

mattklein123 · 2021-01-19T17:19:53Z

Sorry no idea about the build error. cc @htuch @adisuissa @lizan

/wait

htuch · 2021-01-20T14:54:38Z

@cheister this works fine for me on latest main. I'm not sure if there is some strange artifact of being on Mac OS for you or the local environment. Does ./ci/run_envoy_docker.sh 'FORCE_PROTO_FORMAT=yes ./ci/do_ci.sh check_format' work?

The other thing you might want to try is ensuring you are starting with a clean build cache, bazel clean --expunge.

Signed-off-by: Chris Heisterkamp <[email protected]>

cheister · 2021-02-02T02:36:36Z

@htuch Do you mind taking a look at the test failure?

FAIL: //test/extensions/filters/http/grpc_json_transcoder:grpc_json_transcoder_integration_test (see /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test/test.log)
INFO: From Testing //test/extensions/filters/http/grpc_json_transcoder:grpc_json_transcoder_integration_test:
stdout (/build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/_tmp/actions/stdout-23732) exceeds maximum size of --experimental_ui_max_stdouterr_bytes=1048576 bytes; skipping
[19,673 / 20,472] 459 / 723 tests, 1 failed; Testing //test/extensions/filters/http/grpc_web:grpc_web_integration_test; 64s remote-cache, processwrapper-sandbox ... (58 actions running)

I don't know how to access the test log from CI. Is there a link I'm missing?
Locally bazel test //test/extensions/filters/http/grpc_json_transcoder:grpc_json_transcoder_integration_test fails for me with

Executing tests from //test/extensions/filters/http/grpc_json_transcoder:grpc_json_transcoder_integration_test
-----------------------------------------------------------------------------
dyld: Symbol not found: _program_invocation_name
  Referenced from: /private/var/tmp/_bazel_square/c17d6315d705aff163f0d9a22306271d/sandbox/darwin-sandbox/2023/execroot/envoy/bazel-out/darwin-fastbuild/bin/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.runfiles/envoy/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test
  Expected in: flat namespace
 in /private/var/tmp/_bazel_square/c17d6315d705aff163f0d9a22306271d/sandbox/darwin-sandbox/2023/execroot/envoy/bazel-out/darwin-fastbuild/bin/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.runfiles/envoy/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test

but passes if I add char *program_invocation_name; to test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc. I assume this is related to #10478 since I'm on a Mac locally. I'm not sure if that is the same error that CI is getting though.

htuch · 2021-02-03T01:06:31Z

Not sure, I kicked the job again, could be a flake. I don't think I can help much with Mac OS specific link issues, best to work through the existing issue.

htuch · 2021-02-03T01:07:51Z

tools/protoxform/merge_active_shadow.py

@@ -37,8 +37,23 @@ def AdjustReservedRange(target_proto, previous_reserved_range, skip_reserved_num
      target_proto.reserved_range.add().MergeFrom(rr)


+# Add dependencies for envoy.annotations.disallowed_by_default
+def AddDeprecationDependencies(target_proto_dependencies, proto_field, is_enum):


Why is this part of this PR?

This part is needed when the shadow proto brings a deprecated field that uses envoy.annotations.disallowed_by_default and the original proto does include the import for envoy/annotations/deprecation.proto.

Makes sense. What about the struct.proto below?

struct.proto was needed when the merge shadow added something like

map<string, google.protobuf.Struct> hidden_envoy_deprecated_per_filter_config = 12 [deprecated = true];

e.g. from api/envoy/config/route/v3/route_components.proto to generated_api_shadow/envoy/config/route/v3/route_components.proto and struct was not already in the imports.

cheister · 2021-02-04T01:44:41Z

Looks like on the rerun that FAIL: //test/integration:health_check_integration_test failed and //test/extensions/filters/http/grpc_json_transcoder:grpc_json_transcoder_integration_test PASSED in 84.2s, Can you run them again? Seems like maybe there are two flaky tests?

htuch

LGTM, thanks! I kicked the tests, I think this failure is unrelated.

htuch · 2021-02-04T19:10:16Z

tools/protoxform/merge_active_shadow.py

@@ -37,8 +37,23 @@ def AdjustReservedRange(target_proto, previous_reserved_range, skip_reserved_num
      target_proto.reserved_range.add().MergeFrom(rr)


+# Add dependencies for envoy.annotations.disallowed_by_default
+def AddDeprecationDependencies(target_proto_dependencies, proto_field, is_enum):


Makes sense. What about the struct.proto below?

htuch · 2021-02-04T23:51:01Z

/lgtm v2-freeze

repokitteh-read-only bot added v2-freeze api labels Oct 6, 2020

phlax mentioned this pull request Oct 6, 2020

[WIP] protos: Remove unused imports #13284

Closed

htuch suggested changes Oct 7, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 7, 2020

cheister force-pushed the fix-proto-import-warnings branch from d9f7ba2 to 7935e21 Compare October 7, 2020 00:20

repokitteh-read-only bot removed the waiting label Oct 7, 2020

cheister force-pushed the fix-proto-import-warnings branch from 7935e21 to 9d2a198 Compare October 7, 2020 04:28

mattklein123 assigned htuch Oct 7, 2020

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2020

mattklein123 removed the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2020

mattklein123 assigned mattklein123 and unassigned htuch Dec 18, 2020

mattklein123 added the waiting label Dec 18, 2020

Base automatically changed from master to main January 15, 2021 23:01

cheister force-pushed the fix-proto-import-warnings branch from 9d2a198 to b5229bb Compare January 19, 2021 06:54

repokitteh-read-only bot removed the waiting label Jan 19, 2021

cheister force-pushed the fix-proto-import-warnings branch from b5229bb to 02caf59 Compare January 19, 2021 06:56

repokitteh-read-only bot added the waiting label Jan 19, 2021

cheister force-pushed the fix-proto-import-warnings branch from 02caf59 to 9e5efed Compare January 21, 2021 07:25

repokitteh-read-only bot removed the waiting label Jan 21, 2021

mattklein123 added the waiting label Jan 30, 2021

repokitteh-read-only bot removed the waiting label Feb 1, 2021

cheister force-pushed the fix-proto-import-warnings branch 4 times, most recently from bb6f218 to 1ae5185 Compare February 1, 2021 21:46

cheister requested a review from lizan as a code owner February 1, 2021 21:46

Remove unused proto imports

18c522e

Signed-off-by: Chris Heisterkamp <[email protected]>

cheister force-pushed the fix-proto-import-warnings branch from 1ae5185 to 18c522e Compare February 1, 2021 22:24

mattklein123 assigned htuch and unassigned mattklein123 Feb 2, 2021

htuch reviewed Feb 3, 2021

View reviewed changes

htuch approved these changes Feb 4, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Feb 4, 2021

repokitteh-read-only bot removed the v2-freeze label Feb 4, 2021

htuch merged commit 209b8f9 into envoyproxy:main Feb 4, 2021

cheister deleted the fix-proto-import-warnings branch February 5, 2021 00:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unused proto import warnings in api #13401

Fix unused proto import warnings in api #13401

cheister commented Oct 6, 2020

repokitteh-read-only bot commented Oct 6, 2020

rojkov commented Oct 6, 2020

phlax commented Oct 6, 2020 •

edited

Loading

htuch left a comment

cheister commented Oct 7, 2020

cheister commented Oct 7, 2020

cheister commented Oct 7, 2020

phlax commented Oct 7, 2020

github-actions bot commented Dec 9, 2020

htuch commented Dec 9, 2020

cheister commented Jan 19, 2021

mattklein123 commented Jan 19, 2021

htuch commented Jan 20, 2021

cheister commented Feb 2, 2021

htuch commented Feb 3, 2021

htuch Feb 3, 2021

cheister Feb 4, 2021

htuch Feb 4, 2021

cheister Feb 5, 2021

cheister commented Feb 4, 2021

htuch left a comment

htuch Feb 4, 2021

htuch commented Feb 4, 2021

Fix unused proto import warnings in api #13401

Fix unused proto import warnings in api #13401

Conversation

cheister commented Oct 6, 2020

repokitteh-read-only bot commented Oct 6, 2020

rojkov commented Oct 6, 2020

phlax commented Oct 6, 2020 • edited Loading

htuch left a comment

Choose a reason for hiding this comment

cheister commented Oct 7, 2020

cheister commented Oct 7, 2020

cheister commented Oct 7, 2020

phlax commented Oct 7, 2020

github-actions bot commented Dec 9, 2020

htuch commented Dec 9, 2020

cheister commented Jan 19, 2021

mattklein123 commented Jan 19, 2021

htuch commented Jan 20, 2021

cheister commented Feb 2, 2021

htuch commented Feb 3, 2021

htuch Feb 3, 2021

Choose a reason for hiding this comment

cheister Feb 4, 2021

Choose a reason for hiding this comment

htuch Feb 4, 2021

Choose a reason for hiding this comment

cheister Feb 5, 2021

Choose a reason for hiding this comment

cheister commented Feb 4, 2021

htuch left a comment

Choose a reason for hiding this comment

htuch Feb 4, 2021

Choose a reason for hiding this comment

htuch commented Feb 4, 2021

phlax commented Oct 6, 2020 •

edited

Loading