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

Added grpc@1.70.1 #3671

Merged
merged 10 commits into from
Feb 5, 2025
Merged

Added grpc@1.70.1 #3671

merged 10 commits into from
Feb 5, 2025

Conversation

veblush
Copy link
Contributor

@veblush veblush commented Jan 27, 2025

Based on 1.69.0, some changes are

@veblush
Copy link
Contributor Author

veblush commented Jan 27, 2025

@bazel-io skip_check unstable_url

@bazel-io
Copy link
Member

Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (grpc) have been updated in this PR.
Please review the changes. You can view a diff against the previous version in the "Generate module diff" check.

@bazel-io bazel-io added the skip-url-stability-check Skip the URL stability check for the PR label Jan 27, 2025
@veblush
Copy link
Contributor Author

veblush commented Jan 28, 2025

@mering and @meteorcloudy Would you take a look at this? Thanks!

@veblush veblush marked this pull request as ready for review January 28, 2025 21:27
@veblush veblush changed the title Added gRPC 1.70.0 Added gRPC@1.70.0 Jan 28, 2025
Wyverald
Wyverald previously approved these changes Jan 28, 2025
Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

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

very nice! I'd still like @mering at least to take a look, though.

@bazel-io bazel-io dismissed Wyverald’s stale review January 29, 2025 22:00

Require module maintainers' approval for newly pushed changes.

@meteorcloudy meteorcloudy added the presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval label Jan 30, 2025
@meteorcloudy
Copy link
Member

meteorcloudy commented Jan 30, 2025

@veblush Please take a look at the failures on windows, you should be able to reproduce on a windows machine with

git clone https://github.com/bazelbuild/bazel-central-registry.git
cd bazel-central-registry && git fetch origin pull/3671/head && git checkout FETCH_HEAD
bazel run //tools:setup_presubmit_repos -- --module grpc@1.70.0

then follow the instruction.

See https://github.com/bazelbuild/bazel-central-registry/tree/main/docs#reproduce-presubmit-builds-locally

