PHILO-HE commented on code in PR #10650:
URL: 
https://github.com/apache/incubator-gluten/pull/10650#discussion_r2339121547


##########
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"}

Review Comment:
   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.
   



-- 
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]

Reply via email to