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 third_party/protobuf to v3.14.0 (for v1.35.x) #25131

Merged
merged 11 commits into from Jan 18, 2021

Conversation

jtattermusch
Copy link
Contributor

Same as #24855, but for the already-cut v1.35.x branch (and including third_party/protobuf commit that hopefully fixes the build problem encountered in #24855)

@jtattermusch
Copy link
Contributor Author

CC @stanley-cheung

@jtattermusch jtattermusch added release notes: yes Indicates if PR needs to be in release notes release notes: no Indicates if PR should not be in release notes and removed release notes: yes Indicates if PR needs to be in release notes labels Jan 11, 2021
@jtattermusch
Copy link
Contributor Author

Lets wait for the tests to see if all problems encountered in #24855 have been fixed.

@jtattermusch
Copy link
Contributor Author

Still seeing problems:

Python artifact build on Windows
https://source.cloud.google.com/results/invocations/82b67290-2e1a-4882-b3b0-bd026e6c8814/targets/github%2Fgrpc/tests;group=tasks;test=build_artifact.python_windows_x86_Python27_32bit;row=2

third_party\protobuf\src/google/protobuf/stubs/mutex.h:119:13: error: explicitly defaulted function 'constexpr google::protobuf::internal::WrappedMutex::WrappedMutex()' cannot be declared as constexpr because the implicit declaration is not constexpr:
   constexpr WrappedMutex() = default;
             ^~~~~~~~~~~~

Python basic bazel tests
https://source.cloud.google.com/results/invocations/fa5b00dd-ab16-47bd-ae0b-328bbaaef574/log

FAIL: //src/python/grpcio_tests/tests_aio/interop:local_interop_test (see /root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/testlogs/src/python/grpcio_tests/tests_aio/interop/local_interop_test/test.log)
INFO: From Testing //src/python/grpcio_tests/tests_aio/interop:local_interop_test:
==================== 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/2698/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/2698/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'
================================================================================
[3,626 / 3,642] 71 / 138 tests, 1 failed; Testing //src/python/grpcio_tests/tests/unit/_cython:_channel_test.python2; 13s processwrapper-sandbox ... (16 actions running)
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/2873/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'
================================================================================
[3,913 / 3,915] 136 / 138 tests, 2 failed; Testing //src/python/grpcio_tests/tests/unit/_cython:_channel_test.python3; 30s processwrapper-sandbox ... (2 actions running)

@jtattermusch
Copy link
Contributor Author

@gnossen @acozzette looks like the backport protocolbuffers/protobuf#8115 doesn't work 100%:

windows python artifact build:

third_party\protobuf\src/google/protobuf/stubs/mutex.h:119:13: error: explicitly defaulted function 'constexpr google::protobuf::internal::WrappedMutex::WrappedMutex()' cannot be declared as constexpr because the implicit declaration is not constexpr:
   constexpr WrappedMutex() = default;
             ^~~~~~~~~~~~
In file included from C:/tools/msys64_win7/mingw32/include/c++/6.3.0/mutex:44:0,
                 from third_party\protobuf\src/google/protobuf/stubs/mutex.h:33,
                 from third_party\protobuf\src/google/protobuf/descriptor.h:65,
                 from third_party\protobuf\src/google/protobuf/compiler/scc.h:38,
                 from third_party\protobuf\src/google/protobuf/compiler/js/js_generator.h:41,
                 from third_party\protobuf\src\google\protobuf\compiler\js\js_generator.cc:31:
C:/tools/msys64_win7/mingw32/include/c++/6.3.0/bits/std_mutex.h:94:5: note: defaulted constructor calls non-constexpr 'std::mutex::mutex()'
     mutex() noexcept = default;
     ^~~~~

@jtattermusch
Copy link
Contributor Author

@gnossen @lidizheng how can we fix the other issue?

Python basic bazel tests
https://source.cloud.google.com/results/invocations/fa5b00dd-ab16-47bd-ae0b-328bbaaef574/log

