[VL] Support test/debug/benchmark native builds on macOS (follow-up to #12331)#12470
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up to #12331 to make Velox/Gluten native builds on macOS behave correctly for Debug builds and when enabling tests/benchmarks, mainly by tightening header discovery/isolation, preserving requested build type across dependency builds, and improving test execution/linking behavior on Darwin.
Changes:
- Switch macOS header isolation from
/usr/local/includedemotion to using an SDK sysroot (SDKROOT), and keep prefix include dirs available when needed. - Fix build-type propagation across Arrow build scripting and adjust Velox/Gluten linking on macOS to avoid duplicate archive linkage at test runtime.
- Improve C++ test robustness (compile-time test data dir, longer gtest discovery timeout) and restrict
ld/lddpost-build checks to Linux.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ep/build-velox/src/get-velox.sh | Removes macOS /usr/local/include demotion patching and updates Folly jemalloc-related setup notes. |
| ep/build-velox/src/build-velox.sh | Adds SDKROOT-based isolation on macOS, adjusts include flags for prefix installs, and hardens xsimd install handling. |
| dev/builddeps-veloxbe.sh | Exports SDKROOT for macOS prefix-isolated builds and restores BUILD_TYPE after sourcing build-arrow.sh. |
| dev/build-helper-functions.sh | Updates macOS isolation logic in cmake_install() to rely on sysroot-based isolation rather than include-path demotion. |
| cpp/velox/tests/FilePathGenerator.cc | Resolves test data paths via a compile-time source-dir macro instead of working-directory-relative paths. |
| cpp/velox/tests/CMakeLists.txt | Defines the test data dir macro for tests and increases gtest discovery timeout. |
| cpp/velox/CMakeLists.txt | Adds macOS force-load of Velox mono archive into the dylib, adjusts link scope on Darwin, patches Folly↔gflags linkage, and gates ld/ldd checks to Linux. |
| cpp/core/CMakeLists.txt | Adds optional prefix include directories and gates ld/ldd checks to Linux. |
| cpp/CMakeLists.txt | Collects prefix include dirs from CMAKE_INSTALL_PREFIX/CMAKE_PREFIX_PATH for consistent header discovery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+117
to
121
| 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 |
Contributor
Author
There was a problem hiding this 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.
…apache#12331) PR apache#12331 (ce411ad) made INSTALL_PREFIX flow into the Arrow, Velox and Gluten CPP CMake entry points. This follow-up keeps that behavior unchanged and fixes what is still broken when building with --build_type=Debug, --build_tests=ON or --build_benchmarks=ON on macOS: - Replace the "-isystem /usr/local/include" demotion with an SDK sysroot (SDKROOT). Demotion keeps /usr/local/include in the search path ahead of imported targets' system includes, so test builds can compile against /usr/local headers (folly, gtest) while linking INSTALL_PREFIX libraries. Selecting the SDK sysroot removes /usr/local/include from the default search path entirely; package discovery isolation via CMAKE_IGNORE_* is unchanged. - Restore BUILD_TYPE after sourcing build-arrow.sh, which overwrote Debug with Release for the rest of the build. - Force-load the Velox mono archive into libvelox.dylib and link facebook::velox privately on macOS so test executables resolve Velox symbols from the dylib instead of linking a second copy of the archive. - Rewrite folly's imported interface to use gflags_shared on macOS. Folly exports gflags_static while glog links the shared gflags, and loading both copies makes gflags register built-in flags twice and abort test binaries. Done in Gluten's CMake at find_package(Folly) time instead of patching installed folly-targets.cmake. - Resolve test data files from a compile-time source-dir macro instead of the working directory, and bump gtest discovery timeout; run the ld/ldd post-build checks only on Linux. Linux discovery logic and the Folly no-jemalloc setup from apache#12331 are unchanged.
jackylee-ch
force-pushed
the
native-build-install-prefix
branch
from
3c15656 to
19231b6
Compare
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Follow-up to #12331, which made
INSTALL_PREFIXflow into the Arrow, Velox and Gluten CPP CMake entry points. That behavior is unchanged here. This PR fixes what is still broken when building on macOS with--build_type=Debug,--build_tests=ONor--build_benchmarks=ON:-isystem /usr/local/includedemotion with an SDK sysroot (SDKROOT). Demotion keeps/usr/local/includeahead of imported targets' system includes, so test builds can compile against/usr/localheaders (folly, gtest) while linkingINSTALL_PREFIXlibraries. Selecting the SDK sysroot drops/usr/local/includefrom the default search path entirely; package-discovery isolation viaCMAKE_IGNORE_*is unchanged.BUILD_TYPEafter sourcingbuild-arrow.sh, which otherwise overwroteDebugwithReleasefor the rest of the build.libvelox.dyliband linkfacebook::veloxprivately, so test executables resolve Velox symbols from the dylib instead of linking a second copy of the archive.gflags_shared. Folly exportsgflags_staticwhile glog links the shared gflags; loading both makes gflags register its built-in flags twice and abort test binaries. This is done in Gluten's CMake atfind_package(Folly)time rather than by patching the installedfolly-targets.cmake.ld/lddpost-build checks only on Linux.Linux discovery logic and the Folly no-jemalloc setup from #12331 are unchanged.
How was this patch tested?
macOS arm64 local Debug build with tests and benchmarks enabled; the full C++
ctestsuite passed (5663 tests, 0 failures).Was this patch authored or co-authored using generative AI tooling?
Yes — drafted with AI assistance and reviewed and verified by the author.