Copilot commented on code in PR #12470:
URL: https://github.com/apache/gluten/pull/12470#discussion_r3537007516
##########
dev/build-helper-functions.sh:
##########
@@ -180,12 +180,16 @@ function cmake_install {
local MACOS_ISOLATION_FLAGS=""
if [[ "$(uname)" == "Darwin" ]]; then
- if [[ "${INSTALL_PREFIX:-}" == "/usr/local" || "${INSTALL_PREFIX:-}" ==
/usr/local/* ]]; then
+ if [[ -z "${INSTALL_PREFIX:-}" ]]; then
+ :
+ elif [[ "${INSTALL_PREFIX:-}" == "/usr/local" || "${INSTALL_PREFIX:-}" ==
/usr/local/* ]]; then
echo "INFO: INSTALL_PREFIX=${INSTALL_PREFIX} is under /usr/local;
keeping /usr/local visible to CMake." >&2
else
- COMPILER_FLAGS="$COMPILER_FLAGS -isystem /usr/local/include"
- MACOS_ISOLATION_FLAGS="-DCMAKE_NO_SYSTEM_FROM_IMPORTED=ON \
- -DCMAKE_IGNORE_PREFIX_PATH=/usr/local \
+ # CMake ignore paths only affect package discovery. AppleClang still adds
+ # /usr/local/include to the default header search path unless an SDK
+ # sysroot is selected; SDKROOT (exported by builddeps-veloxbe.sh) handles
+ # the compiler side.
+ MACOS_ISOLATION_FLAGS="-DCMAKE_IGNORE_PREFIX_PATH=/usr/local \
-DCMAKE_IGNORE_PATH=/usr/local;/usr/local/include;/usr/local/lib;/usr/local/lib/cmake
\
-DCMAKE_SYSTEM_IGNORE_PATH=/usr/local;/usr/local/include;/usr/local/lib;/usr/local/lib/cmake"
Review Comment:
On macOS prefix-isolated builds, `cmake_install()` no longer demotes
`/usr/local/include`, and currently relies on callers to have exported
`SDKROOT` elsewhere. If someone runs `dev/build-arrow.sh` (or any other
`cmake_install` consumer) directly with `INSTALL_PREFIX` outside `/usr/local`,
the compiler will still pick up `/usr/local/include` by default, undermining
the intended isolation.
##########
cpp/velox/tests/CMakeLists.txt:
##########
@@ -28,12 +28,16 @@ function(add_velox_test TEST_EXEC)
add_executable(${TEST_EXEC} ${SOURCES} ${VELOX_TEST_COMMON_SRCS})
target_include_directories(${TEST_EXEC} PRIVATE ${CMAKE_SOURCE_DIR}/velox
${CMAKE_SOURCE_DIR}/src)
+ target_compile_definitions(
+ ${TEST_EXEC}
+ PRIVATE GLUTEN_VELOX_TEST_DATA_DIR="${CMAKE_CURRENT_SOURCE_DIR}/data")
target_link_libraries(${TEST_EXEC} velox ${GLUTEN_ROARING_LINK_LIBRARY}
Review Comment:
`FilePathGenerator.cc` now requires `GLUTEN_VELOX_TEST_DATA_DIR`, but that
definition is only added in `add_velox_test()`. `add_velox_gpu_test()` also
compiles `VELOX_TEST_COMMON_SRCS` (which includes `FilePathGenerator.cc`) and
does not define this macro, so ENABLE_GPU builds will fail to compile with an
undefined identifier error.
##########
ep/build-velox/src/build-velox.sh:
##########
@@ -114,8 +114,10 @@ function compile {
-Wno-error=uninitialized -Wno-unknown-warning-option
-Wno-deprecated-declarations'
if [[ "$(uname)" == "Darwin" ]]; then
CXX_FLAGS="$CXX_FLAGS -Wno-inconsistent-missing-override
-Wno-macro-redefined"
- if [[ "${INSTALL_PREFIX:-}" != "/usr/local" && "${INSTALL_PREFIX:-}" !=
/usr/local/* ]]; then
- CXX_FLAGS="$CXX_FLAGS -isystem /usr/local/include"
+ if [[ -n "${INSTALL_PREFIX:-}" && "${INSTALL_PREFIX:-}" != "/usr/local" &&
"${INSTALL_PREFIX:-}" != /usr/local/* ]]; then
+ # Some prefix-installed deps only publish loose headers (e.g. xsimd), so
+ # keep the prefix include dir on the compiler command line.
+ CXX_FLAGS="$CXX_FLAGS -I${INSTALL_PREFIX}/include"
fi
Review Comment:
`-I${INSTALL_PREFIX}/include` is appended without quoting, so an
`INSTALL_PREFIX` containing spaces (or other shell-special characters) will
produce an invalid compiler flag and break the 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]
