Copilot commented on code in PR #50195:
URL: https://github.com/apache/arrow/pull/50195#discussion_r4034295258
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4255,15 +4261,15 @@ if(ARROW_S3)
if(ARROW_BUILD_STATIC)
if(${AWSSDK_SOURCE} STREQUAL "SYSTEM")
foreach(AWSSDK_LINK_LIBRARY ${AWSSDK_LINK_LIBRARIES})
- string(APPEND ARROW_PC_LIBS_PRIVATE "
$<TARGET_FILE:${AWSSDK_LINK_LIBRARY}>")
+ string(APPEND ARROW_S3_PC_LIBS_PRIVATE "
$<TARGET_FILE:${AWSSDK_LINK_LIBRARY}>")
endforeach()
else()
if(UNIX)
- string(APPEND ARROW_PC_REQUIRES_PRIVATE " libcurl")
+ string(APPEND ARROW_S3_PC_REQUIRES_PRIVATE " libcurl")
endif()
- string(APPEND ARROW_PC_REQUIRES_PRIVATE " openssl")
+ string(APPEND ARROW_S3_PC_REQUIRES_PRIVATE " openssl")
if(APPLE)
- string(APPEND ARROW_PC_LIBS_PRIVATE " -framework Security")
+ string(APPEND ARROW_S3_PC_LIBS_PRIVATE " -framework Security")
endif()
Review Comment:
When the AWS SDK is bundled on Windows, this branch only records OpenSSL
(and no Windows system libraries) in `arrow-s3.pc`. The generated static CMake
target adds `winhttp`, `bcrypt`, `wininet`, `userenv`, `version`, `ncrypt`,
`Secur32`, and `Shlwapi` to `Arrow::arrow_bundled_dependencies`
(cpp/src/arrow/ArrowConfig.cmake.in:145-153), but those flags are not present
in `ARROW_S3_PC_LIBS_PRIVATE`; therefore `pkg-config --static arrow-s3` can
fail to link. Please emit the same platform libraries in the arrow-s3
pkg-config metadata.
##########
r/configure.win:
##########
@@ -83,24 +83,15 @@ function configure_binaries() {
# NOTE: If you make changes to the libraries below, you should also change
# ci/scripts/r_windows_build.sh and ci/scripts/PKGBUILD
PKG_CFLAGS="-I${RWINLIB}/include -DARROW_STATIC -DARROW_COMPUTE_STATIC
-DPARQUET_STATIC \
- -DARROW_DS_STATIC -DARROW_ACERO_STATIC -DARROW_R_WITH_PARQUET \
- -DARROW_R_WITH_COMPUTE -DARROW_R_WITH_ACERO \
- -DARROW_R_WITH_DATASET -DARROW_R_WITH_JSON"
+ -DARROW_DS_STATIC -DARROW_ACERO_STATIC -DARROW_S3_STATIC \
+ -DARROW_R_WITH_PARQUET -DARROW_R_WITH_COMPUTE
-DARROW_R_WITH_ACERO \
+ -DARROW_R_WITH_DATASET -DARROW_R_WITH_JSON \
+ -DARROW_R_WITH_S3 -DARROW_R_WITH_GCS" # -DARROW_R_WITH_AZURE
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_compute
-larrow -larrow_bundled_dependencies \
+ PKG_LIBS="$PKG_LIBS -larrow_dataset -larrow_acero -lparquet -larrow_compute
-larrow_s3 -larrow -larrow_bundled_dependencies \
-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)
- "${R_HOME}/bin${R_ARCH_BIN}/Rscript.exe" -e 'R.version$major >= 4' | grep
TRUE >/dev/null 2>&1
- if [ $? -eq 0 ]; then
- PKG_CFLAGS="${PKG_CFLAGS} -DARROW_R_WITH_S3 -DARROW_R_WITH_GCS" #
-DARROW_R_WITH_AZURE
- PKG_LIBS="${PKG_LIBS} -lre2 ${AWS_LIBS} ${GCS_LIBS}" # ${AZURE_LIBS}
- else
- # It seems that order matters
- PKG_LIBS="${PKG_LIBS} -lws2_32"
- fi
+ ${MIMALLOC_LIBS} ${OPENSSL_LIBS} -lre2 ${AWS_LIBS} ${GCS_LIBS}" #
${AZURE_LIBS}
Review Comment:
These lines remove the existing Rtools40/R >= 4.0 guard and now always
enable S3/GCS and link re2, AWS, and curl libraries. The Windows binary
packages for older supported Rtools 3.5/R < 4.0 do not provide those
dependencies (the package documentation still limits Windows S3 support to
Rtools40), so `configure.win` will fail while linking those builds. Keep the
base flags unconditional and append the S3/GCS flags and libraries only in the
R >= 4.0 branch, retaining the old `-lws2_32` fallback.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
