Skip to content

Commit

Permalink
GH-45566: [C++][Parquet][CMake] Remove a workaround for Windows in Fi…
Browse files Browse the repository at this point in the history
…ndThriftAlt.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?

apache/thrift#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 <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
  • Loading branch information
kou authored Feb 20, 2025
1 parent 0d6278f commit 59fd94f
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 35 deletions.
17 changes: 15 additions & 2 deletions ci/scripts/PKGBUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions ci/scripts/r_windows_build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -66,23 +66,23 @@ 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
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
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
Expand Down
43 changes: 14 additions & 29 deletions cpp/cmake_modules/FindThriftAlt.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion r/configure.win
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 59fd94f

Please sign in to comment.