Copilot commented on code in PR #11712:
URL: https://github.com/apache/gluten/pull/11712#discussion_r3403319104
##########
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 kernel UAPI header
(asm-generic/socket.h) verbatim and it lacks the project’s standard ASF license
header. Vendoring system headers like this is brittle (may not match the target
kernel headers) and can create licensing/compliance issues; prefer fixing the
build image to provide the correct kernel headers (or adjusting include paths)
instead of copying a header into the repo.
##########
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
Review Comment:
Copying a vendored header into /usr/include at build time mutates the system
include tree, requires root, and can lead to hard-to-debug ABI/header
mismatches across environments. Prefer installing the proper kernel-headers
package (or using a container image that already includes it) rather than
overwriting /usr/include/asm-generic/socket.h.
##########
dev/vcpkg/ports/fbthrift/vcpkg.json:
##########
@@ -0,0 +1,35 @@
+{
+ "name": "fbthrift",
+ "version-string": "2026.02.23.00",
+ "description": "Facebook's branch of Apache Thrift, including a new C++
server.",
+ "homepage": "https://github.com/facebook/fbthrift";,
+ "license": "Apache-2.0",
+ "supports": "x64 & static",
Review Comment:
This port is declared as `supports: "x64 & static"`, but the repository has
ARM CI (arm64-linux-neon triplet). If fbthrift is required for the Velox
feature set, this support restriction will either break ARM builds or force
reliance on --allow-unsupported. Consider widening the supported platforms
(e.g., non-Windows static) if fbthrift actually builds on arm64.
##########
dev/vcpkg/ports/mvfst/vcpkg.json:
##########
@@ -0,0 +1,30 @@
+{
+ "name": "mvfst",
+ "version-string": "2026.02.23.00",
+ "description": "mvfst (Pronounced move fast) is a client and server
implementation of IETF QUIC protocol in C++ by Facebook.",
+ "homepage": "https://github.com/facebook/mvfst";,
+ "license": "MIT",
+ "supports": "!windows | static",
Review Comment:
The vcpkg "supports" expression uses `|` (OR): `!windows | static`. This
unintentionally marks the port as supported on Windows as long as the linkage
is static. If the intent is “non-Windows AND static”, this should use `&`.
##########
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
Review Comment:
Defaulting VELOX_REPO to a personal fork makes builds non-reproducible and
can break CI if the fork/branch is deleted or force-pushed. It’s safer to keep
an org-owned upstream (e.g., IBM/velox or facebookincubator/velox) as the
default and use the existing --velox_repo/--velox_branch flags for custom
testing branches.
##########
dev/vcpkg/vcpkg.json:
##########
@@ -24,6 +24,8 @@
"name": "folly",
"features": ["zstd", "lz4", "lzma", "snappy"]
},
+ "fizz",
+ "fbthrift",
Review Comment:
There’s a stray tab indentation before "fizz" which is inconsistent with the
surrounding JSON formatting and makes diffs noisier.
##########
dev/vcpkg/init.sh:
##########
@@ -92,7 +92,7 @@ if [ "${VCPKG_DYNAMIC_OPENSSL:-OFF}" = "ON" ]; then
fi
-$VCPKG install --no-print-usage \
+$VCPKG install --no-print-usage --allow-unsupported \
--triplet="${VCPKG_TRIPLET}" --host-triplet="${VCPKG_TRIPLET}"
${EXTRA_FEATURES}
Review Comment:
Adding --allow-unsupported makes vcpkg install succeed even when ports
declare they don’t support the current triplet/platform, which can hide real
incompatibilities and lead to later build/runtime failures. It’s better to keep
vcpkg strict and instead fix the relevant port "supports" expressions (or
conditionally enable features) so unsupported configs fail fast.
##########
cpp/velox/CMakeLists.txt:
##########
@@ -375,6 +375,22 @@ else()
endif()
target_link_libraries(velox PUBLIC external::geos)
+find_package(veloxthrift CONFIG)
+if(veloxthrift_FOUND AND TARGET veloxthrift::veloxthrift)
+ add_library(external::veloxthrift ALIAS veloxthrift::veloxthrift)
+else()
+ message(STATUS "import Velox bundled veloxthrift")
+ import_library(external::veloxthrift
${VELOX_BUILD_PATH}/velox/dwio/parquet/thrift/libvelox_dwio_parquet_thrift_raw.a)
+endif()
+target_link_libraries(velox PUBLIC external::veloxthrift)
+
+target_link_libraries(
+ velox
+ PRIVATE
+ ${VCPKG_INSTALLED_DIR}/x64-linux-avx/lib/libthriftcpp2.a
+ ${VCPKG_INSTALLED_DIR}/x64-linux-avx/lib/libthriftprotocol.a
+)
Review Comment:
Hardcoding vcpkg paths to a specific triplet (x64-linux-avx) and linking raw
`.a` files makes this build non-portable (e.g., it won’t work for
arm64-linux-neon) and bypasses transitive link/interface settings that CMake
package targets provide. Prefer `find_package(FBThrift CONFIG ...)` and link
against exported targets (e.g., `FBThrift::thriftcpp2`) so the correct triplet
and dependencies are selected automatically.
##########
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 \
Review Comment:
`-DARROW_WITH_THRIFT=ON` was removed, but the script still unconditionally
installs Thrift from Arrow’s build tree (`_build/thrift_ep-prefix/...`). With
Parquet disabled, Arrow typically won’t build the Thrift external project
unless ARROW_WITH_THRIFT is enabled, so this can make the install step fail due
to a missing directory.
##########
dev/vcpkg/ports/fbthrift/portfile.cmake:
##########
@@ -0,0 +1,102 @@
+vcpkg_from_github(
+ OUT_SOURCE_PATH SOURCE_PATH
+ REPO facebook/fbthrift
+ REF "v${VERSION}"
+ SHA512
32f2a648496a321b6aaf55197c2dc1412d030ca82f8d8a5ca0516379379a0f71dc17e2edcb2be3972f76603935c7dcae29769c067caa1ec8a33e7f4efb251581
+ HEAD_REF main
+ PATCHES
+ fix-deps.patch
+ folly-has-liburing.diff
+ fix-fmt-include.patch
+ compactv1-protocol-refiller.patch
+)
Review Comment:
`fix-test.patch` is added under this port directory but isn’t referenced in
the `vcpkg_from_github(PATCHES ...)` list, so it will never be applied. This is
easy to miss and can lead to confusion about which fixes are actually in
effect; either add it to the PATCHES list or remove the unused patch file.
--
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]
