Copilot commented on code in PR #12625:
URL: https://github.com/apache/gluten/pull/12625#discussion_r3651694267
##########
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:
With `set -e`, `FLEX_EXECUTABLE="$(realpath "$(command -v flex)")"` will
fail with a hard-to-diagnose error if `flex` is not installed or not on PATH.
Add an explicit presence check and a clearer error before calling `realpath` so
vcpkg builds fail fast with actionable output.
##########
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:
The vcpkg Thrift static archives are currently added to `velox` as `PUBLIC`
dependencies via absolute paths. Since `velox` is a SHARED library, this
typically shouldn't be a propagated usage requirement and can unnecessarily
force all linkers of `velox` to see these archives/paths. Prefer `PRIVATE` here
unless a consumer of `velox` headers truly needs to link Thrift directly.
##########
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:
`velox` is a SHARED library, and xxhash is an implementation dependency (no
public headers in this repo include xxhash). Linking it as `PUBLIC` needlessly
leaks it as a usage requirement to downstream targets and can introduce
duplicate-link issues. Prefer `PRIVATE` unless the dependency is required by
Velox's public interface.
##########
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:
The overlay port declares `brotli` and `bzip2` as hard dependencies
(dev/vcpkg/ports/arrow/vcpkg.json), but the portfile explicitly disables both
(`-DARROW_WITH_BROTLI=OFF` and `-DARROW_WITH_BZ2=OFF`). This is inconsistent
(extra build time/closure) and also unexpectedly removes codecs that
Arrow/Parquet users may rely on. Either enable them here or drop them from the
dependency list.
--
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]
