Fix Thrift dependency when using system Arrow (facebookincubator#10355)

Summary: With this facebookincubator@0d80228 commit included, velox will resolve dependency by firstly finding lib arrow from system. In this path, linking with lib thrift is lacking, which causes thrift header not found issue. With this pr, velox will try to find thrift lib. If it is not found, velox will build arrow from source with thrift bundled. If found, lib arrow will be linked with it. This pr also lets setup scripts install thrift bundled in arrow. Pull Request resolved: facebookincubator#10355 Reviewed By: bikramSingh91 Differential Revision: D59595645 Pulled By: pedroerp fbshipit-source-id: 9f26eee8347a6c650e70163554fa9b2b72f5bfac
PHILO-HE · Jul 13, 2024 · faf3a3a · faf3a3a
1 parent f34c9b1
commit faf3a3a
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 38 deletions.
diff --git a/.github/workflows/linux-build.yml b/.github/workflows/linux-build.yml
@@ -62,7 +62,7 @@ jobs:
       Protobuf_SOURCE: BUNDLED # can be removed after #10134 is merged
       simdjson_SOURCE: BUNDLED
       xsimd_SOURCE: BUNDLED
-      Arrow_SOURCE: BUNDLED
+      Arrow_SOURCE: AUTO
       CUDA_VERSION: "12.4"
     steps:
       - uses: actions/checkout@v4

diff --git a/CMake/FindArrow.cmake b/CMake/FindArrow.cmake
@@ -16,11 +16,18 @@ find_library(ARROW_LIB libarrow.a)
 find_library(PARQUET_LIB libparquet.a)
 find_library(ARROW_TESTING_LIB libarrow_testing.a)
 if("${ARROW_LIB}" STREQUAL "ARROW_LIB-NOTFOUND"
-   # OR "${PARQUET_LIB}" STREQUAL "PARQUET_LIB-NOTFOUND"
    OR "${ARROW_TESTING_LIB}" STREQUAL "ARROW_TESTING_LIB-NOTFOUND")
   set(Arrow_FOUND false)
   return()
 endif()
+find_package(Thrift)
+if(NOT Thrift_FOUND)
+  # Requires building arrow from source with thrift bundled.
+  set(Arrow_FOUND false)
+  return()
+endif()
+add_library(thrift ALIAS thrift::thrift)
+
 set(Arrow_FOUND true)
 
 add_library(arrow STATIC IMPORTED GLOBAL)
@@ -31,7 +38,8 @@ find_path(ARROW_INCLUDE_PATH arrow/api.h)
 set_target_properties(
   arrow arrow_testing parquet PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
                                          ${ARROW_INCLUDE_PATH})
-set_target_properties(arrow PROPERTIES IMPORTED_LOCATION ${ARROW_LIB})
+set_target_properties(arrow PROPERTIES IMPORTED_LOCATION ${ARROW_LIB}
+                                       INTERFACE_LINK_LIBRARIES thrift)
 set_target_properties(parquet PROPERTIES IMPORTED_LOCATION ${PARQUET_LIB})
 set_target_properties(arrow_testing PROPERTIES IMPORTED_LOCATION
                                                ${ARROW_TESTING_LIB})
diff --git a/scripts/setup-centos9.sh b/scripts/setup-centos9.sh
@@ -191,23 +191,29 @@ ARROW_VERSION=15.0.0
 
 function install_arrow {
   wget_and_untar https://archive.apache.org/dist/arrow/arrow-${ARROW_VERSION}/apache-arrow-${ARROW_VERSION}.tar.gz arrow
-  cd arrow/cpp
-  cmake_install \
-    -DARROW_PARQUET=OFF \
-    -DARROW_WITH_THRIFT=ON \
-    -DARROW_WITH_LZ4=ON \
-    -DARROW_WITH_SNAPPY=ON \
-    -DARROW_WITH_ZLIB=ON \
-    -DARROW_WITH_ZSTD=ON \
-    -DARROW_JEMALLOC=OFF \
-    -DARROW_SIMD_LEVEL=NONE \
-    -DARROW_RUNTIME_SIMD_LEVEL=NONE \
-    -DARROW_WITH_UTF8PROC=OFF \
-    -DARROW_TESTING=ON \
-    -DCMAKE_INSTALL_PREFIX=/usr/local \
-    -DCMAKE_BUILD_TYPE=Release \
-    -DARROW_BUILD_STATIC=ON \
-    -DThrift_SOURCE=BUNDLED
+  (
+    cd arrow/cpp
+    cmake_install \
+      -DARROW_PARQUET=OFF \
+      -DARROW_WITH_THRIFT=ON \
+      -DARROW_WITH_LZ4=ON \
+      -DARROW_WITH_SNAPPY=ON \
+      -DARROW_WITH_ZLIB=ON \
+      -DARROW_WITH_ZSTD=ON \
+      -DARROW_JEMALLOC=OFF \
+      -DARROW_SIMD_LEVEL=NONE \
+      -DARROW_RUNTIME_SIMD_LEVEL=NONE \
+      -DARROW_WITH_UTF8PROC=OFF \
+      -DARROW_TESTING=ON \
+      -DCMAKE_INSTALL_PREFIX=/usr/local \
+      -DCMAKE_BUILD_TYPE=Release \
+      -DARROW_BUILD_STATIC=ON \
+      -DThrift_SOURCE=BUNDLED
+
+    # Install thrift.
+    cd _build/thrift_ep-prefix/src/thrift_ep-build
+    cmake --install ./ --prefix /usr/local/
+  )
 }
 
 function install_cuda {

diff --git a/scripts/setup-ubuntu.sh b/scripts/setup-ubuntu.sh
@@ -83,7 +83,6 @@ function install_velox_deps_from_apt {
     libre2-dev \
     libsnappy-dev \
     libsodium-dev \
-    libthrift-dev \
     liblzo2-dev \
     libelf-dev \
     libdwarf-dev \
@@ -161,23 +160,29 @@ ARROW_VERSION=15.0.0
 
 function install_arrow {
   wget_and_untar https://archive.apache.org/dist/arrow/arrow-${ARROW_VERSION}/apache-arrow-${ARROW_VERSION}.tar.gz arrow
-  cd arrow/cpp
-  cmake_install \
-    -DARROW_PARQUET=OFF \
-    -DARROW_WITH_THRIFT=ON \
-    -DARROW_WITH_LZ4=ON \
-    -DARROW_WITH_SNAPPY=ON \
-    -DARROW_WITH_ZLIB=ON \
-    -DARROW_WITH_ZSTD=ON \
-    -DARROW_JEMALLOC=OFF \
-    -DARROW_SIMD_LEVEL=NONE \
-    -DARROW_RUNTIME_SIMD_LEVEL=NONE \
-    -DARROW_WITH_UTF8PROC=OFF \
-    -DARROW_TESTING=ON \
-    -DCMAKE_INSTALL_PREFIX=/usr/local \
-    -DCMAKE_BUILD_TYPE=Release \
-    -DARROW_BUILD_STATIC=ON \
-    -DThrift_SOURCE=BUNDLED
+  (
+    cd arrow/cpp
+    cmake_install \
+      -DARROW_PARQUET=OFF \
+      -DARROW_WITH_THRIFT=ON \
+      -DARROW_WITH_LZ4=ON \
+      -DARROW_WITH_SNAPPY=ON \
+      -DARROW_WITH_ZLIB=ON \
+      -DARROW_WITH_ZSTD=ON \
+      -DARROW_JEMALLOC=OFF \
+      -DARROW_SIMD_LEVEL=NONE \
+      -DARROW_RUNTIME_SIMD_LEVEL=NONE \
+      -DARROW_WITH_UTF8PROC=OFF \
+      -DARROW_TESTING=ON \
+      -DCMAKE_INSTALL_PREFIX=/usr/local \
+      -DCMAKE_BUILD_TYPE=Release \
+      -DARROW_BUILD_STATIC=ON \
+      -DThrift_SOURCE=BUNDLED
+
+    # Install thrift.
+    cd _build/thrift_ep-prefix/src/thrift_ep-build
+    $SUDO cmake --install ./ --prefix /usr/local/
+  )
 }
 
 function install_cuda {