FAIL: //src/python/grpcio_tests/tests_aio/interop:local_interop_test (see /root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/testlogs/src/python/grpcio_tests/tests_aio/interop/local_interop_test/test.log)
INFO: From Testing //src/python/grpcio_tests/tests_aio/interop:local_interop_test:
==================== 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/2698/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/2698/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'
================================================================================
[3,626 / 3,642] 71 / 138 tests, 1 failed; Testing //src/python/grpcio_tests/tests/unit/_cython:_channel_test.python2; 13s processwrapper-sandbox ... (16 actions running)
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/2873/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'
================================================================================
[3,913 / 3,915] 136 / 138 tests, 2 failed; Testing //src/python/grpcio_tests/tests/unit/_cython:_channel_test.python3; 30s processwrapper-sandbox ... (2 actions running)

@acozzette
Copy link
Contributor

I think this pull request might fix the WrappedMutex error.

@jtattermusch
Copy link
Contributor Author

@gnossen @lidizheng how can we fix the other issue?

Python basic bazel tests
https://source.cloud.google.com/results/invocations/fa5b00dd-ab16-47bd-ae0b-328bbaaef574/log

FAIL: //src/python/grpcio_tests/tests_aio/interop:local_interop_test (see /root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/testlogs/src/python/grpcio_tests/tests_aio/interop/local_interop_test/test.log)
INFO: From Testing //src/python/grpcio_tests/tests_aio/interop:local_interop_test:
==================== 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/2698/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/2698/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'
================================================================================
[3,626 / 3,642] 71 / 138 tests, 1 failed; Testing //src/python/grpcio_tests/tests/unit/_cython:_channel_test.python2; 13s processwrapper-sandbox ... (16 actions running)
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/2873/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'
================================================================================
[3,913 / 3,915] 136 / 138 tests, 2 failed; Testing //src/python/grpcio_tests/tests/unit/_cython:_channel_test.python3; 30s processwrapper-sandbox ... (2 actions running)

@gnossen @lidizheng it looks like this is the last issue that's preventing us from upgrading third_party/protobuf.
I tried to fix it by upgrading rules_python to 0.1.0 but that seems to lead to other test failures (see #25143). Can you take a look and suggest how to fix this issue (a hotfix is fine)?
Since we're trying to backport this into 1.35.x branch, we don't want to any major changes, so I'd prefer to fix this in some simple way and then perform the rules_python upgrade properly on the master branch.

@lidizheng
Copy link
Contributor

@jtattermusch Sorry, I missed the last notification. Richard and I discussed this problem, and it can be resolved by an existing workaround in our repo: https://github.com/grpc/grpc/blob/master/src/python/grpcio_tests/tests/bazel_namespace_package_hack.py.

@gnossen
Copy link
Contributor

gnossen commented Jan 14, 2021

And it looks like it was caused by this PR. I'm still investigating to determine the best way to resolve the issue.

@gnossen
Copy link
Contributor

gnossen commented Jan 14, 2021

So it turns out that the Envoy project dealt with this problem before us. The Protobuf team has intentionally removed their namespace package declarations from the __init__.py file, instead declaring them in their setup.py file, which we do not consume at all. It seems that this solution is only necessary until all other packages using the google namespace package (google-auth) do the same. I've implemented that solution in our repo here.

@jtattermusch jtattermusch mentioned this pull request Jan 15, 2021
@jtattermusch
Copy link
Contributor Author

I cherry-picked the patch from #25171. After that, all tests should be green.

@jtattermusch
Copy link
Contributor Author

@jtattermusch
Copy link
Contributor Author

@gnossen @veblush @stanley-cheung I checked all the test results and they are looking good, and this PR is now ready to merge. Please review so I can merge.

After this is merged, I'll create a PR to upport this change to the master branch.

Copy link
Contributor

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

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

LGTM. I was also following this and came to the same conclusion. Tests are looking good. Thanks for taking care of this!

@jtattermusch
Copy link
Contributor Author

jtattermusch commented Jan 18, 2021

LGTM. I was also following this and came to the same conclusion. Tests are looking good. Thanks for taking care of this!

That was quick! Happy Martin Luther King day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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