[CELEBORN-2266] Modernize Protobuf CMake usage and add install rules#3602
[CELEBORN-2266] Modernize Protobuf CMake usage and add install rules#3602jaystarshot wants to merge 2 commits intoapache:mainfrom
Conversation
|
Thanks for the PR, I will take a look soon @jaystarshot |
| #include <google/protobuf/io/coded_stream.h> | ||
| #include <charconv> | ||
| #include <chrono> | ||
| #include <set> |
There was a problem hiding this comment.
Good catch on the transitive dependency 👍
|
@afterincomparableyum Thanks for the quick review! Just curious are you using the C++ client in production, or is it still primarily a WIP? The README mentions it's WIP. I'm starting to explore it for a POC use case. |
|
@jaystarshot , @HolyLow and I are still working on C++ client. I have a few more PRs to open/wrap up once my current ones get merged. |
|
@jaystarshot, thanks for great contribution. Could you create new Jira ticket for this which refers to #1053? I have run the CI for you. |
|
Will do are is the CI flaky? The cpp changes shouldn't affect those i think |
|
@jaystarshot, the failure of CI is not related with the changes of this pull request. You just create jira ticket for this pull request. |
jaystarshot
changed the title
3 participants
What changes were proposed in this pull request?
Switched Protobuf CMake integration from the legacy FindProtobuf module to modern CONFIG mode with imported targets (protobuf::protoc, protobuf::libprotobuf).
Added install() rules for public headers, generated proto headers, and static libraries so the C++ client can be consumed as an installed package.
Added missing #include in CelebornUtils.h.
Why are the changes needed?
The legacy FindProtobuf module-variable style (${PROTOBUF_LIBRARY}, bare protoc command) is fragile and does not work reliably with package managers like vcpkg or Conan that provide Protobuf via CMake config files. Switching to CONFIG mode and imported targets is the modern CMake best practice and ensures the correct protoc binary and library are used regardless of the build environment.
The install rules are needed so that downstream projects can consume the Celeborn C++ client from a clean install prefix rather than pointing directly at the source and build trees.
The missing include was causing compilation failures in certain toolchain configurations where the header was not transitively included.
Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
How was this patch tested?