-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-16588: [C++][FlightRPC] Don't subclass GTest in test helpers #13169
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
Conversation
Also, don't link every Arrow library to UCX when enabled.
|
|
| if(ARROW_WITH_UCX) | ||
| list(APPEND ARROW_LINK_LIBS ucx::ucx) | ||
| list(APPEND ARROW_STATIC_LINK_LIBS ucx::ucx) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing it causes bundled ucx build error.
I'm not pretty sure of what this two lines does. Is it possible to include the headers somewhere else? @kou
In file included from ../src/arrow/flight/transport/ucx/flight_transport_ucx_test.cc:33:
../src/arrow/flight/transport/ucx/ucx_internal.h:27:10: fatal error: 'ucp/api/ucp.h' file not found
#include <ucp/api/ucp.h>
^~~~~~~~~~~~~~~
1 error generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you try the following patch?
diff --git a/cpp/src/arrow/flight/transport/ucx/CMakeLists.txt b/cpp/src/arrow/flight/transport/ucx/CMakeLists.txt
index 71392ec0af..d682ead633 100644
--- a/cpp/src/arrow/flight/transport/ucx/CMakeLists.txt
+++ b/cpp/src/arrow/flight/transport/ucx/CMakeLists.txt
@@ -53,6 +53,7 @@ if(ARROW_BUILD_TESTS)
arrow_flight_static
arrow_flight_testing_static
arrow_flight_transport_ucx_static
+ ucx::ucx
${ARROW_TEST_LINK_LIBS})
else()
set(ARROW_FLIGHT_UCX_TEST_LINK_LIBS
@@ -60,6 +61,7 @@ if(ARROW_BUILD_TESTS)
arrow_flight_shared
arrow_flight_testing_shared
arrow_flight_transport_ucx_shared
+ ucx::ucx
${ARROW_TEST_LINK_LIBS})
endif()
add_arrow_test(flight_transport_ucx_testThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works. Thanks @kou!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Thanks for the fix!
kou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Thanks!
… GoogleTest for arrow_flight_testing We can remove this because apache#13169/ARROW-16588 solved the link problem.
|
Benchmark runs are scheduled for baseline = 5f7a5a4 and contender = a71e5f0. a71e5f0 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
… GoogleTest for arrow_flight_testing (#13180) We can remove this because #13169/ARROW-16588 solved the link problem. Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Also, don't link every Arrow library to UCX when enabled.