[VL] Respect INSTALL_PREFIX when building arrow

[Zouxxyy]

What changes are proposed in this pull request?

The path /usr/local is not good for MacOS to install dependencies like arrow libs. With this PR, arrow installation will respect the setting for INSTALL_PREFIX, which defaults to ${VELOX_HOME}/deps-install if not explicitly set.

How was this patch tested?

Local test.

[Zouxxyy]

Not sure what's wrong with this modification, maybe we need to rebuild the image

[philo-he]

In the docker, arrow libs have already been installed into /usr/local/. In every PR validation, we expect the installed arrow libs are used and arrow will not be re-built. With this PR, we still expect the installed arrow libs are found by Velox. But the CI shows Velox cannot find the libs for some reason, then tries to build Arrow, during which the failure happens.

I suspect it's caused by the changes made in dev/builddeps-veloxbe.sh.

[jackylee-ch]

You can try setting ARROW_HOME or INSTALL_PREFIX to CMAKE_PREFIX_PATH when building Velox, similar to the change for gluten cpp build.

[Zouxxyy]

@jackylee-ch @philo-he Thanks for the suggestion, can you confirm if this matches what we want

[philo-he]

Hi @Zouxxyy, thanks for your work.

With the patch below, my local tests show that the build passes successfully in the same Docker environment used by CI. This patch might be similar to the changes you initially proposed. I'm not entirely sure why the CI build failed earlier.

I prefer directly setting the environment variable instead of parsing script options. I noticed that Velox automatically uses CMAKE_PREFIX_PATH based on the environment variable, see code here. Therefore, it doesn't seem necessary to explicitly set it in build_velox.sh.

diff --git a/dev/build_arrow.sh b/dev/build_arrow.sh
index 271c1d90c..a54f0a94d 100755
--- a/dev/build_arrow.sh
+++ b/dev/build_arrow.sh
@@ -23,6 +23,7 @@ source ${CURRENT_DIR}/build_helper_functions.sh
 VELOX_ARROW_BUILD_VERSION=15.0.0
 ARROW_PREFIX=$CURRENT_DIR/../ep/_ep/arrow_ep
 BUILD_TYPE=Release
+INSTALL_PREFIX=${INSTALL_PREFIX:-"/usr/local"}
 
 function prepare_arrow_build() {
   mkdir -p ${ARROW_PREFIX}/../ && pushd ${ARROW_PREFIX}/../ && ${SUDO} rm -rf arrow_ep/
@@ -62,14 +63,14 @@ function build_arrow_cpp() {
        -DARROW_RUNTIME_SIMD_LEVEL=NONE \
        -DARROW_WITH_UTF8PROC=OFF \
        -DARROW_TESTING=ON \
-       -DCMAKE_INSTALL_PREFIX=/usr/local \
+       -DCMAKE_INSTALL_PREFIX=${INSTALL_PREFIX} \
        -DCMAKE_BUILD_TYPE=${BUILD_TYPE} \
        -DARROW_BUILD_SHARED=OFF \
        -DARROW_BUILD_STATIC=ON
 
  # Install thrift.
  cd _build/thrift_ep-prefix/src/thrift_ep-build
- ${SUDO} cmake --install ./ --prefix /usr/local/
+ ${SUDO} cmake --install ./ --prefix ${INSTALL_PREFIX}
  popd
 }
 
@@ -103,7 +104,7 @@ function build_arrow_java() {
       -Dmaven.test.skip -Drat.skip -Dmaven.gitcommitid.skip -Dcheckstyle.skip -N
 
     # Arrow JNI Date Interface CPP libraries
-    export PKG_CONFIG_PATH=/usr/local/lib64/pkgconfig${PKG_CONFIG_PATH:+:${PKG_CONFIG_PATH}}
+    export PKG_CONFIG_PATH=${INSTALL_PREFIX}/lib64/pkgconfig${PKG_CONFIG_PATH:+:${PKG_CONFIG_PATH}}
     mvn generate-resources -Pgenerate-libs-jni-macos-linux -N -Darrow.dataset.jni.dist.dir=$ARROW_INSTALL_DIR \
       -DARROW_GANDIVA=OFF -DARROW_JAVA_JNI_ENABLE_GANDIVA=OFF -DARROW_ORC=OFF -DARROW_JAVA_JNI_ENABLE_ORC=OFF \
 	    -Dmaven.test.skip -Drat.skip -Dmaven.gitcommitid.skip -Dcheckstyle.skip -N
diff --git a/dev/builddeps-veloxbe.sh b/dev/builddeps-veloxbe.sh
index 0eb1a0b2e..34d961e17 100755
--- a/dev/builddeps-veloxbe.sh
+++ b/dev/builddeps-veloxbe.sh
@@ -159,9 +159,9 @@ do
 done
 
 if [[ "$(uname)" == "Darwin" ]]; then
-    INSTALL_PREFIX=${INSTALL_PREFIX:-${VELOX_HOME}/deps-install}
+    export INSTALL_PREFIX=${INSTALL_PREFIX:-${VELOX_HOME}/deps-install}
 else
-    INSTALL_PREFIX=${INSTALL_PREFIX:-"/usr/local"}
+    export INSTALL_PREFIX=${INSTALL_PREFIX:-"/usr/local"}
 fi
 
 function concat_velox_param {

[Zouxxyy]

Sure, I'll roll back to the previous solution and investigate why it failed. I remember only a few CI jobs failed.

[philo-he]

With this line, if INSTALL_PREFIX is not explicitly set in env, Velox will use "/user/local" to set CMAKE_PREFIX_PATH, which is unnecessary since "/user/local" is already CMake's default searching path. This setting may somehow cause failures in finding the installed thrift and arrow on Centos-8, as shown in CI.

I think we can remove line 164 and only keep setting the env variable INSTALL_PREFIX for MacOS, since a non-default path is used.

If users explicitly set env variable INSTALL_PREFIX regardless of MacOS or others, I assume their setting is respected by build_arrow.sh and Velox build.

[Zouxxyy]

Thanks, I see, we only need to provide INSTALL_PREFIX for Mac

[philo-he]

Looks good. Thanks. The CI failure appears unrelated.

[philo-he]

@jackylee-ch, do you have any comment on this PR? Thanks.

[philo-he]

Let's merge this PR first. If there is any other comment, let's fix it in follow-up PR. Thanks.