Copilot commented on code in PR #12321:
URL: https://github.com/apache/gluten/pull/12321#discussion_r3616638559
##########
ep/build-velox/src/setup-openeuler24.sh:
##########
@@ -110,11 +111,23 @@ function install_boost {
)
}
+function install_xxhash {
+ wget_and_untar
https://github.com/Cyan4973/xxHash/archive/refs/tags/v0.8.2.tar.gz xxHash
+ cd ${DEPENDENCY_DIR}/xxHash
+ make "-j${NPROC}"
+ make install PREFIX=${INSTALL_PREFIX}
Review Comment:
`install_xxhash` changes the current working directory (`cd
${DEPENDENCY_DIR}/xxHash`) without restoring it, which can leak state into
subsequent install functions. Wrap the build/install steps in a subshell (as
done in other install_* functions) to keep directory changes local.
##########
ep/build-velox/src/setup-openeuler24.sh:
##########
@@ -36,6 +36,7 @@ NPROC=$(getconf _NPROCESSORS_ONLN)
export CFLAGS=$(get_cxx_flags $CPU_TARGET) # Used by LZO.
export CXXFLAGS=$CFLAGS # Used by boost.
export CPPFLAGS=$CFLAGS # Used by LZO.
+EXTRA_PKG_CXXFLAGS=" -isystem ${INSTALL_PREFIX}/include"
Review Comment:
`EXTRA_PKG_CXXFLAGS` is computed before `INSTALL_PREFIX` is
defaulted/exported, so when `INSTALL_PREFIX` isn’t set in the environment this
expands to an empty prefix (" -isystem /include"). Move the assignment to after
`export INSTALL_PREFIX=...` (or compute it from the same default) so it always
points at the real install prefix.
##########
ep/build-velox/src/setup-centos8.sh:
##########
@@ -228,7 +232,7 @@ function install_velox_deps {
(
if [[ $# -ne 0 ]]; then
# Activate gcc11; enable errors on unset variables afterwards.
Review Comment:
This comment still says “Activate gcc11” but the script now sources
`gcc-toolset-12`. Please update the comment to match the actual toolset version
to avoid confusion when debugging build environments.
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -65,6 +65,7 @@ if [ "$VELOX_HOME" == "" ]; then
fi
function process_setup_ubuntu {
+ sed -i "s|run_and_time install_arrow||g" scripts/setup-centos9.sh
echo "Using setup script from Velox"
}
Review Comment:
`process_setup_ubuntu` currently edits `scripts/setup-centos9.sh`, which
isn’t the setup script invoked on Ubuntu (Linux setup uses
`scripts/setup-ubuntu.sh`). Also, `process_setup_alinux3` still tries to patch
out `gcc-toolset-11` from `setup-centos8.sh`, but `setup-centos8.sh` now uses
`gcc-toolset-12`, so those sed patterns won’t match and the ALink3 workaround
won’t apply.
##########
dev/build-thirdparty.sh:
##########
@@ -32,13 +32,16 @@ function process_setup_ubuntu_2004 {
function process_setup_ubuntu_2204 {
cp
/usr/lib/${ARCH}-linux-gnu/{libre2.so.9,libdouble-conversion.so.3,libglog.so.0,libgflags.so.2.2,libevent-2.1.so.7,libsnappy.so.1,libunwind.so.8,libcurl.so.4,libxml2.so.2,libicui18n.so.70,libicuuc.so.70,libnghttp2.so.14,libldap-2.5.so.0,liblber-2.5.so.0,librtmp.so.1,libsasl2.so.2,libssh.so.4,libicudata.so.70}
$THIRDPARTY_LIB/
- cp /usr/local/lib/{libboost_context.so.1.84.0,libboost_regex.so.1.84.0}
$THIRDPARTY_LIB/
+ find /usr/ -name "libboost_context.so.1.84.0" -exec cp {} $THIRDPARTY_LIB/
\; 2>/dev/null
+ find /usr/ -name "libboost_regex.so.1.84.0" -exec cp {} $THIRDPARTY_LIB/ \;
2>/dev/null
}
function process_setup_centos_9 {
cp
/lib64/{libre2.so.9,libdouble-conversion.so.3,libevent-2.1.so.7,libdwarf.so.0,libicudata.so.67,libicui18n.so.67,libicuuc.so.67,libsodium.so.23}
$THIRDPARTY_LIB/
cp
/usr/local/lib/{libboost_context.so.1.84.0,libboost_filesystem.so.1.84.0,libboost_program_options.so.1.84.0,libboost_regex.so.1.84.0,libboost_system.so.1.84.0,libboost_thread.so.1.84.0,libboost_atomic.so.1.84.0}
$THIRDPARTY_LIB/
- cp /usr/local/lib64/{libgflags.so.2.2,libglog.so.1,libgeos.so.3.10.7}
$THIRDPARTY_LIB/
+ cp /usr/local/lib/libgflags.so.2.2 $THIRDPARTY_LIB/
Review Comment:
`process_setup_centos_9` now hard-codes `libgflags.so.2.2` under
`/usr/local/lib`, but on RHEL-family systems the install often lands in
`/usr/local/lib64` (and this script already uses lib64 for CentOS 8 /
openEuler). Use the same `find /usr/local/lib /usr/local/lib64 ...` approach as
glog/geos to avoid missing gflags at packaging time.
##########
.github/workflows/velox_weekly.yml:
##########
@@ -125,35 +84,40 @@ jobs:
# sed -i 's|http://archive|http://us.archive|g' /etc/apt/sources.list
# To avoid the prompt for region selection during installing tzdata.
export DEBIAN_FRONTEND=noninteractive
- apt-get update && apt-get install -y sudo maven wget git curl
- if [ "${{ matrix.os }}" = "ubuntu:20.04" ]; then
- sudo apt install -y software-properties-common
- sudo add-apt-repository ppa:ubuntu-toolchain-r/test
- sudo apt update && sudo apt install -y gcc-11 g++-11
build-essential cmake
- export CC=/usr/bin/gcc-11
- export CXX=/usr/bin/g++-11
- wget
https://github.com/Cyan4973/xxHash/archive/refs/tags/v0.8.1.tar.gz
- tar xf v0.8.1.tar.gz
- cd xxHash-0.8.1/
- make && make install
- cd /tmp
- wget
https://github.com/google/googletest/archive/refs/tags/v1.13.0.tar.gz
- tar xf v1.13.0.tar.gz && cd googletest-1.13.0/
- export CXXFLAGS=-fPIC
- cmake . && make install -j
- apt autoremove -y
- rm -rf /tmp/*
- fi
- sudo apt-get install -y openjdk-8-jdk python3-pip cmake
+ export INSTALL_PREFIX=/usr/
+ apt-get update && apt-get install -y sudo maven wget git curl gcc-11
g++-11 build-essential cmake openjdk-8-jdk python3-pip
+ pip3 install cmake==3.31.4
+ cd /tmp
+ wget https://github.com/gflags/gflags/archive/v2.2.2.tar.gz -O
gflags.tar.gz
+ tar -xzf gflags.tar.gz && cd gflags-2.2.2
+ cmake -B build \
+ -DCMAKE_POSITION_INDEPENDENT_CODE=ON \
+ -DBUILD_SHARED_LIBS=ON \
+ -DBUILD_STATIC_LIBS=ON \
+ -DBUILD_gflags_LIB=ON \
+ -DCMAKE_INSTALL_PREFIX=/usr/local \
+ -DCMAKE_BUILD_TYPE=Release
+ cmake --build build -j4 --target install
+ wget
https://github.com/Cyan4973/xxHash/archive/refs/tags/v0.8.1.tar.gz
+ tar xf v0.8.1.tar.gz
+ cd xxHash-0.8.1/
+ make && make install
+ cd /tmp
+ wget
https://github.com/google/googletest/archive/refs/tags/v1.13.0.tar.gz
+ tar xf v1.13.0.tar.gz && cd googletest-1.13.0/
+ export CXXFLAGS=-fPIC
+ cmake . && make install -j
+ apt autoremove -y
export JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64
+ export VELOX_BUILD_SHARED=ON
cd $GITHUB_WORKSPACE/ && ./dev/package.sh --spark_version=3.5
- build-on-openeuler:
+ weekly-build-on-openeuler:
strategy:
fail-fast: false
matrix:
os: [ "openeuler/openeuler:24.03" ]
Review Comment:
This workflow removed the `if: ${{ startsWith(github.repository, 'apache/')
}}` guard that is used across other scheduled workflows (e.g.
`.github/workflows/velox_nightly.yml:37`). Without it, forks that enable
Actions may start running the weekly builds as well. If that’s not intended,
please restore the guard at the job level.
##########
.github/workflows/velox_weekly.yml:
##########
@@ -35,87 +35,46 @@ concurrency:
cancel-in-progress: true
jobs:
- build-on-centos-7:
- strategy:
- fail-fast: false
- if: ${{ startsWith(github.repository, 'apache/') }}
- runs-on: ubuntu-22.04
- container: centos:7
- steps:
- - name: Build Gluten native libraries and package
- run: |
- sed -i -e 's|mirrorlist=|#mirrorlist=|g' /etc/yum.repos.d/CentOS-*
|| true
- sed -i -e
's|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g'
/etc/yum.repos.d/CentOS-* || true
- yum update -y
- yum install -y epel-release sudo dnf
- yum install -y centos-release-scl
- rm /etc/yum.repos.d/CentOS-SCLo-scl.repo -f
- sed -i \
- -e 's/^mirrorlist/#mirrorlist/' \
- -e 's/^#baseurl/baseurl/' \
- -e 's/mirror\.centos\.org/vault.centos.org/' \
- /etc/yum.repos.d/CentOS-SCLo-scl-rh.repo
- yum install -y devtoolset-11
- source /opt/rh/devtoolset-11/enable
- yum install -y java-1.8.0-openjdk-devel patch wget git perl
python3-pip
- export JAVA_HOME=/usr/lib/jvm/java-1.8.0-openjdk && \
- export PATH=$JAVA_HOME/bin:$PATH
-
- # action/checkout does not work centos7 anymore, so we clone the
branch instead.
- git clone -b main --depth=1 https://github.com/apache/gluten.git &&
cd gluten/
- if [ ${{ github.event_name }} = "pull_request" ]; then
- git fetch origin ${{ github.ref }}:pr_branch && git checkout
pr_branch
- fi
- ./dev/package.sh --spark_version=3.5
- build-on-centos:
+ weekly-build-on-centos:
strategy:
fail-fast: false
matrix:
os: [ "centos:8", "quay.io/centos/centos:stream9" ]
- if: ${{ startsWith(github.repository, 'apache/') }}
+
runs-on: ubuntu-22.04
container: ${{ matrix.os }}
steps:
- name: Checkout repository
uses: actions/checkout@v4
- - name: Update mirror list
- run: |
- sed -i -e "s|mirrorlist=|#mirrorlist=|g" /etc/yum.repos.d/CentOS-*
|| true
- sed -i -e
"s|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g"
/etc/yum.repos.d/CentOS-* || true
- name: build
run: |
- yum update -y
- yum install -y epel-release sudo dnf
- if [ "${{ matrix.os }}" = "centos:7" ]; then
- yum install -y centos-release-scl
- rm /etc/yum.repos.d/CentOS-SCLo-scl.repo -f
- sed -i \
- -e 's/^mirrorlist/#mirrorlist/' \
- -e 's/^#baseurl/baseurl/' \
- -e 's/mirror\.centos\.org/vault.centos.org/' \
- /etc/yum.repos.d/CentOS-SCLo-scl-rh.repo
- yum install -y devtoolset-11
- source /opt/rh/devtoolset-11/enable
- elif [ "${{ matrix.os }}" = "quay.io/centos/centos:stream9" ]; then
- dnf install -y --setopt=install_weak_deps=False gcc-toolset-12
- source /opt/rh/gcc-toolset-12/enable || exit 1
- else
- dnf install -y --setopt=install_weak_deps=False gcc-toolset-11
- source /opt/rh/gcc-toolset-11/enable || exit 1
+ if [ "${{ matrix.os }}" = "centos:8" ]; then
+ sed -i -e "s|mirrorlist=|#mirrorlist=|g" /etc/yum.repos.d/CentOS-*
|| true
+ sed -i -e
"s|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g"
/etc/yum.repos.d/CentOS-* || true
+ dnf update -y
+ dnf install -y centos-release-stream
+ sed -i -e "s|mirrorlist=|#mirrorlist=|g"
/etc/yum.repos.d/CentOS-Stream-* || true
+ sed -i -e
"s|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g"
/etc/yum.repos.d/CentOS-Stream-* || true
+ dnf swap -y --allowerasing centos-{linux,stream}-repos
+ sed -i -e "s|mirrorlist=|#mirrorlist=|g"
/etc/yum.repos.d/CentOS-Stream-* || true
+ sed -i -e
"s|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g"
/etc/yum.repos.d/CentOS-Stream-* || true
fi
+ dnf install -y epel-release sudo dnf
+ dnf install -y --setopt=install_weak_deps=False gcc-toolset-12
+ source /opt/rh/gcc-toolset-12/enable || exit 1
yum install -y java-1.8.0-openjdk-devel patch wget git perl
export JAVA_HOME=/usr/lib/jvm/java-1.8.0-openjdk && \
export PATH=$JAVA_HOME/bin:$PATH
-
+ export VELOX_BUILD_SHARED=ON
cd $GITHUB_WORKSPACE/ && ./dev/package.sh --spark_version=3.5
- build-on-ubuntu:
+ weekly-build-on-ubuntu:
strategy:
fail-fast: false
matrix:
- os: [ "ubuntu:22.04" ] # skip "ubuntu:20.04" due to not enough disk
space
- if: ${{ startsWith(github.repository, 'apache/') }}
+ os: [ "ubuntu:22.04" ]
Review Comment:
This workflow removed the `if: ${{ startsWith(github.repository, 'apache/')
}}` guard that is used across other scheduled workflows (e.g.
`.github/workflows/velox_nightly.yml:37`). Without it, forks that enable
Actions may start running the weekly builds as well. If that’s not intended,
please restore the guard at the job level.
##########
.github/workflows/velox_weekly.yml:
##########
@@ -177,12 +141,12 @@ jobs:
$TPCH_TEST --extra-conf=spark.gluten.loadLibFromJar=true \
--extra-conf=spark.gluten.loadLibOS=openEuler
--extra-conf=spark.gluten.loadLibOSVersion=24.03
- build-on-openeuler-vcpkg:
+ weekly-build-on-openeuler-vcpkg:
strategy:
fail-fast: false
matrix:
os: [ "openeuler/openeuler:24.03" ]
Review Comment:
This workflow removed the `if: ${{ startsWith(github.repository, 'apache/')
}}` guard that is used across other scheduled workflows (e.g.
`.github/workflows/velox_nightly.yml:37`). Without it, forks that enable
Actions may start running the weekly builds as well. If that’s not intended,
please restore the guard at the job level.
##########
ep/build-velox/src/setup-centos8.sh:
##########
@@ -242,7 +246,7 @@ function install_velox_deps {
echo "Skipping installation of build dependencies since
INSTALL_PREREQUISITES is not set"
fi
# Activate gcc11; enable errors on unset variables afterwards.
Review Comment:
This comment still says “Activate gcc11” but the script now sources
`gcc-toolset-12`. Please update the comment to match the actual toolset version
to avoid confusion when debugging build environments.
##########
.github/workflows/velox_weekly.yml:
##########
@@ -35,87 +35,46 @@ concurrency:
cancel-in-progress: true
jobs:
- build-on-centos-7:
- strategy:
- fail-fast: false
- if: ${{ startsWith(github.repository, 'apache/') }}
- runs-on: ubuntu-22.04
- container: centos:7
- steps:
- - name: Build Gluten native libraries and package
- run: |
- sed -i -e 's|mirrorlist=|#mirrorlist=|g' /etc/yum.repos.d/CentOS-*
|| true
- sed -i -e
's|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g'
/etc/yum.repos.d/CentOS-* || true
- yum update -y
- yum install -y epel-release sudo dnf
- yum install -y centos-release-scl
- rm /etc/yum.repos.d/CentOS-SCLo-scl.repo -f
- sed -i \
- -e 's/^mirrorlist/#mirrorlist/' \
- -e 's/^#baseurl/baseurl/' \
- -e 's/mirror\.centos\.org/vault.centos.org/' \
- /etc/yum.repos.d/CentOS-SCLo-scl-rh.repo
- yum install -y devtoolset-11
- source /opt/rh/devtoolset-11/enable
- yum install -y java-1.8.0-openjdk-devel patch wget git perl
python3-pip
- export JAVA_HOME=/usr/lib/jvm/java-1.8.0-openjdk && \
- export PATH=$JAVA_HOME/bin:$PATH
-
- # action/checkout does not work centos7 anymore, so we clone the
branch instead.
- git clone -b main --depth=1 https://github.com/apache/gluten.git &&
cd gluten/
- if [ ${{ github.event_name }} = "pull_request" ]; then
- git fetch origin ${{ github.ref }}:pr_branch && git checkout
pr_branch
- fi
- ./dev/package.sh --spark_version=3.5
- build-on-centos:
+ weekly-build-on-centos:
strategy:
fail-fast: false
matrix:
os: [ "centos:8", "quay.io/centos/centos:stream9" ]
Review Comment:
This workflow removed the `if: ${{ startsWith(github.repository, 'apache/')
}}` guard that is used across other scheduled workflows (e.g.
`.github/workflows/velox_nightly.yml:37`). Without it, forks that enable
Actions may start running the weekly builds as well. If that’s not intended,
please restore the guard at the job level.
##########
dev/build-thirdparty.sh:
##########
@@ -32,13 +32,16 @@ function process_setup_ubuntu_2004 {
function process_setup_ubuntu_2204 {
cp
/usr/lib/${ARCH}-linux-gnu/{libre2.so.9,libdouble-conversion.so.3,libglog.so.0,libgflags.so.2.2,libevent-2.1.so.7,libsnappy.so.1,libunwind.so.8,libcurl.so.4,libxml2.so.2,libicui18n.so.70,libicuuc.so.70,libnghttp2.so.14,libldap-2.5.so.0,liblber-2.5.so.0,librtmp.so.1,libsasl2.so.2,libssh.so.4,libicudata.so.70}
$THIRDPARTY_LIB/
- cp /usr/local/lib/{libboost_context.so.1.84.0,libboost_regex.so.1.84.0}
$THIRDPARTY_LIB/
+ find /usr/ -name "libboost_context.so.1.84.0" -exec cp {} $THIRDPARTY_LIB/
\; 2>/dev/null
+ find /usr/ -name "libboost_regex.so.1.84.0" -exec cp {} $THIRDPARTY_LIB/ \;
2>/dev/null
Review Comment:
These `find /usr/ ...` scans traverse the entire filesystem tree under
`/usr`, which can be very slow in CI images and makes packaging time
unpredictable. Limit the search to likely library roots (e.g. `/usr/local` and
`/usr/lib/${ARCH}-linux-gnu`) and add a max depth/type filter.
##########
.github/workflows/velox_weekly.yml:
##########
@@ -205,10 +169,10 @@ jobs:
cd $GITHUB_WORKSPACE/tools/gluten-it
$TPCH_TEST
- build-on-macos:
+ weekly-build-on-macos:
strategy:
fail-fast: false
Review Comment:
This workflow removed the `if: ${{ startsWith(github.repository, 'apache/')
}}` guard that is used across other scheduled workflows (e.g.
`.github/workflows/velox_nightly.yml:37`). Without it, forks that enable
Actions may start running the weekly builds as well. If that’s not intended,
please restore the guard at the job level.
--
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]
