Skip to content

Upgrade third_party/protobuf to v3.14.0#24855

Closed
jtattermusch wants to merge 5 commits intogrpc:masterfrom
jtattermusch:protobuf_3_14_upgrade
Closed

Upgrade third_party/protobuf to v3.14.0#24855
jtattermusch wants to merge 5 commits intogrpc:masterfrom
jtattermusch:protobuf_3_14_upgrade

Conversation

@jtattermusch
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch commented Nov 30, 2020

No description provided.

@jtattermusch jtattermusch changed the title DRAFT: Protobuf 3.14 upgrade Upgrade third_party/protobuf to v3.14.0 Dec 1, 2020
@jtattermusch jtattermusch marked this pull request as ready for review December 1, 2020 09:42
@jtattermusch jtattermusch added the release notes: no Indicates if PR should not be in release notes label Dec 1, 2020
@jtattermusch jtattermusch mentioned this pull request Dec 1, 2020
Closed
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Dec 1, 2020 •
edited
Loading

Some of the failures might have already been investigated as part of #24723.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Dec 1, 2020

Problems that likely need to be solved (the rest looks like preexisting issues)

Python27 on windows:
https://source.cloud.google.com/results/invocations/1ff03886-f487-4466-9149-67aca05767e8/targets/github%2Fgrpc/tests

Python bazel tests:
https://source.cloud.google.com/results/invocations/0fda9ba0-4fd9-4a50-9af1-41cc753f04d3/log

Ruby native gem on macos:
https://source.cloud.google.com/results/invocations/7a195775-9293-4177-a79e-370af526a00f/targets/github%2Fgrpc/tests

@gnossen @lidizheng can you take a look at the python-related issues?

@lidizheng
Copy link
Copy Markdown
Contributor

lidizheng commented Dec 1, 2020 •
edited
Loading 

C:\tools\msys64_win7\mingw64\bin\gcc.exe -mdll -O -Wall -DWIN32_LEAN_AND_MEAN=1 -DMS_WIN64=1 -I. -Igrpc_root -Igrpc_root\include -Ithird_party\protobuf\src -IC:\Python27\include -IC:\Python27\PC -c third_party\protobuf\src\google\protobuf\util\internal\json_escaping.cc -o build\temp.win-amd64-2.7\Release\third_party\protobuf\src\google\protobuf\util\internal\json_escaping.o -std=c++11 -D_ftime=_ftime64 -D_timeb=__timeb64 -D_hypot=hypot
C:\tools\msys64_win7\mingw64\bin\gcc.exe -mdll -O -Wall -DWIN32_LEAN_AND_MEAN=1 -DMS_WIN64=1 -I. -Igrpc_root -Igrpc_root\include -Ithird_party\protobuf\src -IC:\Python27\include -IC:\Python27\PC -c third_party\protobuf\src\google\protobuf\compiler\cpp\cpp_map_field.cc -o build\temp.win-amd64-2.7\Release\third_party\protobuf\src\google\protobuf\compiler\cpp\cpp_map_field.o -std=c++11 -D_ftime=_ftime64 -D_timeb=__timeb64 -D_hypot=hypot
In file included from third_party\protobuf\src\google\protobuf\util\message_differencer.cc:50:0:
third_party\protobuf\src/google/protobuf/map_field.h: In constructor 'constexpr google::protobuf::internal::MapFieldBase::MapFieldBase(google::protobuf::internal::ConstantInitialized)':
third_party\protobuf\src/google/protobuf/map_field.h:336:37: error: call to non-constexpr function 'google::protobuf::internal::WrappedMutex::WrappedMutex()'
         state_(STATE_MODIFIED_MAP) {}
                                     ^

The Python compilation error seems caused by the new ProtoBuf.

The Bazel test failure seems caused by namespace packaging disfunction. The namespace package should be a part of ProtoBuf Python package, which we installed via pip, I'm not sure why upgrade our third_party ProtoBuf will cause that. Is it failing consistantly?

@gnossen
Copy link
Copy Markdown
Contributor

gnossen commented Dec 1, 2020 •
edited
Loading

The build failure looks like a legitimate issue in protobuf that they've already fixed on master. The error indicates that the WrappedMutex constructor is not constexpr, which it isn't on 3.14.0. Some compilers are lenient on this and will add the constexpr for you if possible. Our Windows compiler appears not to be one of them.

