Uh oh!
There was an error while loading. Please reload this page.
ARROW-14708: [C++] Adding missing abseil dependencies to enable static flight build - #11889
ARROW-14708: [C++] Adding missing abseil dependencies to enable static flight build#11889rok wants to merge 10 commits into
Conversation
Thanks! Unfortunately I still get a lot of linker errors using the repro from JIRA, but at least the Abseil issues are eliminated. I think the remaining issues are just because of the order of libraries. This is what CMake generates, which fails with linker errors: The errors indicate This works: i.e. link libarrow_flight, then libarrow, then libarrow_bundled_dependencies, then OpenSSL. Flipping the order of libraries in the CMakeLists.txt fixes the first issue, so I suppose this is user error, or we're missing some dependency expressed in CMake(?). The installed ArrowFlightTargets.cmake does not declare arrow_static in INTERFACE_LINK_LIBRARIES - my CMake understanding is poor but perhaps we need to do so? (It is odd because arrow_static is in the STATIC_LINK_LIBS in flight/CMakeLists.txt.) Also, ArrowTargets.cmake declares OpenSSL as part of arrow_static's INTERFACE_LINK_LIBRARIES, but ArrowConfig.cmake does not do the same for arrow_bundled_dependencies. |
lidavidm
commented
Dec 7, 2021
This diff gets CMake to repeat diff --git a/cpp/src/arrow/flight/CMakeLists.txt b/cpp/src/arrow/flight/CMakeLists.txt
index 8a3228e50..846a420f9 100644
--- a/cpp/src/arrow/flight/CMakeLists.txt+++ b/cpp/src/arrow/flight/CMakeLists.txt@@ -170,7 +170,9 @@ add_arrow_lib(arrow_flight
${ARROW_FLIGHT_LINK_LIBS}
STATIC_LINK_LIBS
arrow_static
- ${ARROW_FLIGHT_LINK_LIBS})+ ${ARROW_FLIGHT_LINK_LIBS}+ STATIC_INSTALL_INTERFACE_LIBS+ arrow_static)
foreach(LIB_TARGET ${ARROW_FLIGHT_LIBRARIES})
target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_FLIGHT_EXPORTING)And this additional diff, while questionable, fixes the rest of the link errors: diff --git a/cpp/src/arrow/ArrowConfig.cmake.in b/cpp/src/arrow/ArrowConfig.cmake.in
index 6209baeec..750d3aba6 100644
--- a/cpp/src/arrow/ArrowConfig.cmake.in+++ b/cpp/src/arrow/ArrowConfig.cmake.in@@ -87,6 +87,10 @@ if(NOT (TARGET arrow_shared OR TARGET arrow_static))
set_target_properties(
arrow_static PROPERTIES INTERFACE_LINK_LIBRARIES
"${arrow_static_interface_link_libraries};arrow_bundled_dependencies")
+ # Propagate dependencies like OpenSSL+ set_target_properties(+ arrow_bundled_dependencies PROPERTIES INTERFACE_LINK_LIBRARIES+ "${arrow_static_interface_link_libraries}")
endif()
endif()
endif() |
rok
commented
Dec 7, 2021
Thanks @lidavidm I was gonna ask for ideas about the flight linking errors! I'll include your diffs and test. |
rok
commented
Dec 8, 2021
@lidavidm I'm able to compile this locally now. I've also reduced the abseil dependencies (I think to a minimum). |
lidavidm
commented
Dec 8, 2021
@github-actions crossbow submit -g nightly |
Revision: f3745060a1982f3ee46705bec28df3830312a8b9 Submitted crossbow builds: ursacomputing/crossbow @ actions-1268
|
lidavidm
commented
Dec 8, 2021
@kou Sorry to trouble you, but would you be able to look over the CMake changes here? I'm not sure if the changes to ArrowConfig.cmake.in are strictly correct. |
There was a problem hiding this comment.
I don't know why this is needed.
Is there a use-case that uses arrow_bundled_dependencies without arrow_static?
BTW, indent for this code is broken.
There was a problem hiding this comment.
Ah, the problem here is the order of libraries to the linker. As described above, currently, the order is OpenSSL, then libarrow_bundled_dependencies, so linking will fail because libarrow_bundled_dependencies itself also depends on OpenSSL. Adding this gets CMake to repeat OpenSSL after libarrow_bundled_dependencies and then the linker will find the necessary symbols.
There was a problem hiding this comment.
I see. Then we should list OpenSSL in INTERFACE_LINK_LIBRARIES of arrow_bundled_dependencies not arrow_static and INTERFACE_LINK_LIBRARIES of arrow_static should list only arrow_bundled_dependencies something like this:
diff --git a/cpp/src/arrow/ArrowConfig.cmake.in b/cpp/src/arrow/ArrowConfig.cmake.in
index 6209baeec..ac2b9a027 100644
--- a/cpp/src/arrow/ArrowConfig.cmake.in+++ b/cpp/src/arrow/ArrowConfig.cmake.in@@ -38,6 +38,7 @@ set(ARROW_LIBRARY_PATH_SUFFIXES "@ARROW_LIBRARY_PATH_SUFFIXES@")
set(ARROW_INCLUDE_PATH_SUFFIXES "@ARROW_INCLUDE_PATH_SUFFIXES@")
set(ARROW_SYSTEM_DEPENDENCIES "@ARROW_SYSTEM_DEPENDENCIES@")
set(ARROW_BUNDLED_STATIC_LIBS "@ARROW_BUNDLED_STATIC_LIBS@")
+set(ARROW_STATIC_INSTALL_INTERFACE_LIBS "@ARROW_STATIC_INSTALL_INTERFACE_LIBS@")
include("${CMAKE_CURRENT_LIST_DIR}/ArrowOptions.cmake")
@@ -79,14 +80,9 @@ if(NOT (TARGET arrow_shared OR TARGET arrow_static))
PROPERTIES
IMPORTED_LOCATION
"${arrow_lib_dir}/${CMAKE_STATIC_LIBRARY_PREFIX}arrow_bundled_dependencies${CMAKE_STATIC_LIBRARY_SUFFIX}"
+ INTERFACE_LINK_LIBRARIES+ "${ARROW_STATIC_INSTALL_INTERFACE_LIBS}"
)
-- get_property(arrow_static_interface_link_libraries- TARGET arrow_static- PROPERTY INTERFACE_LINK_LIBRARIES)- set_target_properties(- arrow_static PROPERTIES INTERFACE_LINK_LIBRARIES- "${arrow_static_interface_link_libraries};arrow_bundled_dependencies")
endif()
endif()
endif()
diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt
index 5736c557b..66a24fbfc 100644
--- a/cpp/src/arrow/CMakeLists.txt+++ b/cpp/src/arrow/CMakeLists.txt@@ -526,6 +526,9 @@ endif()
if(ARROW_BUILD_BUNDLED_DEPENDENCIES)
string(APPEND ARROW_PC_LIBS_PRIVATE " -larrow_bundled_dependencies")
+ set(ARROW_STATIC_INSTALL_INTERFACE_LIBS_REAL "arrow_bundled_dependencies")+else()+ set(ARROW_STATIC_INSTALL_INTERFACE_LIBS_REAL ${ARROW_STATIC_INSTALL_INTERFACE_LIBS})
endif()
# Need -latomic on Raspbian.
# See also: https://issues.apache.org/jira/browse/ARROW-12860
@@ -557,7 +560,7 @@ add_arrow_lib(arrow
SHARED_INSTALL_INTERFACE_LIBS
${ARROW_SHARED_INSTALL_INTERFACE_LIBS}
STATIC_INSTALL_INTERFACE_LIBS
- ${ARROW_STATIC_INSTALL_INTERFACE_LIBS})+ ${ARROW_STATIC_INSTALL_INTERFACE_LIBS_REAL})
add_dependencies(arrow ${ARROW_LIBRARIES})
There was a problem hiding this comment.
Can we create a one-linear(?) to collect this list like https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L2659-L2667 ?
It may be better that we have a shell script that generates the list and include it from this file:
# cpp/build-support/list-grpc-absl-libraries.sh
...
echo"set(GRPC_GPR_ABSL_LIBRARIES absl::...)"$ cpp/build-support/list-grpc-absl-libraries.sh > cpp/cmake_modules/gRPCVariables.cmakediff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 66d04acae..36565fdbf 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -3475,14 +3475,7 @@ macro(build_grpc)
PROPERTIES IMPORTED_LOCATION "${GRPC_STATIC_LIBRARY_UPB}"
INTERFACE_INCLUDE_DIRECTORIES "${GRPC_INCLUDE_DIR}")
- set(GRPC_GPR_ABSL_LIBRARIES- absl::base- absl::statusor- absl::status- absl::cord- absl::strings- absl::synchronization- absl::time)+ include(gRPCVariables)
add_library(gRPC::gpr STATIC IMPORTED)
set_target_properties(gRPC::gpr
PROPERTIES IMPORTED_LOCATION "${GRPC_STATIC_LIBRARY_GPR}"a87ae67 to
7d55e3cComparerok
commented
Dec 9, 2021
Thanks for the feedback @kou and @lidavidm! I've moved things around as suggested. My cmake skills are lacking. I've also added a shell script to extract the |
1c9e45d to
9c13c18Comparerok
commented
Dec 9, 2021
I'm getting: When running: I'll continue tomorrow but would appreciate pointers :) |
lidavidm
commented
Dec 9, 2021
Just a thought, maybe the |
kou
commented
Dec 9, 2021
Ah, it seems that we also need to specify diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 38c35d7e7..b88768790 100644
--- a/cpp/cmake_modules/BuildUtils.cmake+++ b/cpp/cmake_modules/BuildUtils.cmake@@ -240,7 +240,9 @@ function(ADD_ARROW_LIB LIB_NAME)
DEPENDENCIES
SHARED_INSTALL_INTERFACE_LIBS
STATIC_INSTALL_INTERFACE_LIBS
- OUTPUT_PATH)+ OUTPUT_PATH+ SHARED_INSTALL_EXTRA_TARGETS+ STATIC_INSTALL_EXTRA_TARGETS)
cmake_parse_arguments(ARG
"${options}"
"${one_value_args}"
@@ -396,7 +398,7 @@ function(ADD_ARROW_LIB LIB_NAME)
"${_lib_install_name}")
endif()
- install(TARGETS ${LIB_NAME}_shared ${INSTALL_IS_OPTIONAL}+ install(TARGETS ${LIB_NAME}_shared ${ARG_SHARED_INSTALL_EXTRA_TARGETS} ${INSTALL_IS_OPTIONAL}
EXPORT ${LIB_NAME}_targets
RUNTIME DESTINATION ${RUNTIME_INSTALL_DIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
@@ -451,7 +453,7 @@ function(ADD_ARROW_LIB LIB_NAME)
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
endif()
- install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL}+ install(TARGETS ${LIB_NAME}_static ${ARG_STATIC_INSTALL_EXTRA_TARGETS} ${INSTALL_IS_OPTIONAL}
EXPORT ${LIB_NAME}_targets
RUNTIME DESTINATION ${RUNTIME_INSTALL_DIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
@@ -464,7 +466,7 @@ function(ADD_ARROW_LIB LIB_NAME)
arrow_install_cmake_find_module("${ARG_CMAKE_PACKAGE_NAME}")
set(TARGETS_CMAKE "${ARG_CMAKE_PACKAGE_NAME}Targets.cmake")
- install(EXPORT ${LIB_NAME}_targets+ install(EXPORT ${LIB_NAME}_targets ${ARG_CMAKE_
FILE "${TARGETS_CMAKE}"
DESTINATION "${ARROW_CMAKE_INSTALL_DIR}")
diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt
index 5736c557b..07520d919 100644
--- a/cpp/src/arrow/CMakeLists.txt+++ b/cpp/src/arrow/CMakeLists.txt@@ -557,7 +557,9 @@ add_arrow_lib(arrow
SHARED_INSTALL_INTERFACE_LIBS
${ARROW_SHARED_INSTALL_INTERFACE_LIBS}
STATIC_INSTALL_INTERFACE_LIBS
- ${ARROW_STATIC_INSTALL_INTERFACE_LIBS})+ ${ARROW_STATIC_INSTALL_INTERFACE_LIBS}+ STATIC_EXTRA_TARGETS+ arrow_bundled_dependencies)
add_dependencies(arrow ${ARROW_LIBRARIES})
Or we can add Do you want me to take this up? |
rok
commented
Dec 9, 2021
@kou thanks for the pointers! -Wall -fno-semantic-interposition -msse4.2 CMake Error at cmake_modules/BuildUtils.cmake:469 (install):
install EXPORT given unknown argument "ArrowTargets.cmake".
Call Stack (most recent call first):
src/arrow/CMakeLists.txt:539 (add_arrow_lib)
-- Creating bundled static library target arrow_bundled_dependencies at /home/rok/Documents/repos/arrow/cpp/build/release/libarrow_bundled_dependencies.a
CMake Error at cmake_modules/BuildUtils.cmake:469 (install):
install EXPORT given unknown argument "ArrowTestingTargets.cmake".
Call Stack (most recent call first):
src/arrow/CMakeLists.txt:601 (add_arrow_lib)
CMake Error at cmake_modules/BuildUtils.cmake:469 (install):
install EXPORT given unknown argument "ArrowDatasetTargets.cmake".
Call Stack (most recent call first):
src/arrow/dataset/CMakeLists.txt:51 (add_arrow_lib)
-- Found Python3: /home/rok/miniconda3/envs/pyarrow-dev/bin/python3.7 (found version "3.7.12") found components: Interpreter Development NumPy Development.Module Development.Embed CMake Error at cmake_modules/BuildUtils.cmake:469 (install):
install EXPORT given unknown argument "ArrowPythonTargets.cmake".
Call Stack (most recent call first):
src/arrow/python/CMakeLists.txt:70 (add_arrow_lib)
CMake Error at cmake_modules/BuildUtils.cmake:469 (install):
install EXPORT given unknown argument "ParquetTargets.cmake".
Call Stack (most recent call first):
src/parquet/CMakeLists.txt:240 (add_arrow_lib) |
rok
commented
Jan 21, 2022
@github-actions crossbow submit -g nightly |
Revision: aed4157ff68fcde5f16a472cd689640dc624f588 Submitted crossbow builds: ursacomputing/crossbow @ actions-1422
|
cc1b7a1 to
85c9208CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
f632d02 to
89bd9b4Comparerok
commented
Mar 7, 2022
@lidavidm Implemented your suggestions and added a workaround for the abseil issue on macOS. |
There was a problem hiding this comment.
I'm still confused here, now the arguments were added back but we don't use them?
There was a problem hiding this comment.
This won't work on non-Apple platforms will it?
There was a problem hiding this comment.
Do we need this branch at all? Presumably absl::time's deps are correct on non-Apple platforms and we don't need to overwrite them?
There was a problem hiding this comment.
I'm not sure about absl::time's deps on non-Apple so leaving that branch as is.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Shouldn't this end with -Wl,--no-force-load or something?
Related, does it take -force-load or --force-load?
There was a problem hiding this comment.
So all_load/noall_load currently won't work due to symbol duplication so force-load is the only option and there appears to be no noforce-load option.
More here.
There was a problem hiding this comment.
Is this back to NOT? I feel like it flip-flops on every commit…was there a rebase error or something? It might be easier to squash current commits if needed
There was a problem hiding this comment.
Ugh, sorry. I was switching back and forth between linux and mac to fix the mac issue and probably caused this. I'll squash later today.
27e3cd0 to
f525cd6Comparerok
commented
Mar 7, 2022
@lidavidm thanks for your patience. I've pushed another change with your suggestions. |
| STATIC_INSTALL_INTERFACE_LIBS | ||
| OUTPUT_PATH) |
There was a problem hiding this comment.
Hmm, did you mean to remove this parameter?
| ) | ||
| INTERFACE_LINK_LIBRARIES | ||
| ${ARROW_STATIC_INSTALL_INTERFACE_LIBS} |
lidavidm
left a comment
There was a problem hiding this comment.
Thanks! Things work for me now.
rok
commented
Mar 7, 2022
Sorry for the rebasing mess @lidavidm ! :) |
| endif() | ||
| if(ARROW_BUILD_BUNDLED_DEPENDENCIES) | ||
| string(APPEND ARROW_PC_LIBS_PRIVATE " -larrow_bundled_dependencies") |
| ${ARROW_SHARED_INSTALL_INTERFACE_LIBS} | ||
| STATIC_INSTALL_INTERFACE_LIBS | ||
| ${ARROW_STATIC_INSTALL_INTERFACE_LIBS}) | ||
| STATIC_INSTALL_INTERFACE_LIBS) |
There was a problem hiding this comment.
Can we remove STATIC_INSTALL_INTERFACE_LIBS?
| if(ARROW_BUILD_BUNDLED_DEPENDENCIES) | ||
| string(APPEND ARROW_PC_LIBS_PRIVATE " -larrow_bundled_dependencies") | ||
| set(ARROW_STATIC_INSTALL_INTERFACE_LIBS_REAL ${ARROW_STATIC_INSTALL_INTERFACE_LIBS}) |
There was a problem hiding this comment.
Why do we need _REAL alias?
Is there any problem if we use ARROW_STATIC_INSTALL_INTERFACE_LIBS directly?
There was a problem hiding this comment.
Removing _REAL. It works for me locally, waiting or tests.
rok
commented
Mar 8, 2022
@kou thank you for the review! I've pushed suggested changes. |
Benchmark runs are scheduled for baseline = 5772d65 and contender = 2fea818. 2fea818 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
No description provided.