GH-49991: [C++][FlightRPC] Fix unity build ordering issue#49990
Conversation
Signed-off-by: Tom spot Callaway <spotaws@amazon.com>
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
|
|
lidavidm
left a comment
There was a problem hiding this comment.
Thanks for tracking this down - there's just a formatter error it seems
| #if defined(GRPC_CPP_VERSION_MAJOR) && defined(GRPC_CPP_VERSION_MINOR) && \ | ||
| defined(GRPC_CPP_VERSION_PATCH) |
There was a problem hiding this comment.
Why do we need these checks?
It seems that grpcpp/version_info.h always provides them.
There was a problem hiding this comment.
I was being cautious and trying to future proof, but maybe it's overkill.
There was a problem hiding this comment.
If they are not needed, could you remove them to simplify our implementation?
There was a problem hiding this comment.
Pull request overview
Fixes a CMake UNITY_BUILD ordering problem in the Flight gRPC transport where util_internal.h could be processed before gRPC version macros were available, causing FromAbslStatus to not be declared (and later calls to fail to compile).
Changes:
- Include
<grpcpp/version_info.h>inutil_internal.hsoGRPC_CPP_VERSION_*macros are available regardless of include order. - Move/adjust
GRPC_CPP_VERSION_CHECKmacro definition so it is usable before the namespace block (with a fallback when version macros aren’t defined). - Conditionally include
<absl/status/status.h>when building against gRPC >= 1.80.0 so::absl::Statusis known for the conditional declaration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rationale for this change
Original error message:
In testing builds against latest grpc and protobuf, I found a Unity build (CMake UNITY_BUILD) ordering issue. The file
server_tracing_middleware.ccis the first file in the Unity translation unit and includesutil_internal.hwithout first including<grpcpp/grpcpp.h>. Sinceutil_internal.huses#pragmaonce, it's only processed once — at that pointGRPC_CPP_VERSION_MAJORis undefined, so the#if GRPC_CPP_VERSION_CHECK(1, 80, 0)guard evaluates to false and theFromAbslStatusdeclaration is skipped. Whengrpc_client.cclater tries to callFromAbslStatus, the declaration was never emitted.What changes are included in this PR?
I added
#include <grpcpp/version_info.h>at the top so the version macros are always available, moved theGRPC_CPP_VERSION_CHECKmacro definition before the namespace block (with a safety check for the macros being defined), and added#include <absl/status/status.h> conditionally when gRPC >= 1.80.0Are these changes tested?
Yes, I ran the full test suite with this change applied (and the build now succeeds).
100% tests passed, 0 tests failed out of 97
Label Time Summary:
arrow-compute-tests = 7.32 secproc (14 tests)
arrow-tests = 24.99 secproc (40 tests)
arrow_acero = 26.32 secproc (14 tests)
arrow_dataset = 27.48 secproc (14 tests)
arrow_flight = 2.00 secproc (2 tests)
filesystem = 0.86 secproc (2 tests)
parquet-tests = 11.88 secproc (13 tests)
unittest = 99.98 secproc (97 tests)
Total Test time (real) = 15.10 sec
Are there any user-facing changes?
No.