chore: merge prepare-for-v3.0.0 branch into main#15962
Conversation
Co-authored-by: Mend Renovate <bot@renovateapp.com>
Co-authored-by: Mend Renovate <bot@renovateapp.com>
chore: update to protobuf v30 and remove cxx14 build (#15013) * chore: update to protobuf v30 and remove cxx14 build * update grpc to v1.71.0 * use new abseil repo name * add repo_mapping for absl * disable bzlmod for bazel-targets build * combine zypper commands chore: update abseil to 20250127.1 (#15039) chore(ci): update api and abi for v3 (#15041) chore(bazel): get bazel 8 working (#15042) * chore(bazel): get bazel 8 working * enable workspace where we disable bzlmod * patch googleapis BUILD.bazel when using workspace * check for null json * determine generation proto paths dynamically * bazel quickstart version has to be changed in two phases * update quickstart bazelrc to use c++17
* chore(bazel): get bazel 8 working * enable workspace where we disable bzlmod * patch googleapis BUILD.bazel when using workspace * check for null json * determine generation proto paths dynamically * bazel quickstart version has to be changed in two phases * update quickstart bazelrc to use c++17
Co-authored-by: Mend Renovate <bot@renovateapp.com>
Co-authored-by: Mend Renovate <bot@renovateapp.com>
chore: update to protobuf v30 and remove cxx14 build (#15013) * chore: update to protobuf v30 and remove cxx14 build * update grpc to v1.71.0 * use new abseil repo name * add repo_mapping for absl * disable bzlmod for bazel-targets build * combine zypper commands chore: update abseil to 20250127.1 (#15039) chore(ci): update api and abi for v3 (#15041) chore(bazel): get bazel 8 working (#15042) * chore(bazel): get bazel 8 working * enable workspace where we disable bzlmod * patch googleapis BUILD.bazel when using workspace * check for null json * determine generation proto paths dynamically * bazel quickstart version has to be changed in two phases * update quickstart bazelrc to use c++17
* chore(bazel): get bazel 8 working * enable workspace where we disable bzlmod * patch googleapis BUILD.bazel when using workspace * check for null json * determine generation proto paths dynamically * bazel quickstart version has to be changed in two phases * update quickstart bazelrc to use c++17
…ge::internal::AccessControlCommon` (#15138) * remove CommonMetadata and AccessControlCommon * minor fixes * minor fixes --------- Co-authored-by: Denis DelGrosso <85250797+ddelgrosso1@users.noreply.github.com>
* Use proto_library from protobuf * Use cc_proto_library and cc_grpc_library from protobuf and grpc repos
* feat: get googleapis from BCR * feat: add repo_name to googleapis dependency * feat: remove switched_rules for com_google_googleapis_imports * feat: remove all switched_rules * feat: add explicit dependencies for googleapis cc bindings * fix: update googleapis version to 0.0.0-20250703-f9d6fe4a in BCR
* fix: explicitly register macOS toolchain * fix: formatting * fix: register the Xcode toolchain for macOS builds * chore: trigger CI * fix: enable apple cpp toolchain for macOS builds * fix: rely on rules_apple auto-detection for macOS toolchain.
* update abi dumps
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @scotthart, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request merges the Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request merges the prepare-for-v3.0.0 branch, which includes a significant number of changes to update the project for its v3 release. Key changes include upgrading the minimum C++ standard to C++17, updating numerous dependencies to their newer versions, and transitioning to Bazel modules for dependency management. My review focused on ensuring the consistency and correctness of these widespread changes, particularly within the build and CI configurations. I've identified several issues related to dependency management in the Dockerfiles for demo environments, a critical configuration error in the Bazel WORKSPACE setup, and a potential issue with suppressed warnings in one of the build scripts. Addressing these points will help ensure a smooth transition to v3 for users and maintain the project's quality.
| patches = [ | ||
|
|
||
| # NOTE: This should only be used while developing with a new | ||
| # protobuf message. No changes to `patches` should ever be | ||
| # committed to the main branch. | ||
| #"googleapis.patch", | ||
|
|
||
| # Mirrors the patch from the current bazel module | ||
| "//bazel:remove_upb_c_rules.patch", | ||
| ], | ||
| patch_tool = "patch", |
There was a problem hiding this comment.
The patches attribute is used here, but patch_strip is not set. The patch file //bazel:remove_upb_c_rules.patch is a git-style diff and requires patch -p1 to apply correctly. Please add patch_strip = 1 to this http_archive rule to ensure the patch applies correctly for WORKSPACE-based builds.
| patches = [ | |
| # NOTE: This should only be used while developing with a new | |
| # protobuf message. No changes to `patches` should ever be | |
| # committed to the main branch. | |
| #"googleapis.patch", | |
| # Mirrors the patch from the current bazel module | |
| "//bazel:remove_upb_c_rules.patch", | |
| ], | |
| patch_tool = "patch", | |
| patches = [ | |
| # NOTE: This should only be used while developing with a new | |
| # protobuf message. No changes to `patches` should ever be | |
| # committed to the main branch. | |
| #"googleapis.patch", | |
| # Mirrors the patch from the current bazel module | |
| "//bazel:remove_upb_c_rules.patch", | |
| ], | |
| patch_strip = 1, | |
| patch_tool = "patch", |
| apk add abseil-cpp-dev c-ares-dev curl-dev grpc-dev \ | ||
| protobuf-dev nlohmann-json openssl-dev re2-dev |
There was a problem hiding this comment.
Alpine 3.20 provides Abseil 20240116.2, but this project now depends on a much newer version (20250814.1). This version mismatch might cause build failures for the demo. Consider building Abseil from source in this Dockerfile to ensure version compatibility.
| WORKDIR /var/tmp/build/abseil-cpp | ||
| RUN curl -fsSL https://github.com/abseil/abseil-cpp/archive/20250814.1.tar.gz | \ | ||
| tar -xzf - --strip-components=1 && \ | ||
| cmake \ | ||
| -DCMAKE_BUILD_TYPE=Release \ | ||
| -DBUILD_SHARED_LIBS=yes \ | ||
| -DCRC32C_BUILD_TESTS=OFF \ | ||
| -DCRC32C_BUILD_BENCHMARKS=OFF \ | ||
| -DCRC32C_USE_GLOG=OFF \ | ||
| -S . -B cmake-out && \ | ||
| -DCMAKE_BUILD_TYPE=Release \ | ||
| -DCMAKE_CXX_STANDARD=17 \ | ||
| -DABSL_BUILD_TESTING=OFF \ | ||
| -DBUILD_SHARED_LIBS=yes \ | ||
| -S . -B cmake-out && \ | ||
| cmake --build cmake-out -- -j ${NCPU:-4} && \ | ||
| cmake --build cmake-out --target install -- -j ${NCPU:-4} && \ | ||
| ldconfig |
There was a problem hiding this comment.
This Dockerfile appears to install libabsl-dev via apt-get earlier in the file, and then builds a different version of Abseil from source here. This can lead to version conflicts and unexpected behavior. To ensure the correct version is used, please remove libabsl-dev from the apt-get install command.
| dnf install -y protobuf-compiler protobuf-devel grpc-cpp grpc-devel \ | ||
| json-devel libcurl-devel google-crc32c-devel openssl-devel | ||
| json-devel libcurl-devel openssl-devel |
There was a problem hiding this comment.
Fedora 40 provides Abseil 20230802.1, but this project now depends on a much newer version (20250814.1). This version mismatch might cause build failures for the demo. Consider building Abseil from source in this Dockerfile, similar to how it's done in fedora-latest-cmake.Dockerfile, to ensure version compatibility.
| zypper install --allow-downgrade -y abseil-cpp-devel c-ares-devel \ | ||
| libcurl-devel libopenssl-devel libcrc32c-devel nlohmann_json-devel \ | ||
| libcurl-devel libopenssl-devel nlohmann_json-devel \ | ||
| grpc-devel libprotobuf-devel |
There was a problem hiding this comment.
openSUSE Leap 15 provides a very old version of Abseil (20211102.0), while this project now depends on a much newer version (20250814.1). This will likely cause build failures. Please build Abseil from source in this Dockerfile to ensure version compatibility.
| WORKDIR /var/tmp/build/abseil-cpp | ||
| RUN curl -fsSL https://github.com/abseil/abseil-cpp/archive/20250814.1.tar.gz | \ | ||
| tar -xzf - --strip-components=1 && \ | ||
| cmake \ | ||
| -DCMAKE_BUILD_TYPE=Release \ | ||
| -DBUILD_SHARED_LIBS=yes \ | ||
| -DCRC32C_BUILD_TESTS=OFF \ | ||
| -DCRC32C_BUILD_BENCHMARKS=OFF \ | ||
| -DCRC32C_USE_GLOG=OFF \ | ||
| -S . -B cmake-out && \ | ||
| -DCMAKE_BUILD_TYPE=Release \ | ||
| -DABSL_BUILD_TESTING=OFF \ | ||
| -DBUILD_SHARED_LIBS=yes \ | ||
| -S . -B cmake-out && \ | ||
| cmake --build cmake-out -- -j ${NCPU:-4} && \ | ||
| cmake --build cmake-out --target install -- -j ${NCPU:-4} && \ | ||
| ldconfig |
There was a problem hiding this comment.
This Dockerfile appears to install libabsl-dev via apt-get earlier in the file, and then builds a different version of Abseil from source here. This can lead to version conflicts and unexpected behavior. To ensure the correct version is used, please remove libabsl-dev from the apt-get install command.
|
|
||
| io::run cmake "${cmake_args[@]}" \ | ||
| -DCMAKE_CXX_STANDARD=17 \ | ||
| -DCMAKE_CXX_FLAGS="-Wno-error=conversion" \ |
There was a problem hiding this comment.
Could you add a comment explaining why -Wno-error=conversion is needed? Suppressing this warning might hide potential issues, so it would be good to document the reason for it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15962 +/- ##
==========================================
- Coverage 92.95% 92.64% -0.32%
==========================================
Files 2460 2334 -126
Lines 228901 214633 -14268
==========================================
- Hits 212785 198843 -13942
+ Misses 16116 15790 -326 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7 participants
This PR moves v3 development from a feature branch to main.
The following failing builds will be disabled/removed/not required:
quickstart-cmake-pris expected to fail until vcpkg is successfully updatedclang-7-0is no longer supported; a new clang-14-0 will be added to replace itcxx14is obsolete as C++17 is the new minimumdemo-ubuntu-focalis obsolete as focal is EoLcmake-oldest-depsused focal and will be updated to jammyotel-disabled-bazelis obsolete as OpenTelemetry is now a required dependencykokorobuilds will require additional work post merge to get workingSuggested changes from Gemini will be applied post merge in order to keep this PR a pure "merge".
This change is