Copilot commented on code in PR #12331:
URL: https://github.com/apache/gluten/pull/12331#discussion_r3503950512
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -89,6 +89,21 @@ function process_setup_tencentos32 {
sed -i "/^[[:space:]]*#/!s/.*dnf config-manager --set-enabled
powertools/#&/" ${CURRENT_DIR}/setup-centos8.sh
}
+# Keep macOS dependency builds on INSTALL_PREFIX even when /usr/local is
present.
+# Apple clang injects /usr/local/include as a normal include path before
CMake's
+# imported system includes, which can mix /usr/local headers with
INSTALL_PREFIX
+# libraries. Demote it to system include order. Folly also enables jemalloc
from
+# Homebrew headers but does not link libjemalloc, so keep Folly's
Linux-equivalent
+# no-jemalloc behavior here; Gluten's own jemalloc build is independent of
this.
+function process_setup_macos {
+ if ! grep -Fq '/usr/local/include' scripts/setup-macos.sh; then
+ sed -i '' 's|OS_CXXFLAGS=" -isystem $(brew --prefix)/include
"|OS_CXXFLAGS=" -isystem $(brew --prefix)/include -isystem /usr/local/include
"|' scripts/setup-macos.sh
+ fi
+ if ! grep -Fq 'FOLLY_USE_JEMALLOC=OFF' scripts/setup-common.sh; then
+ sed -i '' 's/local FOLLY_FLAGS=(/local
FOLLY_FLAGS=(-DFOLLY_USE_JEMALLOC=OFF /' scripts/setup-common.sh
+ fi
+}
Review Comment:
`process_setup_macos` currently demotes `/usr/local/include` unconditionally
on macOS. That changes behavior even when the effective `INSTALL_PREFIX` is
`/usr/local` (or under it), which the rest of the build scripts explicitly
treat as the non-isolated case. Consider gating the `/usr/local/include` sed
patch on `INSTALL_PREFIX` being outside `/usr/local`, consistent with
`build-velox.sh` and `cmake_install`.
##########
cpp/velox/CMakeLists.txt:
##########
@@ -413,16 +413,26 @@ if(DEFINED VCPKG_INSTALLED_DIR
PRIVATE ${VCPKG_INSTALLED_DIR}/${VCPKG_TRIPLET_DIR}/lib/libthriftcpp2.a
${VCPKG_INSTALLED_DIR}/${VCPKG_TRIPLET_DIR}/lib/libthriftprotocol.a)
else()
- message(STATUS "Using system thrift libraries from /usr/local/lib")
+ set(THRIFT_LIBRARY_DIRS)
+ if(CMAKE_INSTALL_PREFIX)
+ list(APPEND THRIFT_LIBRARY_DIRS "${CMAKE_INSTALL_PREFIX}/lib")
+ endif()
+ foreach(PREFIX_PATH IN LISTS CMAKE_PREFIX_PATH)
+ list(APPEND THRIFT_LIBRARY_DIRS "${PREFIX_PATH}/lib" "${PREFIX_PATH}")
+ endforeach()
+ list(APPEND THRIFT_LIBRARY_DIRS /usr/local/lib)
+ list(REMOVE_DUPLICATES THRIFT_LIBRARY_DIRS)
+
+ message(STATUS "Using system thrift libraries from ${THRIFT_LIBRARY_DIRS}")
Review Comment:
This status message is now printed with a list of search directories (which
may include `CMAKE_INSTALL_PREFIX`/`CMAKE_PREFIX_PATH`, not necessarily
“system” locations). Updating the wording makes the log output accurate and
less confusing when debugging prefix resolution.
##########
dev/builddeps-veloxbe.sh:
##########
@@ -260,8 +262,11 @@ function build_gluten_cpp {
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DENABLE_ENHANCED_FEATURES=$ENABLE_ENHANCED_FEATURES"
- if [ $OS == 'Darwin' ]; then
+ if [ -n "${INSTALL_PREFIX:-}" ]; then
GLUTEN_CMAKE_OPTIONS+=" -DCMAKE_PREFIX_PATH=$INSTALL_PREFIX"
+ GLUTEN_CMAKE_OPTIONS+=" -DCMAKE_INSTALL_PREFIX=$INSTALL_PREFIX"
+ fi
Review Comment:
`INSTALL_PREFIX` is concatenated into a whitespace-delimited options string
and then expanded unquoted in the `cmake` invocation, so any prefix containing
spaces will be split into multiple arguments and break configuration. The
helper `cmake_install` already avoids this by passing `-DCMAKE_*_PREFIX` as a
single, quoted argument; this call site should do the same (e.g., via an array)
to make prefix propagation robust.
--
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]
