jackylee-ch commented on code in PR #12470:
URL: https://github.com/apache/gluten/pull/12470#discussion_r3541811787
##########
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:
Fixed. `cmake_install()` now exports `SDKROOT` itself in the macOS
non-`/usr/local` branch, so it stays self-contained when invoked directly (e.g.
via `dev/build-arrow.sh`) instead of relying on a caller to have exported it.
##########
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:
Leaving this as-is intentionally. `${INSTALL_PREFIX}` is used unquoted
throughout this script (the very next lines pass
`-DCMAKE_PREFIX_PATH=${INSTALL_PREFIX}` and
`-DCMAKE_INSTALL_PREFIX=${INSTALL_PREFIX}` the same way), so paths with spaces
are already unsupported here regardless of this line. Since the flag is later
embedded into `-DCMAKE_CXX_FLAGS="..."` and handed to `make`, quoting just this
one occurrence would not actually make space-containing prefixes work. Proper
whitespace support would be a separate, script-wide change outside the scope of
this test/debug/benchmark follow-up.
--
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]
