This is an automated email from the ASF dual-hosted git repository.
kou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 59fd94f195 GH-45566: [C++][Parquet][CMake] Remove a workaround for
Windows in FindThriftAlt.cmake (#45567)
59fd94f195 is described below
commit 59fd94f19591f1db1c72a840f5859e14521271fb
Author: Sutou Kouhei <[email protected]>
AuthorDate: Fri Feb 21 05:30:12 2025 +0900
GH-45566: [C++][Parquet][CMake] Remove a workaround for Windows in
FindThriftAlt.cmake (#45567)
### Rationale for this change
In general, we want to remove workarounds as much as possible for
maintainability.
### What changes are included in this PR?
https://github.com/apache/thrift/pull/2725 isn't released yet but MSYS2 has
another workaround:
https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-thrift/002-fix-pkgconfig-paths.patch
R uses RTools packages but the Apache Thrift package on RTools packages
doesn't have the fix. So we use bundled Apache Thrift instead.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* GitHub Issue: #45566
Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
---
ci/scripts/PKGBUILD | 17 ++++++++++++--
ci/scripts/r_windows_build.sh | 6 ++---
cpp/cmake_modules/FindThriftAlt.cmake | 43 ++++++++++++-----------------------
r/configure.win | 2 +-
4 files changed, 33 insertions(+), 35 deletions(-)
diff --git a/ci/scripts/PKGBUILD b/ci/scripts/PKGBUILD
index bcff5bb73e..9eac3ef5cb 100644
--- a/ci/scripts/PKGBUILD
+++ b/ci/scripts/PKGBUILD
@@ -28,7 +28,6 @@ depends=("${MINGW_PACKAGE_PREFIX}-bzip2"
"${MINGW_PACKAGE_PREFIX}-curl" # for google-cloud-cpp bundled build
"${MINGW_PACKAGE_PREFIX}-libutf8proc"
"${MINGW_PACKAGE_PREFIX}-re2"
- "${MINGW_PACKAGE_PREFIX}-thrift"
"${MINGW_PACKAGE_PREFIX}-snappy"
"${MINGW_PACKAGE_PREFIX}-zlib"
"${MINGW_PACKAGE_PREFIX}-lz4"
@@ -85,6 +84,19 @@ build() {
# and does not work with newer versions of CMake. See comments:
# https://github.com/apache/arrow/pull/44989/files#r1901428998
+ # We use the bundled Apache Thrift instead of the MINGW one because
+ # the upstream one on rtools packages is unmaintained. Apache Thrift
+ # still have the following problem:
+ #
+ # https://github.com/apache/thrift/pull/2725
+ #
+ # The original MSYS2 package has another fix:
+ #
+ #
https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-thrift/002-fix-pkgconfig-paths.patch
+ #
+ # But one on rtools packages doesn't have the fix. So we can't use
+ # the MINGW one.
+
# MSYS2_ARG_CONV_EXCL is needed to prevent autoconverting
CMAKE_INSTALL_PREFIX
# to Windows paths. See
https://www.msys2.org/docs/filesystem-paths/#process-arguments
@@ -128,7 +140,8 @@ build() {
-DCMAKE_BUILD_TYPE="release" \
-DCMAKE_INSTALL_PREFIX=${MINGW_PREFIX} \
-DCMAKE_UNITY_BUILD=OFF \
- -DCMAKE_VERBOSE_MAKEFILE=ON
+ -DCMAKE_VERBOSE_MAKEFILE=ON \
+ -DThrift_SOURCE=BUNDLED
make -j3
popd
diff --git a/ci/scripts/r_windows_build.sh b/ci/scripts/r_windows_build.sh
index af29858bfe..de92addf08 100755
--- a/ci/scripts/r_windows_build.sh
+++ b/ci/scripts/r_windows_build.sh
@@ -66,7 +66,7 @@ if [ -d mingw64/lib/ ]; then
# Move the 64-bit versions of libarrow into the expected location
mv mingw64/lib/*.a $DST_DIR/lib/x64
# These are from https://dl.bintray.com/rtools/mingw{32,64}/
- cp
$MSYS_LIB_DIR/mingw64/lib/lib{thrift,snappy,zstd,lz4,brotli*,bz2,crypto,curl,ss*,utf8proc,re2,nghttp2}.a
$DST_DIR/lib/x64
+ cp
$MSYS_LIB_DIR/mingw64/lib/lib{snappy,zstd,lz4,brotli*,bz2,crypto,curl,ss*,utf8proc,re2,nghttp2}.a
$DST_DIR/lib/x64
fi
# Same for the 32-bit versions
@@ -74,7 +74,7 @@ if [ -d mingw32/lib/ ]; then
ls $MSYS_LIB_DIR/mingw32/lib/
mkdir -p $DST_DIR/lib/i386
mv mingw32/lib/*.a $DST_DIR/lib/i386
- cp
$MSYS_LIB_DIR/mingw32/lib/lib{thrift,snappy,zstd,lz4,brotli*,bz2,crypto,curl,ss*,utf8proc,re2,nghttp2}.a
$DST_DIR/lib/i386
+ cp
$MSYS_LIB_DIR/mingw32/lib/lib{snappy,zstd,lz4,brotli*,bz2,crypto,curl,ss*,utf8proc,re2,nghttp2}.a
$DST_DIR/lib/i386
fi
# Do the same also for ucrt64
@@ -82,7 +82,7 @@ if [ -d ucrt64/lib/ ]; then
ls $MSYS_LIB_DIR/ucrt64/lib/
mkdir -p $DST_DIR/lib/x64-ucrt
mv ucrt64/lib/*.a $DST_DIR/lib/x64-ucrt
- cp
$MSYS_LIB_DIR/ucrt64/lib/lib{thrift,snappy,zstd,lz4,brotli*,bz2,crypto,curl,ss*,utf8proc,re2,nghttp2}.a
$DST_DIR/lib/x64-ucrt
+ cp
$MSYS_LIB_DIR/ucrt64/lib/lib{snappy,zstd,lz4,brotli*,bz2,crypto,curl,ss*,utf8proc,re2,nghttp2}.a
$DST_DIR/lib/x64-ucrt
fi
# Create build artifact
diff --git a/cpp/cmake_modules/FindThriftAlt.cmake
b/cpp/cmake_modules/FindThriftAlt.cmake
index 98a706deb9..0c5aed8e4e 100644
--- a/cpp/cmake_modules/FindThriftAlt.cmake
+++ b/cpp/cmake_modules/FindThriftAlt.cmake
@@ -32,35 +32,20 @@ if(ThriftAlt_FOUND)
return()
endif()
-# There are some problems in ThriftConfig.cmake provided by MSYS2 and
-# conda on Windows:
-#
-# * https://github.com/conda-forge/thrift-cpp-feedstock/issues/68
-# *
https://github.com/msys2/MINGW-packages/issues/6619#issuecomment-649728718
-#
-# We can remove the following "if(NOT WIN32)" condition once the
-# followings are fixed and a new version that includes these fixes is
-# published by MSYS2 and conda:
-#
-# * https://github.com/apache/thrift/pull/2725
-# * https://github.com/apache/thrift/pull/2726
-# * https://github.com/conda-forge/thrift-cpp-feedstock/issues/68
-if(NOT WIN32)
- set(find_package_args "")
- if(ThriftAlt_FIND_VERSION)
- list(APPEND find_package_args ${ThriftAlt_FIND_VERSION})
- endif()
- if(ThriftAlt_FIND_QUIETLY)
- list(APPEND find_package_args QUIET)
- endif()
- find_package(Thrift ${find_package_args})
- if(Thrift_FOUND)
- set(ThriftAlt_FOUND TRUE)
- add_executable(thrift::compiler IMPORTED)
- set_target_properties(thrift::compiler PROPERTIES IMPORTED_LOCATION
- "${THRIFT_COMPILER}")
- return()
- endif()
+set(find_package_args "")
+if(ThriftAlt_FIND_VERSION)
+ list(APPEND find_package_args ${ThriftAlt_FIND_VERSION})
+endif()
+if(ThriftAlt_FIND_QUIETLY)
+ list(APPEND find_package_args QUIET)
+endif()
+find_package(Thrift ${find_package_args})
+if(Thrift_FOUND)
+ set(ThriftAlt_FOUND TRUE)
+ add_executable(thrift::compiler IMPORTED)
+ set_target_properties(thrift::compiler PROPERTIES IMPORTED_LOCATION
+ "${THRIFT_COMPILER}")
+ return()
endif()
function(extract_thrift_version)
diff --git a/r/configure.win b/r/configure.win
index a8f8422478..e0682917e9 100755
--- a/r/configure.win
+++ b/r/configure.win
@@ -87,7 +87,7 @@ function configure_binaries() {
PKG_LIBS="-L${RWINLIB}/lib"'$(subst gcc,,$(COMPILED_BY))$(R_ARCH) '
PKG_LIBS="$PKG_LIBS -L${RWINLIB}/lib"'$(R_ARCH)$(CRT) '
PKG_LIBS="$PKG_LIBS -larrow_dataset -larrow_acero -lparquet -larrow
-larrow_bundled_dependencies \
- -lutf8proc -lthrift -lsnappy -lz -lzstd -llz4 -lbz2 ${BROTLI_LIBS}
-lole32 \
+ -lutf8proc -lsnappy -lz -lzstd -llz4 -lbz2 ${BROTLI_LIBS} -lole32 \
${MIMALLOC_LIBS} ${OPENSSL_LIBS}"
# S3, GCS, and re2 support only for Rtools40 (i.e. R >= 4.0)
