Copilot commented on code in PR #11712:
URL: https://github.com/apache/gluten/pull/11712#discussion_r3398256088
##########
dev/ci-velox-buildstatic-centos-7.sh:
##########
@@ -17,6 +17,6 @@
set -e
-export NUM_THREADS=4
+cp ep/build-velox/src/socket.h /usr/include/asm-generic/socket.h
./dev/builddeps-veloxbe.sh --enable_vcpkg=ON --build_arrow=OFF
--build_tests=OFF --build_benchmarks=OFF \
--build_examples=OFF --enable_s3=ON --enable_gcs=ON
--enable_hdfs=ON --enable_abfs=ON
Review Comment:
This script now overwrites a system header in /usr/include. That’s a
high-risk CI workaround (requires root, can mask real dependency issues, and
may affect other builds inside the container). Prefer installing the correct
kernel-headers package in the CI image or scoping the workaround to the build
(e.g., via include paths) without mutating /usr/include.
##########
ep/build-velox/src/socket.h:
##########
@@ -0,0 +1,105 @@
+#ifndef __ASM_GENERIC_SOCKET_H
+#define __ASM_GENERIC_SOCKET_H
+
+#include <asm/sockios.h>
+
Review Comment:
This file appears to vendor a Linux UAPI header (asm-generic/socket.h)
verbatim but lacks the standard Apache license header and any upstream
SPDX/license attribution. Adding kernel headers directly to the repository can
create licensing/compliance issues; it would be safer to depend on the
distro-provided kernel headers (or patch the build to avoid needing this
header) rather than committing a copy into the source tree.
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -17,8 +17,8 @@
set -exu
CURRENT_DIR=$(cd "$(dirname "$BASH_SOURCE")"; pwd)
-VELOX_REPO=https://github.com/IBM/velox.git
-VELOX_BRANCH=dft-2026_06_06
+VELOX_REPO=https://github.com/zhouyuan/velox.git
+VELOX_BRANCH=dft-2026_06_06-fbthrift
VELOX_ENHANCED_BRANCH=ibm-2026_06_06
Review Comment:
Defaulting VELOX_REPO to a personal fork makes builds less reproducible and
may break CI for contributors without access/without the branch. Consider
keeping an official upstream (e.g., the project’s standard Velox fork) as the
default and requiring `--velox_repo/--velox_branch` overrides for experimental
branches.
##########
dev/build-arrow.sh:
##########
@@ -53,7 +53,6 @@ function build_arrow_cpp() {
-DARROW_PROTOBUF_USE_SHARED=OFF \
-DARROW_DEPENDENCY_USE_SHARED=OFF \
-DARROW_DEPENDENCY_SOURCE=BUNDLED \
- -DARROW_WITH_THRIFT=ON \
-DARROW_WITH_LZ4=ON \
-DARROW_WITH_SNAPPY=ON \
-DARROW_WITH_ZLIB=${ARROW_WITH_ZLIB} \
Review Comment:
`-DARROW_WITH_THRIFT=ON` was removed from the Arrow CMake configuration, but
later in this script `build_arrow_cpp()` still installs Thrift from
`_build/thrift_ep-prefix/...`. If Thrift isn’t built anymore, the subsequent
`cd _build/thrift_ep-prefix/...` will fail. Either keep `ARROW_WITH_THRIFT`
enabled here or remove/guard the Thrift install step below to keep the script
consistent.
--
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]
