baibaichen commented on code in PR #12625:
URL: https://github.com/apache/gluten/pull/12625#discussion_r3672927256
##########
cpp/velox/CMakeLists.txt:
##########
@@ -443,8 +466,8 @@ if(DEFINED VCPKG_INSTALLED_DIR
)
target_link_libraries(
velox
- PRIVATE ${VCPKG_INSTALLED_DIR}/${VCPKG_TRIPLET_DIR}/lib/libthriftcpp2.a
-
${VCPKG_INSTALLED_DIR}/${VCPKG_TRIPLET_DIR}/lib/libthriftprotocol.a)
+ PUBLIC ${VCPKG_INSTALLED_DIR}/${VCPKG_TRIPLET_DIR}/lib/libthriftcpp2.a
+ ${VCPKG_INSTALLED_DIR}/${VCPKG_TRIPLET_DIR}/lib/libthriftprotocol.a)
Review Comment:
Fixed in 45e11c44e. The raw vcpkg Thrift archives are now PRIVATE
implementation dependencies of the shared `velox` target. A full Debug vcpkg
package build completed successfully with tests, benchmarks, and the Spark 4.1
Maven reactor.
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -153,11 +153,7 @@ function apply_provided_velox_patch {
}
function apply_compilation_fixes {
- local SUDO_CMD=""
- if [ "$OS" == "Linux" ]; then
- SUDO_CMD="sudo"
- fi
- $SUDO_CMD cp ${CURRENT_DIR}/modify_arrow.patch
${VELOX_HOME}/CMake/resolve_dependency_modules/arrow/
+ cp ${CURRENT_DIR}/modify_arrow.patch
${VELOX_HOME}/CMake/resolve_dependency_modules/arrow/
Review Comment:
The original `sudo cp` was introduced in #3916 while staging patches for EP
caching; that PR contains no permission requirement. #12129 later noted that
`VELOX_HOME` is normally user-owned and removed sudo on macOS while
conservatively retaining Linux behavior. Further tracing showed the copy itself
became obsolete when Velox patch wiring was removed in commit 0a89a6ac2. Commit
aa6d8e51d therefore removes the dead staging function and invocation entirely.
##########
dev/vcpkg/ports/arrow/vcpkg.json:
##########
@@ -0,0 +1,149 @@
+{
+ "name": "arrow",
+ "version": "18.0.0",
Review Comment:
Version alignment is reasonable, but it needs a separate change. A simple
native 15->18 switch is unsafe: the standalone Arrow patches do not all apply
to Arrow 18, and ppc64le builds locally install Arrow Java JNI artifacts while
Spark 3.x expects Arrow 15.0.0 and Spark 4.x expects 18.1.0. This PR keeps the
existing standalone path at 15 and limits its scope to vcpkg-managed Arrow 18;
native/Java/PPC alignment will be handled together in a follow-up.
##########
cpp/velox/CMakeLists.txt:
##########
@@ -246,6 +246,11 @@ endif()
add_library(velox SHARED ${VELOX_SRCS})
+if(ENABLE_GLUTEN_VCPKG)
+ find_package(xxHash CONFIG REQUIRED)
+ target_link_libraries(velox PUBLIC xxHash::xxhash)
+endif()
Review Comment:
I tested this suggestion, but xxHash must remain PUBLIC with the current
target graph. Making it PRIVATE caused the full Debug vcpkg build to fail while
linking `row_based_checksum_test`: `RowBasedChecksumTest.cc` directly
references `XXH64`, and the executable relies on the `velox` usage requirement
to receive xxHash. The final code intentionally keeps PUBLIC; the complete
Debug tests/benchmarks/Maven build passes.
##########
cpp/velox/CMakeLists.txt:
##########
@@ -342,12 +347,28 @@ if(BUILD_TESTS)
import_library(
facebook::velox::file_test_utils
${VELOX_BUILD_PATH}/velox/common/file/tests/libvelox_file_test_utils.a)
+ import_library(
+ facebook::velox::vector_fuzzer_util
+ ${VELOX_BUILD_PATH}/velox/vector/fuzzer/libvelox_vector_fuzzer_util.a)
+ import_library(
+ facebook::velox::constrained_vector_generator
+
${VELOX_BUILD_PATH}/velox/vector/fuzzer/libvelox_constrained_vector_generator.a
+ )
+ target_link_libraries(facebook::velox::constrained_vector_generator
+ INTERFACE facebook::velox::vector_fuzzer_util)
+ import_library(
+ facebook::velox::vector_fuzzer
+ ${VELOX_BUILD_PATH}/velox/vector/fuzzer/libvelox_vector_fuzzer.a)
+ target_link_libraries(
+ facebook::velox::vector_fuzzer
+ INTERFACE facebook::velox::vector_fuzzer_util
+ facebook::velox::constrained_vector_generator)
Review Comment:
Yes, indirectly. Velox and Gluten are configured in separate CMake
invocations, so Gluten cannot reuse Velox test targets and imports their static
archives by path. That loses the original transitive target metadata. `nm -uC
libvelox_exec_test_lib.a` confirms unresolved `VectorFuzzer::*` references, so
these imported fuzzer archives reconstruct the dependency chain required by the
clean vcpkg test build.
##########
cpp/velox/benchmarks/CMakeLists.txt:
##########
@@ -20,6 +20,9 @@ target_include_directories(
${CMAKE_SOURCE_DIR}/core)
target_link_libraries(velox_benchmark_common PUBLIC velox benchmark::benchmark
google::glog)
+if(BUILD_TESTS)
+ target_link_libraries(velox_benchmark_common PRIVATE GTest::gtest)
+endif()
Review Comment:
This link is required when tests and benchmarks are built together. `velox`
propagates the imported Velox test archives to benchmark consumers, and those
archives reference GTest. I verified this by removing the link and running the
full Debug vcpkg package build: `generic_benchmark` failed with undefined
`testing::internal::AssertHelper` symbols from `libvelox_exec_test_lib.a`.
Restoring this conditional link makes all tests and benchmarks link
successfully.
##########
ep/build-velox/src/build-velox.sh:
##########
@@ -166,6 +169,14 @@ function compile {
-DCMAKE_CUDA_COMPILER=/usr/local/cuda/bin/nvcc"
fi
if [ -n "${GLUTEN_VCPKG_ENABLED:-}" ]; then
+ # Flex is a host build tool, so admit only its matching executable and
headers.
+ FLEX_EXECUTABLE="$(realpath "$(command -v flex)")"
+ FLEX_INCLUDE_DIR="$(dirname "$(dirname "${FLEX_EXECUTABLE}")")/include"
+ if [ ! -f "${FLEX_INCLUDE_DIR}/FlexLexer.h" ]; then
+ echo "FlexLexer.h not found for ${FLEX_EXECUTABLE}: ${FLEX_INCLUDE_DIR}"
>&2
+ return 1
+ fi
+ COMPILE_OPTION="$COMPILE_OPTION -DFLEX_EXECUTABLE=${FLEX_EXECUTABLE}
-DFLEX_INCLUDE_DIR=${FLEX_INCLUDE_DIR}"
COMPILE_OPTION="$COMPILE_OPTION -DVELOX_GFLAGS_TYPE=static"
fi
Review Comment:
The temporary Flex path workaround has been removed in 50cc99919.
Prerequisite PR #12627 installed Flex 2.6.4 under `/usr`, and the rebuilt
`apache/gluten:vcpkg-centos-7-gcc13` image completed successfully, so explicit
Flex discovery and this error path are no longer needed.
##########
dev/vcpkg/ports/arrow/portfile.cmake:
##########
@@ -0,0 +1,152 @@
+vcpkg_download_distfile(
+ ARCHIVE_PATH
+ URLS
"https://archive.apache.org/dist/arrow/arrow-${VERSION}/apache-arrow-${VERSION}.tar.gz";
+ FILENAME apache-arrow-${VERSION}.tar.gz
+ SHA512
4df30ab5561da695eaa864422626b9898555d86ca56835c3b8a8ca93a1dbaf081582bb36e2440d1daf7e1dd48c76941f1152a4f25ce0dbcc1c2abe244a00c05e
+)
+vcpkg_extract_source_archive(
+ SOURCE_PATH
+ ARCHIVE ${ARCHIVE_PATH}
+ PATCHES
+ android.patch
+ msvc-static-name.patch
+ utf8proc.patch
+ thrift.patch
+ arrow-testing-boost.patch
+ arrow-testing-static-boost.patch
+)
+
+vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
+ FEATURES
+ acero ARROW_ACERO
+ compute ARROW_COMPUTE
+ csv ARROW_CSV
+ cuda ARROW_CUDA
+ dataset ARROW_DATASET
+ filesystem ARROW_FILESYSTEM
+ flight ARROW_FLIGHT
+ flightsql ARROW_FLIGHT_SQL
+ gcs ARROW_GCS
+ jemalloc ARROW_JEMALLOC
+ json ARROW_JSON
+ mimalloc ARROW_MIMALLOC
+ orc ARROW_ORC
+ parquet ARROW_PARQUET
+ parquet PARQUET_REQUIRE_ENCRYPTION
+ s3 ARROW_S3
+ testing ARROW_TESTING
+)
+
+if(VCPKG_TARGET_IS_WINDOWS AND NOT VCPKG_TARGET_IS_MINGW)
+ list(APPEND FEATURE_OPTIONS "-DARROW_USE_NATIVE_INT128=OFF")
+endif()
+
+string(COMPARE EQUAL ${VCPKG_LIBRARY_LINKAGE} "dynamic" ARROW_BUILD_SHARED)
+string(COMPARE EQUAL ${VCPKG_LIBRARY_LINKAGE} "static" ARROW_BUILD_STATIC)
+string(COMPARE EQUAL ${VCPKG_LIBRARY_LINKAGE} "dynamic"
ARROW_DEPENDENCY_USE_SHARED)
+
+vcpkg_cmake_configure(
+ SOURCE_PATH "${SOURCE_PATH}/cpp"
+ OPTIONS
+ ${FEATURE_OPTIONS}
+ -DARROW_BUILD_SHARED=${ARROW_BUILD_SHARED}
+ -DARROW_BUILD_STATIC=${ARROW_BUILD_STATIC}
+ -DARROW_BUILD_TESTS=OFF
+ -DARROW_DEPENDENCY_SOURCE=SYSTEM
+ -DARROW_DEPENDENCY_USE_SHARED=${ARROW_DEPENDENCY_USE_SHARED}
+ -DARROW_PACKAGE_KIND=vcpkg
+ -DARROW_WITH_BROTLI=OFF
+ -DARROW_WITH_BZ2=OFF
+ -DARROW_WITH_LZ4=ON
+ -DARROW_WITH_SNAPPY=ON
+ -DARROW_WITH_ZLIB=ON
+ -DARROW_WITH_ZSTD=ON
Review Comment:
Fixed in 50cc99919. The Arrow overlay no longer declares Brotli or BZip2
directly when both codecs are disabled. Gluten’s root vcpkg manifest still
directly requires BZip2 for the broader Velox dependency graph.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
