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

upgrade protobuf to 3.18.1 (automatically generated) #27722

Merged
merged 7 commits into from
Oct 21, 2021

Conversation

jtattermusch
Copy link
Contributor

Same as #27712, but generated automatically with the release_open_source.py script.

Let's double check the PRs are identical before merging one of them.

@jtattermusch jtattermusch marked this pull request as ready for review October 14, 2021 11:02
@jtattermusch jtattermusch added release notes: no Indicates if PR should not be in release notes lang/all wrapped languages labels Oct 14, 2021
@jtattermusch jtattermusch changed the title upgrade protobuf to 3.18.1 upgrade protobuf to 3.18.1 (automatically generated) Oct 14, 2021
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

From a quick skim, they look the same to me. I'll leave it up to you as to which one you want to merge.

@jtattermusch
Copy link
Contributor Author

jtattermusch commented Oct 15, 2021

The python bazel build failure seems as a legitimate failure:
https://source.cloud.google.com/results/invocations/af161a15-1c8c-475c-8cfb-4aea52d101c0/log ?

INFO: From Testing //src/python/grpcio_tests/tests/health_check:health_servicer_test.python2:
==================== Test output for //src/python/grpcio_tests/tests/health_check:health_servicer_test.python2:
Traceback (most recent call last):
  File "/root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/sandbox/processwrapper-sandbox/3139/execroot/com_github_grpc_grpc/bazel-out/k8-py2-fastbuild/bin/src/python/grpcio_tests/tests/health_check/health_servicer_test.python2.runfiles/com_github_grpc_grpc/src/python/grpcio_tests/tests/health_check/_health_servicer_test.py", line 22, in <module>
    from grpc_health.v1 import health
  File "/root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/sandbox/processwrapper-sandbox/3139/execroot/com_github_grpc_grpc/bazel-out/k8-py2-fastbuild/bin/src/python/grpcio_tests/tests/health_check/health_servicer_test.python2.runfiles/com_github_grpc_grpc/src/python/grpcio_health_checking/grpc_health/v1/health.py", line 21, in <module>
    from grpc_health.v1 import health_pb2 as _health_pb2
  File "/root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/sandbox/processwrapper-sandbox/3139/execroot/com_github_grpc_grpc/bazel-out/k8-py2-fastbuild/bin/src/python/grpcio_tests/tests/health_check/health_servicer_test.python2.runfiles/com_github_grpc_grpc/src/python/grpcio_health_checking/grpc_health/v1/health_pb2.py", line 5, in <module>
    from google.protobuf import descriptor as _descriptor
  File "/root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/sandbox/processwrapper-sandbox/3139/execroot/com_github_grpc_grpc/bazel-out/k8-py2-fastbuild/bin/src/python/grpcio_tests/tests/health_check/health_servicer_test.python2.runfiles/com_google_protobuf/python/google/protobuf/descriptor.py", line 113
    class DescriptorBase(metaclass=DescriptorMetaclass):
                                  ^
SyntaxError: invalid syntax
================================================================================
FAIL: //src/python/grpcio_tests/tests/csds:test_csds.python2 (see /root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/execroot/com_github_grpc_grpc/bazel-out/k8-py2-fastbuild/testlogs/src/python/grpcio_tests/tests/csds/test_csds.python2/test.log)

@jtattermusch
Copy link
Contributor Author

jtattermusch commented Oct 19, 2021

Looks like the python problem is
protocolbuffers/protobuf#9045, and the resolution was that the protobuf 3.18.0 release got yanked. Release 3.18.1 is still available though and it seems to be suffering from the same problem
(invalid syntax in class DescriptorBase(metaclass=DescriptorMetaclass))

@acozzette what is the recommended resolution here? Should we be upgrading python protobuf to 3.18.1 or not?

CC @gnossen @lidizheng

@lidizheng
Copy link
Contributor

I'm in favor of getting rid of all Python 2 tests. The only concern I have is the potential impact we might have for GAE users. I have been running TGP for the removal (cl/353911233) every couple months, last time (3 months ago) there were only one target (GAE) left in google3 that still uses Python 2.

@markdroth
Copy link
Member

Sorry for not updating the issue with current state. I spoke with @gnossen about this yesterday, and he said he'd put together a PR to remove the python2 tests.

@markdroth
Copy link
Member

I've triggered a re-run of the tests now that #27776 has been merged.

@markdroth
Copy link
Member

@dennycd Can you please take a look at the iOS test failure and help us figure out if this is related to the protobuf upgrade, and if so, advise as to how to fix it? Thanks!

@jtattermusch
Copy link
Contributor Author

jtattermusch commented Oct 21, 2021

The objc iOS failure is a timeout and seems to be happening on master as well:
(e.g. here: https://source.cloud.google.com/results/invocations/912975a9-03ed-4a59-9ead-a43d547830a8/log)

The C++ iOS failure seems similar to b/200609622, but I'm not 100% sure.

@markdroth
Copy link
Member

Yeah, I'm aware that there is a general issue going on there, but I'm a little suspicious, since this particular failure is in a protobuf-related file. Let me try rerunning that test and we'll see if it happens again.

@markdroth
Copy link
Member

The iOS C++ test is continuing to fail in exactly the same spot. Is there some way we can verify whether this is the ongoing Mac test flakiness or a real problem?

- excluding reflection_tester with gtest dependency
@dennycd
Copy link
Contributor

dennycd commented Oct 21, 2021

looks like protobuf 3.18.1 update pulled in a new gtest dependency in one of the test files (https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/reflection_tester.cc#L35), gonna test excluding this following the official spec (https://github.com/protocolbuffers/protobuf/blob/master/Protobuf-C%2B%2B.podspec#L25)

@dennycd
Copy link
Contributor

dennycd commented Oct 21, 2021

protobuf build break should now be gone. Cronet ios cpp test crash appears non-related to this PR. investigating separately

@markdroth
Copy link
Member

The remaining failures here all seem unrelated, so I'm going to go ahead and merge.

@markdroth markdroth merged commit eceffa0 into grpc:master Oct 21, 2021
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported Specifies if the PR has been imported to the internal repository lang/all wrapped languages lang/c++ priority/P0/RELEASE BLOCKER release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants