Copilot commented on code in PR #55492:
URL: https://github.com/apache/arrow/pull/55492#discussion_r4540851598
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1085,7 +1085,63 @@ endif()
# ----------------------------------------------------------------------
# Add Boost dependencies (code adapted from Apache Kudu)
+# Use the bundled Boost source archive header-only, without Boost's
+# CMake build. This only provides the Boost libraries that Apache
+# Thrift needs. See ARROW_BOOST_FOR_THRIFT_ONLY.
+#
+# This is a macro to set variables in build_boost()'s PARENT_SCOPE.
+macro(build_boost_headers_for_thrift)
+ list(APPEND CMAKE_MESSAGE_INDENT "Boost: ")
+ message(STATUS "Using headers from source (only for Apache Thrift)")
+
+ fetchcontent_declare(boost
+ ${FC_DECLARE_COMMON_OPTIONS}
+ URL ${BOOST_SOURCE_URL}
+ URL_HASH "SHA256=${ARROW_BOOST_BUILD_SHA256_CHECKSUM}"
+ # This doesn't exist. So FetchContent only
+ # extracts Boost and doesn't add Boost's CMake
+ # project.
+ SOURCE_SUBDIR "arrow-header-only")
+ prepare_fetchcontent()
+ fetchcontent_makeavailable(boost)
+
+ # Boost.Uuid (used by thrift/TUuid.h) and the Boost libraries it
+ # includes headers from. If you change this, check
+ # r/tools/trim/boost.sh too.
+ set(libraries
+ assert
+ config
+ throw_exception
+ type_traits
+ uuid)
Review Comment:
This reduced Boost set omits `boost/scoped_array.hpp`, required
unconditionally by supported system Thrift 0.11.0, and
`boost/predef/other/endian.h`, required by bundled Thrift 0.25.0 when system
byte-order aliases are unavailable. Those configurations fail to compile unless
another Boost installation supplies the missing headers. Include the
protocol/transport dependencies in both the CMake include-directory list and
`r/tools/trim/boost.sh`, or restrict the reduced path to configurations it
supports.
##########
r/tools/nixlibs.R:
##########
@@ -625,30 +632,38 @@ build_libarrow <- function(src_dir, dst_dir) {
env_var_list <- with_cloud_support(env_var_list)
env_var_list <- with_wasm_support(env_var_list)
- # turn_off_all_optional_features() needs to happen after
- # with_cloud_support(), since it might turn features ON.
- thirdparty_deps_unavailable <- !download_ok &&
- !dir.exists(thirdparty_dependency_dir) &&
- !env_is("ARROW_DEPENDENCY_SOURCE", "system")
+ # turn_off_all_optional_features() and turn_off_unvendored_features() need to
+ # happen after with_cloud_support(), since it might turn features ON.
do_minimal_build <- env_is("LIBARROW_MINIMAL", "true")
+ # Thirdparty source tarballs shipped in the package
+ vendored <- usable_vendored_dependencies(src_dir)
if (do_minimal_build) {
env_var_list <- turn_off_all_optional_features(env_var_list)
} else if (thirdparty_deps_unavailable) {
cat(paste0(
"*** Building C++ library from source, but downloading thirdparty
dependencies\n",
- " is not possible, so this build will turn off all thirdparty
features.\n",
+ " is not possible, so this build will turn off the thirdparty
features\n",
+ " whose sources are not included in the package.\n",
" See installation guide for details:\n",
" https://arrow.apache.org/docs/r/articles/install.html\n";
))
- env_var_list <- turn_off_all_optional_features(env_var_list)
+ env_var_list <- turn_off_unvendored_features(env_var_list, names(vendored))
} else if (dir.exists(thirdparty_dependency_dir)) {
Review Comment:
With `LIBARROW_MINIMAL=true` and `LIBARROW_VENDORED_DEPS=false`, a supplied
`ARROW_THIRDPARTY_DEPENDENCY_DIR` is never applied: the minimal-build branch
skips `set_thirdparty_urls()`, and this call adds nothing. An offline minimal
install therefore attempts to download required uriparser sources even when
their tarball is present locally. Apply the local dependency directory
independently of the feature-selection branch.
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1790,73 +1870,153 @@ endif()
# ----------------------------------------------------------------------
# Thrift
+# Build only the parts of the Apache Thrift C++ library that Apache
+# Parquet uses (TCompactProtocol and TMemoryBuffer), without Apache
+# Thrift's CMake build. Apache Thrift's CMake build requires Boost's
+# CMake package and builds many sources that need more of Boost
+# (e.g. Boost.Locale for TJSONProtocol). See ARROW_BOOST_FOR_THRIFT_ONLY.
+#
+# This is a macro to be used in build_thrift().
+macro(add_thrift_minimal_library)
+ fetchcontent_declare(thrift
+ ${FC_DECLARE_COMMON_OPTIONS}
+ URL ${THRIFT_SOURCE_URL}
+ URL_HASH "SHA256=${ARROW_THRIFT_BUILD_SHA256_CHECKSUM}"
+ # Only extract. We don't use Apache Thrift's
+ # CMake build.
+ SOURCE_SUBDIR "arrow-no-cmake")
+ prepare_fetchcontent()
+ fetchcontent_makeavailable(thrift)
+
+ set(THRIFT_CPP_SOURCE_DIR "${thrift_SOURCE_DIR}/lib/cpp/src")
+ # If you update Apache Thrift, check whether Apache Parquet needs
+ # more sources (undefined symbols when linking libparquet users).
+ add_library(thrift STATIC
+ "${THRIFT_CPP_SOURCE_DIR}/thrift/TApplicationException.cpp"
+ "${THRIFT_CPP_SOURCE_DIR}/thrift/TOutput.cpp"
+ "${THRIFT_CPP_SOURCE_DIR}/thrift/TUuid.cpp"
+ "${THRIFT_CPP_SOURCE_DIR}/thrift/protocol/TProtocol.cpp"
+ "${THRIFT_CPP_SOURCE_DIR}/thrift/transport/TBufferTransports.cpp"
+
"${THRIFT_CPP_SOURCE_DIR}/thrift/transport/TTransportException.cpp")
+ set_target_properties(thrift PROPERTIES UNITY_BUILD OFF)
+ # Third-party code: don't fail on warnings.
+ target_compile_options(thrift PRIVATE
$<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wno-error>)
+ target_compile_definitions(thrift PUBLIC THRIFT_STATIC_DEFINE)
+ target_include_directories(thrift SYSTEM
+ PUBLIC
$<BUILD_LOCAL_INTERFACE:${thrift_BINARY_DIR}>
+
$<BUILD_LOCAL_INTERFACE:${THRIFT_CPP_SOURCE_DIR}>)
+ if(BOOST_VENDORED)
+ target_link_libraries(thrift PUBLIC
$<BUILD_LOCAL_INTERFACE:Boost::headers>)
+ else()
+ target_link_libraries(thrift PUBLIC Boost::headers)
+ endif()
+ if(TARGET Threads::Threads)
+ target_link_libraries(thrift PUBLIC Threads::Threads)
+ endif()
+
+ # Generate thrift/config.h from Apache Thrift's template, like
+ # Apache Thrift's build/cmake/ConfigureChecks.cmake does, but only
+ # with the checks that matter for the sources above and the headers
+ # they (and Apache Parquet) include.
+ include(CheckCXXSourceCompiles)
+ include(CheckIncludeFileCXX)
+ include(CheckSymbolExists)
+ check_include_file_cxx(inttypes.h ARROW_THRIFT_HAVE_INTTYPES_H)
+ check_include_file_cxx(netinet/in.h ARROW_THRIFT_HAVE_NETINET_IN_H)
+ check_include_file_cxx(stdint.h ARROW_THRIFT_HAVE_STDINT_H)
+ check_include_file_cxx(sys/param.h ARROW_THRIFT_HAVE_SYS_PARAM_H)
+ check_symbol_exists(strerror_r "string.h" ARROW_THRIFT_HAVE_STRERROR_R)
Review Comment:
This probes `strerror_r` as C, although Thrift uses it from C++. On glibc
with `CMAKE_C_EXTENSIONS=OFF`, the strict C11 probe cannot see the declaration
while the C++20 probe succeeds. That leaves `HAVE_STRERROR_R` undefined, so
Thrift's `TOutput.cpp` selects its unavailable `::strerror_s` fallback and
compilation fails. Probe the symbol with the C++ compiler instead.
##########
r/tools/nixlibs.R:
##########
@@ -874,26 +925,84 @@ set_thirdparty_urls <- function(env_var_list) {
env_var_list
}
-# libarrow requires some thirdparty dependencies, even for a minimal build.
-# The libarrow build uses them from the system if they are available and
-# otherwise builds them from source, downloading from ARROW_*_URL. To make sure
-# that works without network access, `make sync-cpp` saves their source
-# tarballs into vendored_dependency_dir and we point ARROW_*_URL at them here.
+# The libarrow build uses thirdparty dependencies from the system if they are
+# available and otherwise builds them from source, downloading from
ARROW_*_URL.
+# Some are required even for a minimal build. To make sure that works without
+# network access, `make sync-cpp` saves (trimmed) source tarballs into
+# vendored_dependency_dir and we point ARROW_*_URL at them here.
# This does not change whether the system versions are used.
-with_vendored_dependencies <- function(env_var_list) {
- if (!dir.exists(vendored_dependency_dir)) {
+# `vendored` is the result of `usable_vendored_dependencies()`.
+# With LIBARROW_VENDORED_DEPS=false, the build uses the full upstream sources
of
+# the thirdparty dependencies instead of the vendored ones, so it needs to be
+# able to download them (or find them in thirdparty_dependency_dir). Some of
+# them are needed even for a minimal build, so error with a clear message
+# rather than failing later in the libarrow build.
+check_vendored_deps_setting <- function(thirdparty_deps_unavailable) {
+ if (use_vendored_deps || !thirdparty_deps_unavailable) {
+ return(invisible(TRUE))
+ }
+ offline_build <- env_is("ARROW_OFFLINE_BUILD", "true")
+ stop(
Review Comment:
This rejects valid offline minimal builds with
`LIBARROW_VENDORED_DEPS=false` even when local upstream `ARROW_URIPARSER_URL`
and `ARROW_XSIMD_URL` archives supply the required sources. The availability
check ignores these overrides and stops before CMake can resolve them. Account
for explicitly configured local sources and installed system dependencies, or
defer dependency failures to CMake.
--
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]
