Copilot commented on code in PR #11712:
URL: https://github.com/apache/gluten/pull/11712#discussion_r3396506665
##########
dev/vcpkg/ports/mvfst/portfile.cmake:
##########
@@ -0,0 +1,21 @@
+vcpkg_from_github(
+ OUT_SOURCE_PATH SOURCE_PATH
+ REPO facebook/mvfst
+ REF "v${VERSION}"
+ SHA512
6669e5b3163f422b3b5b11f298ff16f4f6f196aef765fa4a3da7a4caa69b7675e4ab8a9f9eac0e2f07e5fa30e723af7ab2310d67ec5d1357723da15444333dea
+ HEAD_REF main
+ PATCHES
+ removexsk.patch
+)
+
+vcpkg_cmake_configure(
+ SOURCE_PATH "${SOURCE_PATH}"
+ OPTIONS
+ -DBUILD_TESTS=OFF
+)
+
+vcpkg_cmake_install()
+vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake/mvfst)
+file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/include")
+
+vcpkg_install_copyright(FILE_LIST "${SOURCE_PATH}/LICENSE")
Review Comment:
This portfile appears to have CRLF line endings and a tab-indented
`removexsk.patch` entry. Using LF line endings and consistent space indentation
matches the rest of the repository and avoids patch/formatting issues on Unix
tooling.
##########
dev/ci-velox-buildstatic-centos-7.sh:
##########
@@ -17,6 +17,5 @@
set -e
-export NUM_THREADS=4
./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:
Removing the explicit `NUM_THREADS` cap means `dev/builddeps-veloxbe.sh`
will default to `nproc --ignore=2`, which can significantly increase
parallelism in CI and lead to memory pressure or flaky builds. Other CI scripts
in this repo pin `NUM_THREADS` to a small value (e.g., centos-8/9 scripts use
2).
##########
dev/vcpkg/vcpkg.json:
##########
@@ -24,6 +24,8 @@
"name": "folly",
"features": ["zstd", "lz4", "lzma", "snappy"]
},
+ "fizz",
Review Comment:
The `"fizz"` dependency line is indented with a tab, which is inconsistent
with the surrounding JSON formatting and can cause noisy diffs or formatting
checks to fail.
##########
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
can break unexpectedly if the fork or branch is changed/removed. It also
becomes inconsistent with the enhanced-features defaults:
`--enable_enhanced_features` switches to `VELOX_ENHANCED_BRANCH=ibm-2026_06_06`
but keeps `VELOX_REPO` as-is, so enhanced builds will attempt to fetch an
`ibm-*` branch from the personal fork.
##########
.github/workflows/velox_backend_x86.yml:
##########
@@ -77,7 +77,7 @@ jobs:
export CCACHE_MAXSIZE=1G
mkdir -p /work/.ccache
ccache -sz
- bash dev/ci-velox-buildstatic-centos-7.sh
+ bash dev/ci-velox-buildstatic-centos-9.sh
Review Comment:
The workflow now builds inside a CentOS 9 image, but the saved ccache key
still uses the `centos7` prefix. Updating the key to match the actual build
environment avoids mixing caches between CentOS versions.
##########
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",
+ "dependencies": [
Review Comment:
The `supports` expression uses `|` (OR), which makes the `static` constraint
ineffective on non-Windows triplets: any non-Windows build (including dynamic)
will satisfy `!windows | static`. If the intent is “static-only and
non-Windows”, this should be `!windows & static`.
##########
.github/workflows/velox_backend_x86.yml:
##########
@@ -68,7 +68,7 @@ jobs:
ccache-centos7-release-default
- name: Build Gluten native libraries
run: |
- docker run -v $GITHUB_WORKSPACE:/work -w /work
apache/gluten:vcpkg-centos-7-gcc13 bash -c "
+ docker run -v $GITHUB_WORKSPACE:/work -w /work
apache/gluten:vcpkg-centos-9 bash -c "
Review Comment:
The workflow now builds inside a CentOS 9 image, but the ccache key/restore
prefix still uses `centos7`. This can cause cache pollution across different
build environments (different compilers/libs) and result in hard-to-debug build
failures or reduced cache hit quality.
--
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]
