Copilot commented on code in PR #50195:
URL: https://github.com/apache/arrow/pull/50195#discussion_r3956661228
##########
python/CMakeLists.txt:
##########
@@ -744,6 +744,14 @@ if(PYARROW_BUILD_S3)
if(NOT ARROW_S3)
message(FATAL_ERROR "You must build Arrow C++ with ARROW_S3=ON")
endif()
+ find_package(ArrowS3 REQUIRED)
+ if(PYARROW_BUNDLE_ARROW_CPP)
+ bundle_arrow_lib(${ARROW_S3_SHARED_LIB} SO_VERSION ${ARROW_SO_VERSION})
+ if(MSVC)
+ bundle_arrow_import_lib(${ARROW_S3_IMPORT_LIB})
+ endif()
+ endif()
+ set(S3_LINK_LIBS ArrowS3::arrow_s3_shared)
list(APPEND CYTHON_EXTENSIONS _s3fs)
Review Comment:
PYARROW_BUILD_S3 currently hard-codes linking to ArrowS3::arrow_s3_shared.
This will fail for static-only Arrow builds (ARROW_BUILD_SHARED=OFF) where only
ArrowS3::arrow_s3_static is available. Consider selecting the ArrowS3 target
based on ARROW_BUILD_SHARED (similar to Dataset/Substrait/Parquet blocks).
##########
r/configure.win:
##########
@@ -83,12 +83,13 @@ 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_DS_STATIC -DARROW_ACERO_STATIC -DARROW_R_WITH_PARQUET
-DARROW_S3_STATIC \
-DARROW_R_WITH_COMPUTE -DARROW_R_WITH_ACERO \
- -DARROW_R_WITH_DATASET -DARROW_R_WITH_JSON"
+ -DARROW_R_WITH_DATASET -DARROW_R_WITH_JSON \
+ -DARROW_R_WITH_S3"
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}"
Review Comment:
On Windows this now always defines ARROW_R_WITH_S3/ARROW_S3_STATIC and links
-larrow_s3, but the AWS/GCS link flags are still only added for R >= 4.0. For R
< 4 this will likely fail to link (static arrow_s3 needs AWS libs) and also
contradicts the nearby comment that S3 support is only for Rtools40. Consider
gating the S3 flags/library on the same R >= 4.0 check (or removing the check
entirely if older R is no longer supported).
##########
cpp/cmake_modules/DefineOptions.cmake:
##########
@@ -396,17 +396,11 @@ takes precedence over ccache if a storage backend is
configured" ON)
ARROW_JSON)
define_option(ARROW_S3
- "Build Arrow with S3 support (requires the AWS SDK for C++)"
+ "Build Arrow S3 Module (requires the AWS SDK for C++)"
OFF
DEPENDS
ARROW_FILESYSTEM)
Review Comment:
The ARROW_S3 option no longer builds a "module" (ARROW_S3_MODULE was removed
and ArrowS3 is now built as a separate library), so this option description is
misleading. Updating it will avoid confusion for users configuring builds.
--
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]
