[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.
Disable Brotli and BZ2 in the Arrow overlay to match Velox and avoid unresolved codec symbols when libarrow.a is consumed through Velox FindArrow targets. 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 37 out of 37 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (4)
.github/workflows/velox_weekly.yml:78
if: ${{ false }}disables this weekly build job entirely, which removes coverage that the workflow is meant to provide. Restore the previous repository guard (or remove theifentirely) rather than hard-disabling it.
weekly-build-on-ubuntu:
strategy:
fail-fast: false
matrix:
os: [ "ubuntu:22.04" ]
if: ${{ false }}
runs-on: ubuntu-22.04
.github/workflows/velox_weekly.yml:121
- This job is currently hard-disabled via
if: ${{ false }}, preventing scheduled coverage from running. Restore the original condition if the goal is to avoid running on forks.
weekly-build-on-openeuler:
strategy:
fail-fast: false
matrix:
os: [ "openeuler/openeuler:24.03" ]
if: ${{ false }}
runs-on: ubuntu-24.04-arm
.github/workflows/velox_weekly.yml:150
- This job is hard-disabled with
if: ${{ false }}, so the vcpkg weekly path will never execute. Restore the previous condition so the job runs on the upstream repository.
weekly-build-on-openeuler-vcpkg:
strategy:
fail-fast: false
matrix:
os: [ "openeuler/openeuler:24.03" ]
if: ${{ false }}
runs-on: ubuntu-24.04-arm
.github/workflows/velox_weekly.yml:176
- This macOS weekly job is hard-disabled with
if: ${{ false }}, so it will never run even on schedule. Restore the original guard rather than disabling the job.
weekly-build-on-macos:
strategy:
fail-fast: false
if: ${{ false }}
runs-on: macos-15
| find_program(_GLUTEN_DIAGNOSTIC_CXX NAMES c++ g++ NO_CACHE) | ||
| find_program(_GLUTEN_DIAGNOSTIC_FLEX NAMES flex NO_CACHE) | ||
| find_path(_GLUTEN_DIAGNOSTIC_FLEX_INCLUDE_DIR NAMES FlexLexer.h NO_CACHE) | ||
| message(STATUS "[Gluten vcpkg diagnostics] ENV{CC}=$ENV{CC}") | ||
| message(STATUS "[Gluten vcpkg diagnostics] ENV{CXX}=$ENV{CXX}") | ||
| message(STATUS "[Gluten vcpkg diagnostics] ENV{PATH}=$ENV{PATH}") | ||
| message(STATUS "[Gluten vcpkg diagnostics] CMAKE_C_COMPILER=${CMAKE_C_COMPILER}") | ||
| message(STATUS "[Gluten vcpkg diagnostics] CMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}") | ||
| message(STATUS "[Gluten vcpkg diagnostics] c++=${_GLUTEN_DIAGNOSTIC_CXX}") | ||
| message(STATUS "[Gluten vcpkg diagnostics] flex=${_GLUTEN_DIAGNOSTIC_FLEX}") | ||
| message(STATUS "[Gluten vcpkg diagnostics] FlexLexer.h=${_GLUTEN_DIAGNOSTIC_FLEX_INCLUDE_DIR}") | ||
| unset(_GLUTEN_DIAGNOSTIC_CXX) | ||
| unset(_GLUTEN_DIAGNOSTIC_FLEX) | ||
| unset(_GLUTEN_DIAGNOSTIC_FLEX_INCLUDE_DIR) |
| if [ "$ENABLE_VCPKG" = "ON" ]; then | ||
| # vcpkg will install static depends and init build environment | ||
| BUILD_OPTIONS="--build_tests=$BUILD_TESTS --enable_s3=$ENABLE_S3 --enable_gcs=$ENABLE_GCS \ | ||
| --enable_hdfs=$ENABLE_HDFS --enable_abfs=$ENABLE_ABFS" | ||
| print_vcpkg_diagnostics "before env.sh" | ||
| source ./dev/vcpkg/env.sh ${BUILD_OPTIONS} | ||
| print_vcpkg_diagnostics "after env.sh" | ||
| fi |
| weekly-build-on-centos: | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [ "centos:8", "quay.io/centos/centos:stream9" ] | ||
| if: ${{ startsWith(github.repository, 'apache/') }} | ||
| if: ${{ false }} | ||
| runs-on: ubuntu-22.04 |
| cpp-test-udf-test: | ||
| if: ${{ false }} | ||
| runs-on: ubuntu-22.04 |
| build-cudf-centos-9: | ||
| if: ${{ false }} | ||
| runs-on: ubuntu-22.04 |
| build-fast-build-test: | ||
| if: ${{ false }} | ||
| runs-on: ubuntu-22.04 |
| cpp-test-udf-test: | ||
| if: ${{ false }} | ||
| runs-on: ubuntu-24.04-arm |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (9)
.github/workflows/velox_weekly.yml:44
- This job is permanently disabled via
if: ${{ false }}so the workflow will never run on schedule or PRs. If the intent is to keep weekly CI active, restore the prior repo-gated condition (or remove theifentirely).
if: ${{ false }}
.github/workflows/velox_weekly.yml:77
- This job is permanently disabled via
if: ${{ false }}so the workflow will never run on schedule or PRs. If the intent is to keep weekly CI active, restore the prior repo-gated condition (or remove theifentirely).
if: ${{ false }}
.github/workflows/velox_weekly.yml:120
- This job is permanently disabled via
if: ${{ false }}so the workflow will never run on schedule or PRs. If the intent is to keep weekly CI active, restore the prior repo-gated condition (or remove theifentirely).
if: ${{ false }}
.github/workflows/velox_weekly.yml:149
- This job is permanently disabled via
if: ${{ false }}so the workflow will never run on schedule or PRs. If the intent is to keep weekly CI active, restore the prior repo-gated condition (or remove theifentirely).
if: ${{ false }}
.github/workflows/velox_weekly.yml:175
- This job is permanently disabled via
if: ${{ false }}so the workflow will never run on schedule or PRs. If the intent is to keep weekly CI active, restore the prior repo-gated condition (or remove theifentirely).
if: ${{ false }}
.github/workflows/velox_backend_x86.yml:989
- This job is currently disabled with
if: ${{ false }}, which removes its coverage from CI. If this is not intentional, drop theifso the job runs again.
if: ${{ false }}
runs-on: ubuntu-22.04
.github/workflows/velox_backend_x86.yml:1073
- This job is currently disabled with
if: ${{ false }}, which removes its coverage from CI. If this is not intentional, drop theifso the job runs again.
if: ${{ false }}
runs-on: ubuntu-22.04
.github/workflows/velox_backend_x86.yml:1124
- This job is currently disabled with
if: ${{ false }}, which removes its coverage from CI. If this is not intentional, drop theifso the job runs again.
if: ${{ false }}
runs-on: ubuntu-22.04
.github/workflows/velox_backend_arm.yml:148
- This job is currently disabled with
if: ${{ false }}, which removes its coverage from CI. If this is not intentional, drop theifso the job runs again.
if: ${{ false }}
runs-on: ubuntu-24.04-arm
| yum install -y flex | ||
| mv /usr/local/bin/flex /usr/local/bin/flex-2.6.4 | ||
| hash -r |
| yum install -y flex | ||
| mv /usr/local/bin/flex /usr/local/bin/flex-2.6.4 | ||
| hash -r |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (10)
dev/vcpkg/toolchain.cmake:68
NO_CACHEis not supported by this repo’s CMake minimum version (3.16), so thesefind_program/find_pathcalls will fail at configure time. Drop theNO_CACHEarguments (or bump the project’s CMake minimum if you truly require them).
.github/workflows/velox_weekly.yml:45- This job is currently hard-disabled via
if: ${{ false }}, which prevents the weekly workflow from running at all. If this isn’t intentional long-term, restore the repository-scoped condition (or gate behind an input/variable) so CI coverage isn’t silently removed.
weekly-build-on-centos:
strategy:
fail-fast: false
matrix:
os: [ "centos:8", "quay.io/centos/centos:stream9" ]
if: ${{ false }}
runs-on: ubuntu-22.04
.github/workflows/velox_weekly.yml:78
- This job is currently hard-disabled via
if: ${{ false }}, which prevents the weekly workflow from running at all. If this isn’t intentional long-term, restore the repository-scoped condition (or gate behind an input/variable) so CI coverage isn’t silently removed.
weekly-build-on-ubuntu:
strategy:
fail-fast: false
matrix:
os: [ "ubuntu:22.04" ]
if: ${{ false }}
runs-on: ubuntu-22.04
.github/workflows/velox_weekly.yml:121
- This job is currently hard-disabled via
if: ${{ false }}, which prevents the weekly workflow from running at all. If this isn’t intentional long-term, restore the repository-scoped condition (or gate behind an input/variable) so CI coverage isn’t silently removed.
weekly-build-on-openeuler:
strategy:
fail-fast: false
matrix:
os: [ "openeuler/openeuler:24.03" ]
if: ${{ false }}
runs-on: ubuntu-24.04-arm
.github/workflows/velox_weekly.yml:150
- This job is currently hard-disabled via
if: ${{ false }}, which prevents the weekly workflow from running at all. If this isn’t intentional long-term, restore the repository-scoped condition (or gate behind an input/variable) so CI coverage isn’t silently removed.
weekly-build-on-openeuler-vcpkg:
strategy:
fail-fast: false
matrix:
os: [ "openeuler/openeuler:24.03" ]
if: ${{ false }}
runs-on: ubuntu-24.04-arm
.github/workflows/velox_weekly.yml:176
- This job is currently hard-disabled via
if: ${{ false }}. Unless the intent is to permanently remove macOS weekly validation, restore the prior repository-scoped condition (or gate behind an input/variable) so the job can run in apache/* repos.
weekly-build-on-macos:
strategy:
fail-fast: false
if: ${{ false }}
runs-on: macos-15
.github/workflows/velox_backend_x86.yml:988
- This job is hard-disabled via
if: ${{ false }}, which removes CI coverage for UDF tests. If the intent is just to avoid running in forks, consider gating on the upstream repo instead of permanently disabling the job.
if: ${{ false }}
.github/workflows/velox_backend_x86.yml:1072
- This job is hard-disabled via
if: ${{ false }}, which removes CI coverage for cuDF builds. If it’s only being disabled temporarily, please restore a real condition (e.g., repository-scoped gating) so the job can run again in apache/*.
if: ${{ false }}
.github/workflows/velox_backend_x86.yml:1123
- This job is hard-disabled via
if: ${{ false }}, which removes coverage for the fast-build profile check. If this is intended to run only in apache/* repos, gate it accordingly rather than disabling it outright.
if: ${{ false }}
.github/workflows/velox_backend_arm.yml:147
- This job is hard-disabled via
if: ${{ false }}, which removes CI coverage for UDF tests on ARM. If the intent is to avoid running in forks, gate on the upstream repo instead of disabling the job entirely.
if: ${{ false }}
Delay package-only root lookup until vcpkg has initialized its search paths, avoiding the root prefix side effect that made ARM CMake select the system compiler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 81cd27bb-c736-4cf5-b65f-2aeb9f954996
Pass the selected host Flex executable and its matching include directory explicitly so vcpkg package isolation does not hide FlexLexer.h from existing CentOS 7 images. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 81cd27bb-c736-4cf5-b65f-2aeb9f954996
07d3f88 to
cb96fb9
Compare
| 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 |
| -DARROW_WITH_BROTLI=OFF | ||
| -DARROW_WITH_BZ2=OFF | ||
| -DARROW_WITH_LZ4=ON | ||
| -DARROW_WITH_SNAPPY=ON | ||
| -DARROW_WITH_ZLIB=ON | ||
| -DARROW_WITH_ZSTD=ON |
| if(ENABLE_GLUTEN_VCPKG) | ||
| find_package(xxHash CONFIG REQUIRED) | ||
| target_link_libraries(velox PUBLIC xxHash::xxhash) | ||
| endif() |
| 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) |
philo-he
left a comment
There was a problem hiding this comment.
Thanks for the PR. Just had a quick look, and left some minor comments.
| google::glog) | ||
| if(BUILD_TESTS) | ||
| target_link_libraries(velox_benchmark_common PRIVATE GTest::gtest) | ||
| endif() |
There was a problem hiding this comment.
Just curious why we need the above code. Could you clarify?
| target_link_libraries( | ||
| facebook::velox::vector_fuzzer | ||
| INTERFACE facebook::velox::vector_fuzzer_util | ||
| facebook::velox::constrained_vector_generator) |
There was a problem hiding this comment.
Are the above changes also related to this PR?
| @@ -0,0 +1,149 @@ | |||
| { | |||
| "name": "arrow", | |||
| "version": "18.0.0", | |||
There was a problem hiding this comment.
It seems that the other build path still uses arrow 15.0.0. Should we use a consistent version? Perhaps, we need a follow-up PR to upgrade the arrow version in the following link.
Line 23 in 474ded1
| 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/ |
There was a problem hiding this comment.
Could you check the historical commit that introduced SUDO_CMD="sudo"? Not sure whether we still need it.
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