(Well, there is a small bug to fix #3694)

@veblush
Copy link
Contributor Author

veblush commented Jan 30, 2025

bazel run //tools:setup_presubmit_repos -- --module grpc@1.70.0

I've got this error when running the command you mentioned above on Cloudtop.

$ bazel run //tools:setup_presubmit_repos -- --module grpc@1.70.0
Starting local Bazel server (8.0.1) and connecting to it...
INFO: Running command line: bazel-bin/tools/setup_presubmit_repos <args omitted>
2025-01-30 10:52:33,050 - INFO: Testing using registry at: /usr/local/google/home/veblush/git/a/bazel-central-registry
2025-01-30 10:52:33,053 - INFO: Creating anonymous module repo at: /usr/local/google/home/veblush/git/a/bazel-central-registry/temp_test_repos/grpc/1.70.0/anonymous_module
2025-01-30 10:52:33,054 - INFO: Anonymous module repo ready at: /usr/local/google/home/veblush/git/a/bazel-central-registry/temp_test_repos/grpc/1.70.0/anonymous_module

No task found for the host platform: linux
Please check /usr/local/google/home/veblush/git/a/bazel-central-registry/modules/grpc/1.70.0/presubmit.yml on which targets to build.

2025-01-30 10:52:33,054 - INFO: No test module task config found for grpc@1.70.0

copybara-service bot pushed a commit to grpc/grpc that referenced this pull request Jan 31, 2025
Fix missing windows symbol linker errors found when adding gRPC to BCR (From bazelbuild/bazel-central-registry#3671).

```
  C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.39.33519\bin\HostX64\x64\link.exe @bazel-out/x64_windows-fastbuild/bin/external/grpc+/third_party/address_sorting/address_sorting_6658ee4b.dll-0.params
# Configuration: 5b453b61a931a5d60d186b4b1192045ad0e097a738f75edb2ec811e836145bd1
# Execution platform: @@platforms//host:host
   Creating library bazel-out/x64_windows-fastbuild/bin/external/grpc+/third_party/address_sorting/address_sorting.if.lib and object bazel-out/x64_windows-fastbuild/bin/external/grpc+/third_party/address_sorting/address_sorting.if.exp
address_sorting.obj : error LNK2019: unresolved external symbol __imp_htonl referenced in function in6_is_addr_loopback
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_closesocket referenced in function address_sorting_create_source_addr_factory_for_current_platform
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_connect referenced in function address_sorting_create_source_addr_factory_for_current_platform
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_getsockname referenced in function address_sorting_create_source_addr_factory_for_current_platform
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_socket referenced in function address_sorting_create_source_addr_factory_for_current_platform
bazel-out\x64_windows-fastbuild\bin\external\grpc+\third_party\address_sorting\address_sorting_6658ee4b.dll : fatal error LNK1120: 5 unresolved externals
```

This issue wasn't caught by out CI, likely due to BCR presubmit's use of stricter layering_check.   Regardless, it's good as it correctly defines the `address_sorting` Bazel target's dependency.

Closes #38648

PiperOrigin-RevId: 721549737
veblush added a commit to veblush/grpc that referenced this pull request Jan 31, 2025
Fix missing windows symbol linker errors found when adding gRPC to BCR (From bazelbuild/bazel-central-registry#3671).

```
  C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.39.33519\bin\HostX64\x64\link.exe @bazel-out/x64_windows-fastbuild/bin/external/grpc+/third_party/address_sorting/address_sorting_6658ee4b.dll-0.params
# Configuration: 5b453b61a931a5d60d186b4b1192045ad0e097a738f75edb2ec811e836145bd1
# Execution platform: @@platforms//host:host
   Creating library bazel-out/x64_windows-fastbuild/bin/external/grpc+/third_party/address_sorting/address_sorting.if.lib and object bazel-out/x64_windows-fastbuild/bin/external/grpc+/third_party/address_sorting/address_sorting.if.exp
address_sorting.obj : error LNK2019: unresolved external symbol __imp_htonl referenced in function in6_is_addr_loopback
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_closesocket referenced in function address_sorting_create_source_addr_factory_for_current_platform
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_connect referenced in function address_sorting_create_source_addr_factory_for_current_platform
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_getsockname referenced in function address_sorting_create_source_addr_factory_for_current_platform
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_socket referenced in function address_sorting_create_source_addr_factory_for_current_platform
bazel-out\x64_windows-fastbuild\bin\external\grpc+\third_party\address_sorting\address_sorting_6658ee4b.dll : fatal error LNK1120: 5 unresolved externals
```

This issue wasn't caught by out CI, likely due to BCR presubmit's use of stricter layering_check.   Regardless, it's good as it correctly defines the `address_sorting` Bazel target's dependency.

Closes grpc#38648

PiperOrigin-RevId: 721549737
@meteorcloudy
Copy link
Member

@veblush Please rebase your branch to HEAD to get #3694

Also, the failure is Windows only, so it probably couldn't be reproduced on a cloudtop Linux VM

@veblush veblush changed the title Added gRPC@1.70.0 Added grpc@1.70.1 Jan 31, 2025
@veblush
Copy link
Contributor Author

veblush commented Jan 31, 2025

Please rebase your branch to HEAD to get #3694

Yep, it solved this problem and I'm now able to run the test on both Cloudtop and Windows VM.

@veblush
Copy link
Contributor Author

veblush commented Jan 31, 2025

@meteorcloudy I've resolved the initial Windows presubmit linking error (see grpc/grpc#38648), but came across a new one which is quite similar to the initial one.

   Creating library bazel-out/x64_windows-fastbuild/bin/external/abseil-cpp~/absl/base/base.if.lib and object bazel-out/x64_windows-fastbuild/bin/external/abseil-cpp~/absl/base/base.if.exp
spinlock.obj : error LNK2019: unresolved external symbol "void __cdecl absl::lts_20240722::raw_log_internal::RawLog(enum absl::lts_20240722::LogSeverity,char const *,int,char const *,...)" (?RawLog@raw_log_internal@lts_20240722@absl@@YAXW4LogSeverity@23@PEBDH1ZZ) referenced in function "void __cdecl absl::lts_20240722::base_internal::CallOnceImpl<class <lambda_78cd9549fdc6064a2f0cb01c847c3268> >(struct std::atomic<unsigned int> *,enum absl::lts_20240722::base_internal::SchedulingMode,class <lambda_78cd9549fdc6064a2f0cb01c847c3268> &&)" (??$CallOnceImpl@V<lambda_78cd9549fdc6064a2f0cb01c847c3268>@@$$V@base_internal@lts_20240722@absl@@YAXPEAU?$atomic@I@std@@W4SchedulingMode@012@$$QEAV<lambda_78cd9549fdc6064a2f0cb01c847c3268>@@@Z)
sysinfo.obj : error LNK2001: unresolved external symbol "void __cdecl absl::lts_20240722::raw_log_internal::RawLog(enum absl::lts_20240722::LogSeverity,char const *,int,char const *,...)" (?RawLog@raw_log_internal@lts_20240722@absl@@YAXW4LogSeverity@23@PEBDH1ZZ)
spinlock.obj : error LNK2019: unresolved external symbol "unsigned int __cdecl absl::lts_20240722::base_internal::SpinLockWait(struct std::atomic<unsigned int> *,int,struct absl::lts_20240722::base_internal::SpinLockWaitTransition const * const,enum absl::lts_20240722::base_internal::SchedulingMode)" (?SpinLockWait@base_internal@lts_20240722@absl@@YAIPEAU?$atomic@I@std@@HQEBUSpinLockWaitTransition@123@W4SchedulingMode@123@@Z) referenced in function "void __cdecl absl::lts_20240722::base_internal::CallOnceImpl<class <lambda_78cd9549fdc6064a2f0cb01c847c3268> >(struct std::atomic<unsigned int> *,enum absl::lts_20240722::base_internal::SchedulingMode,class <lambda_78cd9549fdc6064a2f0cb01c847c3268> &&)" (??$CallOnceImpl@V<lambda_78cd9549fdc6064a2f0cb01c847c3268>@@$$V@base_internal@lts_20240722@absl@@YAXPEAU?$atomic@I@std@@W4SchedulingMode@012@$$QEAV<lambda_78cd9549fdc6064a2f0cb01c847c3268>@@@Z)
sysinfo.obj : error LNK2001: unresolved external symbol "unsigned int __cdecl absl::lts_20240722::base_internal::SpinLockWait(struct std::atomic<unsigned int> *,int,struct absl::lts_20240722::base_internal::SpinLockWaitTransition const * const,enum absl::lts_20240722::base_internal::SchedulingMode)" (?SpinLockWait@base_internal@lts_20240722@absl@@YAIPEAU?$atomic@I@std@@HQEBUSpinLockWaitTransition@123@W4SchedulingMode@123@@Z)
spinlock.obj : error LNK2019: unresolved external symbol AbslInternalSpinLockWake_lts_20240722 referenced in function "void __cdecl absl::lts_20240722::base_internal::SpinLockWake(struct std::atomic<unsigned int> *,bool)" (?SpinLockWake@base_internal@lts_20240722@absl@@YAXPEAU?$atomic@I@std@@_N@Z)
sysinfo.obj : error LNK2001: unresolved external symbol AbslInternalSpinLockWake_lts_20240722
spinlock.obj : error LNK2019: unresolved external symbol AbslInternalSpinLockDelay_lts_20240722 referenced in function "void __cdecl absl::lts_20240722::base_internal::SpinLockDelay(struct std::atomic<unsigned int> *,unsigned int,int,enum absl::lts_20240722::base_internal::SchedulingMode)" (?SpinLockDelay@base_internal@lts_20240722@absl@@YAXPEAU?$atomic@I@std@@IHW4SchedulingMode@123@@Z)
bazel-out\x64_windows-fastbuild\bin\external\abseil-cpp~\absl\base\base_2caf69b4.dll : fatal error LNK1120: 4 unresolved externals

Because this error came from Abseil, I'm unsure whether a code fix is required, or if I can resolve it by adjusting the Bazel build or test configuration.

Note that this error occured when building this absl/base and it has a proper raw_logging_internal dependency that defines RawLog function.

@meteorcloudy
Copy link
Member

Linking external/abseil-cpp~/absl/base/base_2caf69b4.dll failed: (Exit 1120): link.exe failed: error executing CppLink command (from target @@abseil-cpp~//absl/base:base)

So, looks like this is dynamic linking error caused by @grpc//examples/protos:helloworld_py_pb2_grpc, which transitively depends on @grpc//src/python/grpcio/grpc/_cython:cygrpc which is a pyx_library that requires all deps to be built as a dynamic library.

Dynamic linking works very differently on Windows, basically all symbols are by default invisible and requires some operations to export them manually. You can check this doc for more details.

Note that this error occured when building this absl/base and it has a proper raw_logging_internal dependency that defines RawLog function.

RawLog isn't decorated with __declspec(dllexport), therefore it is not visible.

Bazel does offer a windows_export_all_symbols feature (similar to the CMake one) to automatically export all symbols from a cc_library.

You can add this features to all corresponding cc_library, but I tried that and it feels a bit like a rabbit hole and you'll need to do that in BUILD files of abseil, boringssl, etc.

Or you can add --features=windows_export_all_symbols in the command line which export all symbols, but that unfortunately will easily hit the limitation of symbol numbers to be exported.

LINK : fatal error LNK1189: library limit of 65535 objects exceeded

TensorFlow implemented a custom solution to filter out unnecessary symbols. But that requires quite a bit of effort.

I think for now, we could probably exclude @grpc//examples/protos:helloworld_py_pb2_grpc on windows and figure out a solution to export symbols on windows properly later. Maybe cc_shared_library could help, but I haven't looked into it.

/cc @pzembrod

@mering
Copy link
Contributor

mering commented Feb 3, 2025

This version fails for us with the following error:

ERROR: no such package '@@[unknown repo 'com_google_googletest' requested from @@grpc+ (did you mean 'com_google_googleapis'?)]//': The repository '@@[unknown repo 'com_google_googletest' requested from @@grpc+ (did you mean 'com_google_googleapis'?)]' could not be resolved: No repository visible as '@com_google_googletest' from repository '@@grpc+'
ERROR: HOME/.cache/bazel/_bazel_USER/311480a20df1a9e749b4616bc4db69a0/external/grpc+/third_party/BUILD:68:6: no such package '@@[unknown repo 'com_google_googletest' requested from @@grpc+ (did you mean 'com_google_googleapis'?)]//': The repository '@@[unknown repo 'com_google_googletest' requested from @@grpc+ (did you mean 'com_google_googleapis'?)]' could not be resolved: No repository visible as '@com_google_googletest' from repository '@@grpc+' and referenced by '@@grpc+//third_party:gtest'

I am not completely sure where this comes from but it might be caused by the aliases in https://github.com/grpc/grpc/blob/master/third_party/BUILD. It might make sense to get rid of the aliases completely.

FYI: Related to grpc/grpc#38626 It might also be better to declare dependencies which are not needed by external repos depending on grpc as dev_dependency=True such that bazel build --enable_bzlmod //... works in the gRPC repo.

@meteorcloudy
Copy link
Member

meteorcloudy commented Feb 3, 2025

A bunch of bazel_dep were removed, see the diff against the previous version at https://github.com/bazelbuild/bazel-central-registry/actions/runs/13076495269/job/36489869601?pr=3671

Do you actually need to depend on @@grpc+//third_party:gtest?

It might also be better to declare dependencies which are not needed by external repos depending on grpc as dev_dependency=True

Agree, some bazel_deps should probably be dev_dependency instead of being removed completely.

@mering
Copy link
Contributor

mering commented Feb 3, 2025

Do you actually need to depend on @@grpc+//third_party:gtest?

We directly depend on the following targets: //bazel:cc_grpc_library.bzl, //bazel:python_rules.bzl, //:grpc++, //:grpc++_reflection, //:grpc++_test, //:grpc_security_base, //:grpc_opencensus_plugin, //:src/python/grpcio/grpc:grpcio, //src/proto/grpc/health/v1:health_cc_proto, //src/proto/grpc/health/v1:health_cc_grpc.

From the //:grpc++_test target, we are using the following headers:

  • grpcpp/test/default_reactor_test_peer.h
  • grpcpp/test/server_context_test_spouse.h

@veblush
Copy link
Contributor Author

veblush commented Feb 4, 2025

So, looks like this is dynamic linking error caused by @grpc//examples/protos:helloworld_py_pb2_grpc, which transitively depends on @grpc//src/python/grpcio/grpc/_cython:cygrpc which is a pyx_library that requires all deps to be built as a dynamic library.

Thanks for your investigation and that makes sense. gRPC is not meant for DLL so seeing errors when building DLLs is expected. I'll disable that test on Windows.

@veblush
Copy link
Contributor Author

veblush commented Feb 4, 2025

FYI: Related to grpc/grpc#38626 It might also be better to declare dependencies which are not needed by external repos depending on grpc as dev_dependency=True such that bazel build --enable_bzlmod //... works in the gRPC

My ultimate goal is a full migration to bzlmod. However, this isn't possible until all dependencies are available in the BCR. Therefore, my current focus is on making the key public targets available through the gRPC BCR package.

copybara-service bot pushed a commit to grpc/grpc that referenced this pull request Feb 4, 2025
A few changes to address comments from bazelbuild/bazel-central-registry#3671

- Reintroduced a few more dependencies in the gRPC Bazel modules since those turned out to be required for some public gRPC targets (e.g. `googletest` is needed for `grpc++_test`)
- Removed `google_cloud_cpp` from the dependency as it's not yet registered to BCR and using it sometimes causes strange problems.
- Added a few public Bazel targets to `bazel_build_with_bzlmod_linux.sh` to make sure those are actually buildable.

Following-up of #38626

Closes #38675

PiperOrigin-RevId: 723192505
@veblush
Copy link
Contributor Author

veblush commented Feb 4, 2025

It looks like grpcpp_gcp_observability target fails to build although it's passing in our repo using bzlmod. I just removed it from the presubmit test for now but it needs to be addressed in the future. Given that monitored_resource_cc_proto is claimed to be missing, I suspect that this might be relevant to switched_rules of googleapis (code).

bazel --nosystem_rc --nohome_rc build --announce_rc --verbose_failures -- @grpc//:grpcpp_gcp_observability
INFO: Reading rc options for 'build' from /usr/local/google/home/veblush/git/bazel-central-registry/temp_test_repos/grpc/1.70.1/anonymous_module/.bazelrc:
  'build' options: --enable_bzlmod --registry=file:///usr/local/google/home/veblush/git/bazel-central-registry
ERROR: /usr/local/google/home/veblush/.cache/bazel/_bazel_veblush/f4e55e1e7c6af5aea6ec923decdef74a/external/googleapis+/google/api/BUILD.bazel: no such target '@@googleapis+//google/api:monitored_resource_cc_proto': target 'monitored_resource_cc_proto' not declared in package 'google/api' defined by /usr/local/google/home/veblush/.cache/bazel/_bazel_veblush/f4e55e1e7c6af5aea6ec923decdef74a/external/googleapis+/google/api/BUILD.bazel (did you mean monitored_resource_proto, or monitored_resource.proto?)
ERROR: /usr/local/google/home/veblush/.cache/bazel/_bazel_veblush/f4e55e1e7c6af5aea6ec923decdef74a/external/grpc+/src/cpp/ext/gcp/BUILD:31:16: no such target '@@googleapis+//google/api:monitored_resource_cc_proto': target 'monitored_resource_cc_proto' not declared in package 'google/api' defined by /usr/local/google/home/veblush/.cache/bazel/_bazel_veblush/f4e55e1e7c6af5aea6ec923decdef74a/external/googleapis+/google/api/BUILD.bazel (did you mean monitored_resource_proto, or monitored_resource.proto?) and referenced by '@@grpc+//src/cpp/ext/gcp:observability'
ERROR: Analysis of target '@@grpc+//:grpcpp_gcp_observability' failed; build aborted: Analysis failed

@veblush veblush requested a review from meteorcloudy February 4, 2025 21:36
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Nice!

@meteorcloudy meteorcloudy merged commit 4fdf78d into bazelbuild:main Feb 5, 2025
24 checks passed
paulosjca pushed a commit to paulosjca/grpc that referenced this pull request Feb 20, 2025
Fix missing windows symbol linker errors found when adding gRPC to BCR (From bazelbuild/bazel-central-registry#3671).

```
  C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.39.33519\bin\HostX64\x64\link.exe @bazel-out/x64_windows-fastbuild/bin/external/grpc+/third_party/address_sorting/address_sorting_6658ee4b.dll-0.params
# Configuration: 5b453b61a931a5d60d186b4b1192045ad0e097a738f75edb2ec811e836145bd1
# Execution platform: @@platforms//host:host
   Creating library bazel-out/x64_windows-fastbuild/bin/external/grpc+/third_party/address_sorting/address_sorting.if.lib and object bazel-out/x64_windows-fastbuild/bin/external/grpc+/third_party/address_sorting/address_sorting.if.exp
address_sorting.obj : error LNK2019: unresolved external symbol __imp_htonl referenced in function in6_is_addr_loopback
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_closesocket referenced in function address_sorting_create_source_addr_factory_for_current_platform
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_connect referenced in function address_sorting_create_source_addr_factory_for_current_platform
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_getsockname referenced in function address_sorting_create_source_addr_factory_for_current_platform
address_sorting_windows.obj : error LNK2019: unresolved external symbol __imp_socket referenced in function address_sorting_create_source_addr_factory_for_current_platform
bazel-out\x64_windows-fastbuild\bin\external\grpc+\third_party\address_sorting\address_sorting_6658ee4b.dll : fatal error LNK1120: 5 unresolved externals
```

This issue wasn't caught by out CI, likely due to BCR presubmit's use of stricter layering_check.   Regardless, it's good as it correctly defines the `address_sorting` Bazel target's dependency.

Closes grpc#38648

PiperOrigin-RevId: 721549737
paulosjca pushed a commit to paulosjca/grpc that referenced this pull request Feb 20, 2025
A few changes to address comments from bazelbuild/bazel-central-registry#3671

- Reintroduced a few more dependencies in the gRPC Bazel modules since those turned out to be required for some public gRPC targets (e.g. `googletest` is needed for `grpc++_test`)
- Removed `google_cloud_cpp` from the dependency as it's not yet registered to BCR and using it sometimes causes strange problems.
- Added a few public Bazel targets to `bazel_build_with_bzlmod_linux.sh` to make sure those are actually buildable.

Following-up of grpc#38626

Closes grpc#38675

PiperOrigin-RevId: 723192505
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval skip-url-stability-check Skip the URL stability check for the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants