ARROW-17494: [C++] Fix substrait tests linkage on static builds #13939
Conversation
|
@github-actions crossbow submit test-ubuntu-*-cpp-static |
|
Revision: 571780b Submitted crossbow builds: ursacomputing/crossbow @ actions-fc0800e62e
|
cpp/src/arrow/engine/CMakeLists.txt
Outdated
| @@ -58,7 +58,11 @@ foreach(LIB_TARGET ${ARROW_SUBSTRAIT_LIBRARIES}) | |||
| target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_ENGINE_EXPORTING) | |||
| endforeach() | |||
|
|
|||
| set(ARROW_SUBSTRAIT_TEST_LINK_LIBS ${ARROW_SUBSTRAIT_LINK_lIBS} ${ARROW_TEST_LINK_LIBS}) | |||
| if(NOT ARROW_SUBSTRAIT_LINK_LIBS) | |||
| set(ARROW_SUBSTRAIT_LINK_LIBS substrait) | |||
There was a problem hiding this comment.
even-though this fixes the issue I am quite confused on why list(APPEND ARROW_SUBSTRAIT_TEST_LINK_LIBS arrow_substrait_static) is not containing substrait as a dependency to be linked statically as I would have expected. I've been able to reproduce also on UBUNTU=22.04.
@lidavidm what do you think? Any idea on a different approach I could take on this one?
There was a problem hiding this comment.
I think there's probably a missing dependency in ThirdpartyToolchain between the generated header and the command that generates it
There was a problem hiding this comment.
This is enough to reproduce it for me
cmake ../cpp -DCMAKE_BUILD_TYPE=Debug -GNinja -DARROW_BUILD_TESTS=ON -DARROW_BUILD_SHARED=OFF -DARROW_BUILD_STATIC=ON -DARROW_SUBSTRAIT=ON
ninja src/arrow/engine/CMakeFiles/arrow-substrait-substrait-test.dir/substrait/function_test.cc.o
There was a problem hiding this comment.
Actually, I don't really get it either. Maybe CMake doesn't export the dependency transitively…? I don't understand CMake well enough to get what's going on here
There was a problem hiding this comment.
Maybe it has to go under STATIC_INSTALL_INTERFACE_LIBS instead?
There was a problem hiding this comment.
Could you try the following without your changes?
diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 888ca19af5..2b65fcc5f0 100644
--- a/cpp/cmake_modules/BuildUtils.cmake
+++ b/cpp/cmake_modules/BuildUtils.cmake
@@ -399,7 +399,7 @@ function(ADD_ARROW_LIB LIB_NAME)
endif()
if(ARG_STATIC_LINK_LIBS)
- target_link_libraries(${LIB_NAME}_static LINK_PRIVATE
+ target_link_libraries(${LIB_NAME}_static PUBLIC
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
if(USE_OBJLIB)
# Ensure that dependencies are built before compilation of objects inThere was a problem hiding this comment.
Thanks @kou this change also solves the issue.
…m LINK_PRIVATE to PUBLIC
|
@github-actions crossbow submit test-ubuntu-*-cpp-static |
|
Revision: 8205c03 Submitted crossbow builds: ursacomputing/crossbow @ actions-b344a34f6c
|
|
@github-actions crossbow submit test-ubuntu-*-cpp-static |
|
Revision: a27a9b5 Submitted crossbow builds: ursacomputing/crossbow @ actions-a4566a376b
|
|
Benchmark runs are scheduled for baseline = 78d586a and contender = 9b23a70. 9b23a70 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
…he#13939) Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…he#13939) Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
No description provided.