[GLUTEN-7139][VL] Manage Velox Arrow through vcpkg#12625
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses #7139 by making vcpkg the single owner of the native Arrow dependency graph used by Velox/Gluten (Velox backend), deprecating the standalone “build Arrow” path in vcpkg mode, and tightening CMake/pkg-config discovery to avoid leaking /usr/local/other inherited prefixes into the build.
Changes:
- Add an Apache Arrow 18.0.0 vcpkg overlay port (including a lightweight
testingfeature to installlibarrow_testing.awithout building Arrow’s full test suite) and wire it into the vcpkg manifest. - Update Velox/Gluten build scripts and CMake wiring to force SYSTEM dependency resolution in vcpkg mode, keep Arrow discovery consistent, and link against exported Arrow targets.
- Update notebooks, Dockerfiles, and CI workflows to stop invoking
--build_arrow=ON/build_arrowwhen vcpkg is enabled.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/workload/benchmark_velox/initialize.ipynb | Removes the “force build Arrow on first build” notebook step for vcpkg builds. |
| ep/build-velox/src/get-velox.sh | Simplifies applying the provided Arrow patch into the Velox checkout. |
| ep/build-velox/src/build-velox.sh | Forces SYSTEM deps in vcpkg mode and prevents prefix-path leakage into Velox builds. |
| dev/vcpkg/vcpkg.json | Adds arrow (with testing feature) to the vcpkg manifest dependencies. |
| dev/vcpkg/vcpkg-configuration.json | Pins Boost registry baseline separately to align modular Boost ports (1.84.x). |
| dev/vcpkg/toolchain.cmake | Disables package registries, prefers config packages, and ignores common inherited prefixes (e.g., /usr/local). |
| dev/vcpkg/env.sh | Isolates pkg-config discovery to vcpkg-installed .pc files (via PKG_CONFIG_LIBDIR). |
| dev/docker/Dockerfile.centos9-static-build | Stops invoking build_arrow in vcpkg mode; clarifies dependency ownership messaging. |
| dev/docker/Dockerfile.centos8-static-build | Stops invoking build_arrow in vcpkg mode; clarifies dependency ownership messaging. |
| dev/docker/Dockerfile.centos8-gcc13-static-build | Stops invoking build_arrow in vcpkg mode; clarifies dependency ownership messaging. |
| dev/docker/Dockerfile.centos7-static-build | Stops invoking build_arrow in vcpkg mode; clarifies dependency ownership messaging. |
| dev/docker/Dockerfile.centos7-gcc13-static-build | Stops invoking build_arrow in vcpkg mode; clarifies dependency ownership messaging. |
| dev/builddeps-veloxbe.sh | Deprecates --build_arrow=ON / build_arrow when vcpkg is active and avoids prefix leakage into Gluten CMake in vcpkg mode. |
| dev/builddep-veloxbe-inc.sh | Ensures vcpkg env variables are set consistently and isolates pkg-config lookup to vcpkg. |
| cpp/velox/operators/writer/VeloxParquetDataSource.h | Removes an Arrow filesystem header include (reducing Arrow surface usage in this header). |
| cpp/velox/CMakeLists.txt | Adds xxHash target linkage in vcpkg mode, fixes test-lib import/linkage, and adjusts thrift linking behavior under vcpkg. |
| cpp/velox/benchmarks/CMakeLists.txt | Adds conditional gtest linkage for benchmark common library when tests are enabled. |
| cpp/core/CMakeLists.txt | Skips bundled Arrow lib discovery in vcpkg mode and links Gluten to Arrow::arrow consistently. |
| cpp/CMakeLists.txt | Adds google::glog alias for vcpkg-provided glog::glog to keep downstream target names stable. |
| cpp/CMake/ConfigArrow.cmake | Uses find_package(Arrow CONFIG) in vcpkg mode and provides an Arrow::arrow target when missing. |
| .github/workflows/velox_weekly.yml | Removes --build_arrow=ON from vcpkg packaging workflow invocation. |
| dev/vcpkg/ports/arrow/vcpkg.json | New overlay port definition for Arrow 18.0.0 with feature modeling (including testing). |
| dev/vcpkg/ports/arrow/portfile.cmake | New build/install logic for Arrow 18 via vcpkg, including config fixups and usage snippets. |
| dev/vcpkg/ports/arrow/android.patch | Patch to improve Android compatibility in Arrow’s build sources. |
| dev/vcpkg/ports/arrow/msvc-static-name.patch | Patch to normalize static lib naming behavior for MSVC builds. |
| dev/vcpkg/ports/arrow/utf8proc.patch | Patch to adjust utf8proc package discovery behavior under vcpkg packaging. |
| dev/vcpkg/ports/arrow/thrift.patch | Patch to adjust Thrift discovery logic in Arrow’s build. |
| dev/vcpkg/ports/arrow/arrow-testing-boost.patch | Patch to fix Arrow testing library’s Boost dependency linking for vcpkg usage. |
| dev/vcpkg/ports/arrow/arrow-testing-static-boost.patch | Patch to ensure Arrow testing’s Boost requirements are correct for static usage. |
| dev/vcpkg/ports/arrow/usage | New vcpkg “usage” snippet for Arrow CMake consumers. |
| dev/vcpkg/ports/arrow/usage-acero | New vcpkg “usage” snippet for ArrowAcero CMake consumers. |
| dev/vcpkg/ports/arrow/usage-dataset | New vcpkg “usage” snippet for ArrowDataset CMake consumers. |
| dev/vcpkg/ports/arrow/usage-flight | New vcpkg “usage” snippet for ArrowFlight CMake consumers. |
| dev/vcpkg/ports/arrow/usage-flightsql | New vcpkg “usage” snippet for ArrowFlightSql CMake consumers. |
| dev/vcpkg/ports/arrow/usage-parquet | New vcpkg “usage” snippet for Parquet CMake consumers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| 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) |
There was a problem hiding this comment.
Fixed in 2ab3f8d. The vcpkg Thrift path now uses VCPKG_TARGET_TRIPLET when available and falls back to the processor-derived triplet only when the active triplet is unavailable.
Clean vcpkg builds skipped Gluten's standalone Arrow installer but could still fall back to bundled or host Arrow, mixing Arrow versions and losing static dependency usage requirements. The Arrow testing library also exposed incompatible Boost.Process selection with newer Boost releases. Add an Arrow 18 overlay with testing support, align Boost at 1.84, force vcpkg-managed SYSTEM resolution, and consume exported CMake targets with the required static dependency propagation. Isolate host package paths, deprecate duplicate vcpkg Arrow installation, and update callers so clean builds cannot silently depend on /usr/local. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 81cd27bb-c736-4cf5-b65f-2aeb9f954996
97809f3 to
916536a
Compare
| bash ./dev/builddeps-veloxbe.sh --enable_vcpkg=ON --build_tests=ON --enable_s3=ON --enable_gcs=ON \ | ||
| --enable_hdfs=ON --enable_abfs=ON build_arrow; \ | ||
| --enable_hdfs=ON --enable_abfs=ON true; \ |
There was a problem hiding this comment.
Addressed in 4b77376. These static images intentionally populate VCPKG_BINARY_SOURCES without building the full Velox/Gluten backend. The opaque positional true has been replaced with a direct Bash source ./dev/vcpkg/env.sh ..., which runs vcpkg initialization/install and then exits.
| bash ./dev/builddeps-veloxbe.sh --enable_vcpkg=ON --build_tests=ON --enable_s3=ON --enable_gcs=ON \ | ||
| --enable_hdfs=ON --enable_abfs=ON build_arrow; \ | ||
| --enable_hdfs=ON --enable_abfs=ON true; \ |
There was a problem hiding this comment.
Addressed in 4b77376. These static images intentionally populate VCPKG_BINARY_SOURCES without building the full Velox/Gluten backend. The opaque positional true has been replaced with a direct Bash source ./dev/vcpkg/env.sh ..., which runs vcpkg initialization/install and then exits.
| bash ./dev/builddeps-veloxbe.sh --enable_vcpkg=ON --build_tests=ON --enable_s3=ON --enable_gcs=ON \ | ||
| --enable_hdfs=ON --enable_abfs=ON build_arrow; \ | ||
| --enable_hdfs=ON --enable_abfs=ON true; \ |
There was a problem hiding this comment.
Addressed in 4b77376. These static images intentionally populate VCPKG_BINARY_SOURCES without building the full Velox/Gluten backend. The opaque positional true has been replaced with a direct Bash source ./dev/vcpkg/env.sh ..., which runs vcpkg initialization/install and then exits.
| bash ./dev/builddeps-veloxbe.sh --enable_vcpkg=ON --build_tests=ON --enable_s3=ON --enable_gcs=ON \ | ||
| --enable_hdfs=ON --enable_abfs=ON build_arrow; \ | ||
| --enable_hdfs=ON --enable_abfs=ON true; \ |
There was a problem hiding this comment.
Addressed in 4b77376. These static images intentionally populate VCPKG_BINARY_SOURCES without building the full Velox/Gluten backend. The opaque positional true has been replaced with a direct Bash source ./dev/vcpkg/env.sh ..., which runs vcpkg initialization/install and then exits.
| bash ./dev/builddeps-veloxbe.sh --enable_vcpkg=ON --build_tests=ON --enable_s3=ON --enable_gcs=ON \ | ||
| --enable_hdfs=ON --enable_abfs=ON build_arrow; \ | ||
| --enable_hdfs=ON --enable_abfs=ON true; \ |
There was a problem hiding this comment.
Addressed in 4b77376. These static images intentionally populate VCPKG_BINARY_SOURCES without building the full Velox/Gluten backend. The opaque positional true has been replaced with a direct Bash source ./dev/vcpkg/env.sh ..., which runs vcpkg initialization/install and then exits.
Use VCPKG_TARGET_TRIPLET when locating vcpkg FBThrift archives so custom triplets do not fall back to an architecture-derived directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 81cd27bb-c736-4cf5-b65f-2aeb9f954996
Source the vcpkg environment explicitly in dependency-cache images instead of using a no-op positional command through builddeps-veloxbe.sh. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 81cd27bb-c736-4cf5-b65f-2aeb9f954996
b8ee765 to
4b77376
Compare
| ${SCRIPT_ROOT}/init.sh "$@" | ||
|
|
||
| unset PKG_CONFIG_PATH | ||
| export PKG_CONFIG_LIBDIR=${VCPKG_TRIPLET_INSTALL_DIR}/lib/pkgconfig:${VCPKG_TRIPLET_INSTALL_DIR}/share/pkgconfig |
There was a problem hiding this comment.
Fixed in cb24e4f. PKG_CONFIG_PATH is now unset and PKG_CONFIG_LIBDIR is set to the active vcpkg triplet before init.sh runs, so vcpkg install no longer inherits host pkg-config search paths.
Advance the scoped Boost 1.84 registry to its CMake packaging transition so Folly can resolve Boost config packages with CMP0167 enabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 81cd27bb-c736-4cf5-b65f-2aeb9f954996
Clear host pkg-config search paths before init.sh runs vcpkg install, preventing system and Conda packages from leaking into dependency builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 81cd27bb-c736-4cf5-b65f-2aeb9f954996
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
cpp/CMakeLists.txt:80
- In vcpkg mode,
GLUTEN_PREFIX_INCLUDE_DIRSstill collects include paths fromCMAKE_PREFIX_PATHunconditionally. This bypassesCMAKE_IGNORE_PREFIX_PATH(set by the vcpkg toolchain) and can re-introduce headers from ignored prefixes (e.g. /usr/local, conda) as system includes viatarget_include_directories(... BEFORE ...), undermining the poisoned-prefix isolation goal.
set(GLUTEN_PREFIX_INCLUDE_DIRS)
if(CMAKE_INSTALL_PREFIX
AND NOT ENABLE_GLUTEN_VCPKG
AND NOT CMAKE_INSTALL_PREFIX STREQUAL "/usr/local"
AND IS_DIRECTORY "${CMAKE_INSTALL_PREFIX}/include")
list(APPEND GLUTEN_PREFIX_INCLUDE_DIRS "${CMAKE_INSTALL_PREFIX}/include")
endif()
foreach(PREFIX_PATH IN LISTS CMAKE_PREFIX_PATH)
if(IS_DIRECTORY "${PREFIX_PATH}/include")
list(APPEND GLUTEN_PREFIX_INCLUDE_DIRS "${PREFIX_PATH}/include")
endif()
endforeach()
| The package arrow provides CMake targets: | ||
|
|
||
| find_package(Arrow CONFIG REQUIRED) | ||
| target_link_libraries(main PRIVATE "$<IF:$<BOOL:${ARROW_BUILD_STATIC}>,Arrow::arrow_static,Arrow::arrow_shared>") |
There was a problem hiding this comment.
Leaving this unchanged in this PR. The usage snippet is copied unchanged from the upstream vcpkg Arrow port and is only displayed as install-time guidance; Gluten does not consume it and selects the exported static target in cpp/CMake/ConfigArrow.cmake. Adjusting the generic upstream usage guidance is outside this integration change.
|
|
||
| find_package(Parquet CONFIG REQUIRED) | ||
| target_link_libraries(main PRIVATE "$<IF:$<BOOL:${ARROW_BUILD_STATIC}>,Parquet::parquet_static,Parquet::parquet_shared>") |
There was a problem hiding this comment.
Leaving this unchanged in this PR. This feature usage snippet is copied unchanged from the upstream vcpkg Arrow port and is only install-time guidance; it is not consumed by Gluten or enabled by Gluten’s Arrow dependency. A generic static/shared guidance change should be made upstream rather than diverging this overlay.
|
|
||
| find_package(ArrowFlight CONFIG REQUIRED) | ||
| target_link_libraries(main PRIVATE "$<IF:$<BOOL:${ARROW_BUILD_STATIC}>,ArrowFlight::arrow_flight_static,ArrowFlight::arrow_flight_shared>") |
There was a problem hiding this comment.
Leaving this unchanged in this PR. This feature usage snippet is copied unchanged from the upstream vcpkg Arrow port and is only install-time guidance; it is not consumed by Gluten or enabled by Gluten’s Arrow dependency. A generic static/shared guidance change should be made upstream rather than diverging this overlay.
|
|
||
| find_package(ArrowFlightSql CONFIG REQUIRED) | ||
| target_link_libraries(main PRIVATE "$<IF:$<BOOL:${ARROW_BUILD_STATIC}>,ArrowFlightSql::arrow_flight_sql_static,ArrowFlightSql::arrow_flight_sql_shared>") |
There was a problem hiding this comment.
Leaving this unchanged in this PR. This feature usage snippet is copied unchanged from the upstream vcpkg Arrow port and is only install-time guidance; it is not consumed by Gluten or enabled by Gluten’s Arrow dependency. A generic static/shared guidance change should be made upstream rather than diverging this overlay.
|
|
||
| find_package(ArrowAcero CONFIG REQUIRED) | ||
| target_link_libraries(main PRIVATE "$<IF:$<BOOL:${ARROW_BUILD_STATIC}>,ArrowAcero::arrow_acero_static,ArrowAcero::arrow_acero_shared>") |
There was a problem hiding this comment.
Leaving this unchanged in this PR. This feature usage snippet is copied unchanged from the upstream vcpkg Arrow port and is only install-time guidance; it is not consumed by Gluten or enabled by Gluten’s Arrow dependency. A generic static/shared guidance change should be made upstream rather than diverging this overlay.
|
|
||
| find_package(ArrowDataset CONFIG REQUIRED) | ||
| target_link_libraries(main PRIVATE "$<IF:$<BOOL:${ARROW_BUILD_STATIC}>,ArrowDataset::arrow_dataset_static,ArrowDataset::arrow_dataset_shared>") |
There was a problem hiding this comment.
Leaving this unchanged in this PR. This feature usage snippet is copied unchanged from the upstream vcpkg Arrow port and is only install-time guidance; it is not consumed by Gluten or enabled by Gluten’s Arrow dependency. A generic static/shared guidance change should be made upstream rather than diverging this overlay.
What changes are proposed in this pull request?
Fixes #7139.
This PR makes vcpkg the owner of native Arrow and the compile-time dependency graph used by Velox builds.
testingfeature that installslibarrow_testing.awithout building Arrow's full test suite./usr/local, inherited prefixes, and package registries while preserving OS/runtime dependencies.The result is that a clean
package-vcpkg.shbuild no longer requires a preinstalled/usr/localArrow or a separatedev/build-arrow.shinvocation. Java Arrow dependencies remain managed by Maven.How was this patch tested?
Built the Arrow vcpkg overlay and confirmed:
libarrow.alibarrow_testing.aarrow/api.hRan a poisoned-prefix Debug matrix with tests, benchmarks, S3, GCS, and HDFS enabled:
Ran the end-to-end build:
All ten Maven reactor modules completed successfully.
Regenerated Debug and Release Velox build graphs and confirmed there are no bare
-larrowor-larrow_testinglinks.Confirmed the compiler dependency graph contains no headers from
/usr/local/include.Was this patch authored or co-authored using generative AI tooling?
Generated-by: GitHub Copilot CLI 1.0.75