kou commented on code in PR #46912:
URL: https://github.com/apache/arrow/pull/46912#discussion_r2188895627
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1747,102 +1729,74 @@ endif()
# ----------------------------------------------------------------------
# Thrift
-macro(build_thrift)
- message(STATUS "Building Apache Thrift from source")
- set(THRIFT_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/thrift_ep-install")
- set(THRIFT_INCLUDE_DIR "${THRIFT_PREFIX}/include")
- set(THRIFT_CMAKE_ARGS
- ${EP_COMMON_CMAKE_ARGS}
- "-DCMAKE_INSTALL_PREFIX=${THRIFT_PREFIX}"
- "-DCMAKE_INSTALL_RPATH=${THRIFT_PREFIX}/lib"
- # Work around https://gitlab.kitware.com/cmake/cmake/issues/18865
- -DBoost_NO_BOOST_CMAKE=ON
- -DBUILD_COMPILER=OFF
- -DBUILD_EXAMPLES=OFF
- -DBUILD_TUTORIALS=OFF
- -DCMAKE_DEBUG_POSTFIX=
- -DWITH_AS3=OFF
- -DWITH_CPP=ON
- -DWITH_C_GLIB=OFF
- -DWITH_JAVA=OFF
- -DWITH_JAVASCRIPT=OFF
- -DWITH_LIBEVENT=OFF
- -DWITH_NODEJS=OFF
- -DWITH_PYTHON=OFF
- -DWITH_QT5=OFF
- -DWITH_ZLIB=OFF)
-
- # Thrift also uses boost. Forward important boost settings if there were
ones passed.
- if(DEFINED BOOST_ROOT)
- list(APPEND THRIFT_CMAKE_ARGS "-DBOOST_ROOT=${BOOST_ROOT}")
- endif()
- list(APPEND
- THRIFT_CMAKE_ARGS
-
"-DBoost_INCLUDE_DIR=$<TARGET_PROPERTY:Boost::headers,INTERFACE_INCLUDE_DIRECTORIES>"
- )
- if(DEFINED Boost_NAMESPACE)
- list(APPEND THRIFT_CMAKE_ARGS "-DBoost_NAMESPACE=${Boost_NAMESPACE}")
+function(build_thrift)
+ list(APPEND CMAKE_MESSAGE_INDENT "Thrift: ")
+ message(STATUS "Building from source")
+
+ if(CMAKE_VERSION VERSION_LESS 3.26)
+ message(FATAL_ERROR "Require CMake 3.26 or later for building bundled
Apache Thrift")
endif()
+ fetchcontent_declare(thrift
+ ${FC_DECLARE_COMMON_OPTIONS}
+ URL ${THRIFT_SOURCE_URL}
+ URL_HASH "SHA256=${ARROW_THRIFT_BUILD_SHA256_CHECKSUM}")
+ prepare_fetchcontent()
+ set(BUILD_COMPILER OFF)
+ set(BUILD_EXAMPLES OFF)
+ set(BUILD_TUTORIALS OFF)
+ set(CMAKE_UNITY_BUILD OFF)
+ set(WITH_AS3 OFF)
+ set(WITH_CPP ON)
+ set(WITH_C_GLIB OFF)
+ set(WITH_JAVA OFF)
+ set(WITH_JAVASCRIPT OFF)
+ set(WITH_LIBEVENT OFF)
if(MSVC)
if(ARROW_USE_STATIC_CRT)
- set(THRIFT_LIB_SUFFIX "mt")
- list(APPEND THRIFT_CMAKE_ARGS "-DWITH_MT=ON")
+ set(WITH_MT ON)
else()
- set(THRIFT_LIB_SUFFIX "md")
- list(APPEND THRIFT_CMAKE_ARGS "-DWITH_MT=OFF")
+ set(WITH_MT OFF)
endif()
- # NOTE(amoeba): When you bump Thrift to >=0.21.0, change bin to lib
- set(THRIFT_LIB
-
"${THRIFT_PREFIX}/bin/${CMAKE_IMPORT_LIBRARY_PREFIX}thrift${THRIFT_LIB_SUFFIX}${CMAKE_IMPORT_LIBRARY_SUFFIX}"
- )
- else()
- set(THRIFT_LIB
-
"${THRIFT_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}thrift${CMAKE_STATIC_LIBRARY_SUFFIX}"
- )
endif()
+ set(WITH_NODEJS OFF)
+ set(WITH_PYTHON OFF)
+ set(WITH_QT5 OFF)
+ set(WITH_ZLIB OFF)
- if(BOOST_VENDORED)
- set(THRIFT_DEPENDENCIES ${THRIFT_DEPENDENCIES} boost_ep)
- endif()
+ # Remove Apache Arrow's CMAKE_MODULE_PATH to ensure using Apache
+ # Thrift's cmake_modules/.
+ list(POP_FRONT CMAKE_MODULE_PATH)
+ fetchcontent_makeavailable(thrift)
- set(THRIFT_PATCH_COMMAND)
- if(CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION
VERSION_GREATER_EQUAL 15.0)
- # Thrift 0.21.0 doesn't support GCC 15.
- # https://github.com/apache/arrow/issues/45096
- # https://github.com/apache/thrift/pull/3078
- find_program(PATCH patch REQUIRED)
- list(APPEND
- THRIFT_PATCH_COMMAND
- ${PATCH}
- -p1
- -i
- ${CMAKE_CURRENT_LIST_DIR}/thrift-cstdint.patch)
+ if(CMAKE_VERSION VERSION_LESS 3.28)
+ set_property(DIRECTORY ${thrift_SOURCE_DIR} PROPERTY EXCLUDE_FROM_ALL TRUE)
endif()
- externalproject_add(thrift_ep
- ${EP_COMMON_OPTIONS}
- URL ${THRIFT_SOURCE_URL}
- URL_HASH "SHA256=${ARROW_THRIFT_BUILD_SHA256_CHECKSUM}"
- BUILD_BYPRODUCTS "${THRIFT_LIB}"
- CMAKE_ARGS ${THRIFT_CMAKE_ARGS}
- DEPENDS ${THRIFT_DEPENDENCIES}
- PATCH_COMMAND ${THRIFT_PATCH_COMMAND})
-
- add_library(thrift::thrift STATIC IMPORTED)
- # The include directory must exist before it is referenced by a target.
- file(MAKE_DIRECTORY "${THRIFT_INCLUDE_DIR}")
- set_target_properties(thrift::thrift PROPERTIES IMPORTED_LOCATION
"${THRIFT_LIB}")
- target_include_directories(thrift::thrift BEFORE INTERFACE
"${THRIFT_INCLUDE_DIR}")
- if(ARROW_USE_BOOST)
- target_link_libraries(thrift::thrift INTERFACE Boost::headers)
+ target_include_directories(thrift
+ INTERFACE
$<BUILD_LOCAL_INTERFACE:${thrift_BINARY_DIR}>
Review Comment:
`BUILD_LOCAL_INTERFACE` requires CMake 3.26.
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2697,9 +2651,8 @@ function(build_lz4)
# Add to bundled static libs.
# We must use lz4_static (not imported target) not LZ4::lz4 (imported
target).
- list(APPEND ARROW_BUNDLED_STATIC_LIBS lz4_static)
set(ARROW_BUNDLED_STATIC_LIBS
- ${ARROW_BUNDLED_STATIC_LIBS}
+ ${ARROW_BUNDLED_STATIC_LIBS} lz4_static
Review Comment:
Sorry. These simplication in
`build_lz4()`/`build_awssdk()`/`build_azure_sdk()` aren't related to this PR. I
just noticed them.
##########
cpp/src/arrow/flight/sql/CMakeLists.txt:
##########
@@ -117,9 +117,9 @@ if(ARROW_BUILD_TESTS OR ARROW_BUILD_EXAMPLES)
set(ARROW_FLIGHT_SQL_TEST_SRCS server_test.cc
server_session_middleware_internals_test.cc)
- set(ARROW_FLIGHT_SQL_TEST_LIBS ${SQLite3_LIBRARIES})
+ set(ARROW_FLIGHT_SQL_TEST_LIBS ${SQLite3_LIBRARIES} Boost::headers)
set(ARROW_FLIGHT_SQL_ACERO_SRCS example/acero_server.cc)
- set(ARROW_FLIGHT_SQL_EXTRA_LINK_LIBS "")
+ set(ARROW_FLIGHT_SQL_TEST_EXTRA_LINK_LIBS "")
Review Comment:
Sorry. This rename isn't related to this PR. I just noticed this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