I think our two options are:

  • Upgrade the compiler to something more lenient on constexpr marking.
  • Backport this fix to v3.14.0

P.S.: I'm secretly rooting for the backport, because that gets us one digit closer to this being the pi release.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Dec 2, 2020

FTR the python bazel build failure is:

==================== Test output for //src/python/grpcio_tests/tests_aio/interop:local_interop_test:
Traceback (most recent call last):
  File "/root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/sandbox/processwrapper-sandbox/2815/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/bin/src/python/grpcio_tests/tests_aio/interop/local_interop_test.runfiles/com_github_grpc_grpc/src/python/grpcio_tests/tests_aio/interop/local_interop_test.py", line 24, in <module>
    from tests_aio.interop import methods
  File "/root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/sandbox/processwrapper-sandbox/2815/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/bin/src/python/grpcio_tests/tests_aio/interop/local_interop_test.runfiles/com_github_grpc_grpc/src/python/grpcio_tests/tests_aio/interop/methods.py", line 29, in <module>
    from google import auth as google_auth
ImportError: cannot import name 'auth'
================================================================================
INFO: From Linking tools/distrib/python/grpcio_tools/grpc_tools/_protoc_compiler.so:
clang-12: warning: '-fuse-ld=' taking a path is deprecated. Use '--ld-path=' instead
INFO: From Linking tools/distrib/python/grpcio_tools/libprotoc_lib.so:
clang-12: warning: '-fuse-ld=' taking a path is deprecated. Use '--ld-path=' instead
FAIL: //src/python/grpcio_tests/tests_aio/status:grpc_status_test (see /root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/testlogs/src/python/grpcio_tests/tests_aio/status/grpc_status_test/test.log)
INFO: From Testing //src/python/grpcio_tests/tests_aio/status:grpc_status_test:
==================== Test output for //src/python/grpcio_tests/tests_aio/status:grpc_status_test:
Traceback (most recent call last):
  File "/root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/sandbox/processwrapper-sandbox/2955/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/bin/src/python/grpcio_tests/tests_aio/status/grpc_status_test.runfiles/com_github_grpc_grpc/src/python/grpcio_tests/tests_aio/status/grpc_status_test.py", line 22, in <module>
    from google.rpc import code_pb2, error_details_pb2, status_pb2
ModuleNotFoundError: No module named 'google.rpc'
================================================================================

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Dec 2, 2020

they've already fixed on master

Looks like that fix is coming from the internal codebase (cl/341698875)
@acozzette can we backport cl/341698875 into 3.14.x branch?

@acozzette
Copy link
Copy Markdown

acozzette commented Dec 2, 2020

@jtattermusch I just sent out protocolbuffers/protobuf#8115 to pull that fix into 3.14.x.

@acozzette acozzette mentioned this pull request Dec 2, 2020
Merged
@jtattermusch jtattermusch force-pushed the protobuf_3_14_upgrade branch from ea582b7 to bf63f1b Compare December 3, 2020 10:25
@gnossen gnossen mentioned this pull request Dec 5, 2020
Merged
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Jan 8, 2021

@stanley-cheung Ideally protobuf should be upgraded before cutting the 1.35.x branch (according to our release process).

@jtattermusch jtattermusch mentioned this pull request Jan 8, 2021
Closed
@stanley-cheung
Copy link
Copy Markdown
Contributor

stanley-cheung commented Jan 8, 2021

@stanley-cheung Ideally protobuf should be upgraded before cutting the 1.35.x branch (according to our release process).

Sorry I missed this. Should have made this a release blocker. What's the remedy now? Should we also backport this to the v1.35.x branch?

@jtattermusch jtattermusch mentioned this pull request Jan 11, 2021
Merged
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Jan 11, 2021

@stanley-cheung Ideally protobuf should be upgraded before cutting the 1.35.x branch (according to our release process).

Sorry I missed this. Should have made this a release blocker. What's the remedy now? Should we also backport this to the v1.35.x branch?

I created #25131 hopefully all the tests pass.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Jan 18, 2021

Superseded by #25183 (which brings in #25131 from v1.35.x)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gnossen gnossen Awaiting requested review from gnossen

@nicolasnoble nicolasnoble Awaiting requested review from nicolasnoble

@veblush veblush Awaiting requested review from veblush

Assignees

@gnossen gnossen

Labels

release notes: no Indicates if PR should not be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jtattermusch @lidizheng @gnossen @acozzette @stanley-cheung