Conversation
jingyichen1223
force-pushed
the
ISSUE-171
branch
from
7782d60 to
0f54db7
Compare
There was a problem hiding this comment.
Pull request overview
This PR upgrades gRPC from version 1.16.1 to 1.40.0 and refactors the abseil-cpp dependency management to address symbol conflicts. The key architectural change is moving abseil-cpp from a git submodule to a system-wide installation shared between gRPC and Gringofts, compiled with C++17 to ensure ABI compatibility and use of standard library types.
Changes:
- Upgraded gRPC to 1.40.0 and abseil-cpp to 20210324.0 LTS release
- Removed abseil-cpp git submodule and implemented system-wide installation
- Updated build scripts to compile both gRPC and abseil-cpp with C++17 standard and consistent compiler flags
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/installDependencies.sh | Added system-wide abseil-cpp installation with C++17 and SIGSTKSZ fix; updated gRPC installation to use system abseil package |
| scripts/downloadDependencies.sh | Updated gRPC version to 1.40.0 and CMake download URL to GitHub |
| scripts/addSubmodules.sh | Removed abseil-cpp submodule initialization |
| CMakeLists.txt | Changed from submodule to system abseil via find_package(); removed submodule include path |
| dockers/dependencies/install.Dockerfile | Updated Docker image tag from v3 to v4 |
| dockers/dependencies/download.Dockerfile | Updated Docker image tag from v3 to v4 |
| README.md | Updated Docker image references from v3 to v4; removed abseil-cpp from third-party license section |
| .github/workflows/main.yml | Updated CI workflow to use v4 Docker image |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sed -i 's/std::max(SIGSTKSZ, 65536)/std::max<size_t>(SIGSTKSZ, 65536)/' \ | ||
| absl/debugging/failure_signal_handler.cc && |
There was a problem hiding this comment.
The sed command to fix SIGSTKSZ assumes the file exists and doesn't check for errors. If the sed command fails (e.g., if the pattern has already been fixed in a newer version of abseil-cpp or the file structure has changed), the script will continue without detecting the failure because the entire block uses &&. Consider checking if the pattern exists before applying the sed, or make the sed more robust with error checking.
| sed -i 's/std::max(SIGSTKSZ, 65536)/std::max<size_t>(SIGSTKSZ, 65536)/' \ | |
| absl/debugging/failure_signal_handler.cc && | |
| if [ -f absl/debugging/failure_signal_handler.cc ]; then | |
| if grep -q 'std::max(SIGSTKSZ, 65536)' absl/debugging/failure_signal_handler.cc; then | |
| sed -i 's/std::max(SIGSTKSZ, 65536)/std::max<size_t>(SIGSTKSZ, 65536)/' \ | |
| absl/debugging/failure_signal_handler.cc | |
| checkLastSuccess "patch SIGSTKSZ in abseil-cpp fails" | |
| else | |
| echo "SIGSTKSZ patch not applied: pattern not found in failure_signal_handler.cc (may already be patched)." | |
| fi | |
| else | |
| echo "SIGSTKSZ patch not applied: absl/debugging/failure_signal_handler.cc not found." | |
| fi && |
| cd ~/temp/grpc && | ||
| sed -i -E "s/(gRPC_ZLIB_PROVIDER.*)module(.*CACHE)/\1package\2/" CMakeLists.txt && | ||
| sed -i -E "s/(gRPC_CARES_PROVIDER.*)module(.*CACHE)/\1package\2/" CMakeLists.txt && | ||
| sed -i -E "s/(gRPC_SSL_PROVIDER.*)module(.*CACHE)/\1package\2/" CMakeLists.txt && | ||
| sed -i -E "s/(gRPC_PROTOBUF_PROVIDER.*)module(.*CACHE)/\1package\2/" CMakeLists.txt && | ||
| echo "src/core/lib/gpr/log_linux.cc,src/core/lib/gpr/log_posix.cc,src/core/lib/iomgr/ev_epollex_linux.cc" | tr "," "\n" | xargs -L1 sed -i "s/gettid/sys_gettid/" && | ||
| CXX=g++-9 CC=gcc-9 cmake -DCMAKE_BUILD_TYPE=Debug && | ||
| make && make install && make clean && ldconfig | ||
| CXX=g++-9 CC=gcc-9 cmake -DCMAKE_BUILD_TYPE=Debug \ | ||
| -DCMAKE_CXX_STANDARD=17 \ | ||
| -DgRPC_ZLIB_PROVIDER=package \ | ||
| -DgRPC_CARES_PROVIDER=package \ | ||
| -DgRPC_SSL_PROVIDER=package \ | ||
| -DgRPC_PROTOBUF_PROVIDER=package \ | ||
| -DgRPC_ABSL_PROVIDER=package \ | ||
| . && | ||
| make -j$(nproc) && make install && make clean && ldconfig |
There was a problem hiding this comment.
The gRPC installation command has been changed from using sed to modify CMakeLists.txt to directly passing cmake flags. However, the old approach also included a sed command to fix gettid references ('echo "src/core/lib/gpr/log_linux.cc..." | ... | xargs -L1 sed -i "s/gettid/sys_gettid/"'). This fix may still be necessary for gRPC 1.40.0 depending on the glibc version being used. Verify if gRPC 1.40.0 still has gettid naming conflicts with glibc 2.34+, or if this was already fixed in the newer gRPC version.
| fi | ||
| # 3. install grpc | ||
| # 3. install grpc (using system absl with C++17) | ||
| # install libssl-dev to skip installing boringssl |
There was a problem hiding this comment.
The comment on line 67 states "install grpc (using system absl with C++17)" but there's also a comment on line 68 about "install libssl-dev to skip installing boringssl". However, the script doesn't actually install libssl-dev here. The prerequisite libssl-dev is installed earlier in downloadDependencies.sh (line 51). Consider updating or removing the outdated comment on line 68 to avoid confusion, or move the libssl-dev installation to this location if that was the intent.
| # install libssl-dev to skip installing boringssl | |
| # requires libssl-dev (installed earlier) so gRPC can use system OpenSSL instead of BoringSSL |
| grpc_version="1.16" | ||
| GRPC=$(grep ${grpc_version} /usr/local/lib/cmake/grpc/gRPCConfigVersion.cmake) | ||
| grpc_version="1.40" | ||
| GRPC=$(grep ${grpc_version} /usr/local/lib/cmake/grpc/gRPCConfigVersion.cmake 2>/dev/null) |
There was a problem hiding this comment.
The gRPC version detection has been improved with the addition of '2>/dev/null' to suppress errors when the cmake config file doesn't exist. This is a good defensive programming practice. However, the grep will match any line containing '1.40', which could lead to false positives if there are other version numbers or dates in the file. Consider using a more specific pattern like 'VERSION 1.40' or checking the exact version string to avoid potential false matches.
| GRPC=$(grep ${grpc_version} /usr/local/lib/cmake/grpc/gRPCConfigVersion.cmake 2>/dev/null) | |
| GRPC=$(grep "VERSION ${grpc_version}" /usr/local/lib/cmake/grpc/gRPCConfigVersion.cmake 2>/dev/null) |
| # 0. install absl (abseil-cpp) to system | ||
| absl_version="20210324.0" | ||
| ABSL=$(find /usr/local/lib -name 'libabsl_*.a' 2>/dev/null | head -1) | ||
| if [ -z "$ABSL" ]; then |
There was a problem hiding this comment.
The abseil-cpp installation relies on finding the abseil library from grpc's third_party subdirectory (~/temp/grpc/third_party/abseil-cpp), which assumes grpc has already been downloaded. However, the download script shows that grpc is downloaded in downloadDependencies.sh and the abseil installation happens before c-ares and protobuf installation. This creates a dependency on the grpc repository being downloaded first. Consider adding a comment or check to ensure that grpc must be downloaded before this script runs, or add an explicit check that the directory exists before attempting to cd into it.
| if [ -z "$ABSL" ]; then | |
| if [ -z "$ABSL" ]; then | |
| if [ ! -d ~/temp/grpc/third_party/abseil-cpp ]; then | |
| echo "Install Error: abseil-cpp source directory '~/temp/grpc/third_party/abseil-cpp' not found." | |
| echo "Please ensure grpc has been downloaded (e.g., by running downloadDependencies.sh) before running this script." | |
| exit 1 | |
| fi |
1 participant
Key Changes
1. Dependencies Version Upgrade
2. Abseil-cpp Dependency Refactoring
Before: abseil-cpp was managed as a git submodule of Gringofts. Gringofts and gRPC both has their own Abseil-cpp. Abseil is compiled separately for both, and are linked together at last.
After: abseil-cpp (version 20210324.0) is installed system-wide and shared across both gRPC and Gringofts. It is only compiled and installed once.
(If don't do so, 20210324.0 abseil in Gringofts and gRPC have symbol conflict)
3. Symbol Conflict Resolution
Problem: gRPC 1.40 exposes abseil symbols, which could conflict with Gringoft's abseil symbols when linking executables
Solution:
std::string_view,std::optional, etc.) instead of abseil's own typesgRPC_ABSL_PROVIDER=packageto ensure gRPC links against system-installed abseil