build: support native macOS (Darwin) build (NGT, Faiss, cgo, make test) - #3573
build: support native macOS (Darwin) build (NGT, Faiss, cgo, make test)#3573aaf2tbz wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds native macOS build support through OS-aware Makefile configuration, dependency installation changes, development documentation, and guarded ChangesNative macOS support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I don't have permission to add labels to this PR (external contributor), and the repo's
(The same applies to the linked issues #3570 and #3571 — currently unlabeled.) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bd7e356f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contributing/development.md`:
- Around line 48-55: Update the optional LLVM PATH instruction in the
development documentation to use Homebrew’s dynamic prefix via brew --prefix
llvm, rather than the hard-coded /opt/homebrew/opt/llvm/bin path. Preserve the
existing guidance to install llvm only when explicitly needed.
In `@Makefile`:
- Line 903: Replace the ldconfig expression at Makefile lines 903-903 and
929-929 with an if command -v ldconfig check that runs ldconfig only when
available and propagates failures. In Makefile.d/tools.mk lines 393-393,
preserve the surrounding recipe’s && chain while using the same conditional
ldconfig body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 270a525b-c8b6-46b1-a56c-4b933dd1fc3d
📒 Files selected for processing (5)
.gitignoreMakefileMakefile.d/tools.mkdocs/contributing/development.mdinternal/core/algorithm/faiss/Capi.cpp
|
Following Up on this PR until Reviewer Bots are Satisfied. |
5231db7 to
0d80115
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Makefile.d/functions.mk (1)
135-150: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard
-staticon Darwin
go-e2e-buildalways passes-extldflags '-static'. macOS native builds are supported here, somake e2ewill fail on Darwin unless this is gated onGOOSand falls back to non-static linking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile.d/functions.mk` around lines 135 - 150, Update the go-e2e-build command’s linker flags around -ldflags so -extldflags '-static' is applied only when GOOS is not Darwin. For Darwin builds, omit the static linker option and retain the existing non-static Go test/build arguments.Makefile (1)
225-225: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid
brewlookups in thesudo makepath.
Makefile#L173-L225evaluates$(shell brew --prefix ...)during parsing, so the documentedsudo make ngt/install hdf5/install faiss/installflow runs Homebrew as root and can fail on macOS.
Makefile#L173-L225: make the Homebrew prefixes overridable or inject them before privilege escalation.docs/contributing/development.md#L41-L42: split the privileged install step from the unprivileged build/configure step instead of recommending a singlesudo make.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 225, Make the Homebrew prefix variables used by the Makefile build/install targets overridable or pass them into the environment before any sudo escalation, so parsing the Makefile never runs brew as root; update Makefile:225 and the related logic in Makefile:173-225. In docs/contributing/development.md:41-42, replace the single sudo make workflow with separate privileged dependency-installation and unprivileged build/configure steps.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 172-173: Update the HDF5 linker flag definitions in HDF5_LDFLAGS
and CGO_LDFLAGS so Darwin builds do not include the Linux-specific -ldl flag;
retain dynamic-loader linking only for non-Darwin targets or use the appropriate
platform-specific library.
In `@Makefile.d/functions.mk`:
- Around line 64-67: Update the cgo flag assignments in all three affected macro
blocks to preserve the exported OpenMP flags, composing CGO_CFLAGS and
CGO_CXXFLAGS with their existing values or explicitly including OPENMP_CFLAGS;
apply the same OpenMP inclusion consistently across the related C, C++, and
linker flag definitions without changing unrelated flags.
---
Outside diff comments:
In `@Makefile`:
- Line 225: Make the Homebrew prefix variables used by the Makefile
build/install targets overridable or pass them into the environment before any
sudo escalation, so parsing the Makefile never runs brew as root; update
Makefile:225 and the related logic in Makefile:173-225. In
docs/contributing/development.md:41-42, replace the single sudo make workflow
with separate privileged dependency-installation and unprivileged
build/configure steps.
In `@Makefile.d/functions.mk`:
- Around line 135-150: Update the go-e2e-build command’s linker flags around
-ldflags so -extldflags '-static' is applied only when GOOS is not Darwin. For
Darwin builds, omit the static linker option and retain the existing non-static
Go test/build arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15c653a1-63ea-42ee-8bf4-dfb38cf87583
📒 Files selected for processing (4)
MakefileMakefile.d/functions.mkMakefile.d/tools.mkdocs/contributing/development.md
Native macOS functional validation: image similarity searchValidated the complete native Darwin/arm64 path at signed commit Test pipeline:
Result (lower distance is more similar): Vald correctly returned This exercised the working end-to-end path: The temporary server and generated demo artifacts were removed afterward; the repository remained clean. |
|
@aaf2tbz
Thank you! |
01a819d to
aeff4dc
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile.d/tools.mk`:
- Around line 470-488: Replace the Faiss post-build header installation command
after the CMake configuration block with POSIX-compatible directory creation and
copying: create each destination directory using mkdir -p, then copy headers
with install -m 0644 without the GNU-only -D option. Preserve the existing
destination paths and ensure the command works on Darwin.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f5425b3-619c-42da-adfc-8199a8fdc982
📒 Files selected for processing (6)
.gitignoreMakefileMakefile.d/functions.mkMakefile.d/tools.mkdocs/contributing/development.mdinternal/core/algorithm/faiss/Capi.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile.d/tools.mk`:
- Line 489: Add a concise Makefile comment immediately before the FAISS
header-copy command describing that it discovers and installs headers under
$(USR_LOCAL)/include while preserving failure handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5c867f5b-303e-4b65-8138-55c2598823a9
📒 Files selected for processing (1)
Makefile.d/tools.mk
2 participants
Description
Enables building and testing Vald natively on macOS (Darwin, arm64/Apple Silicon and amd64), without Docker/devcontainer. Today
make ngt/install,make faiss/install, andmake testall fail at configure/compile time on macOS because the build recipes assume a GNU/Linux toolchain. This makes the flags OS-aware while preserving the existing Linux flag expansions, and fixes one cross-platform source bug. See #3570 for the full design.What changed (6 files, +112/−27):
Makefile,ngt/install):-flto=auto -ffat-lto-objects→NATIVE_LTO_FLAGS(-flto=thinon Darwin;-ffat-lto-objectsis an unsupported LLVM target feature for Mach-O). OpenMP resolved viabrew --prefix libomp(-DOpenMP_ROOT) sofind_package(OpenMP)succeeds for Apple clang.-DCMAKE_INSTALL_RPATH=@loader_path/../libsobin/ngtresolveslibngt.2.dylibwithoutldconfig.ldconfigguarded (absent on Darwin).Makefile,faiss/install): dropped the GNULDFLAGS-as-CFLAGSand the-fopenmp -lopenblas -llapack -lgfortranlinker flags on Darwin; cmake now finds OpenMP (OpenMP_ROOT) and BLAS/LAPACK (Apple Accelerate.framework) natively.BLA_VENDOR=OpenBLASkept on Linux, unset on Darwin.Makefile,Makefile.d/tools.mk): skip the static example binaries on Darwin (-DHDF5_BUILD_EXAMPLES=OFF) —-staticlinking fails (nocrt0.oon macOS). Runldconfigonly when present, while preserving a realldconfigfailure on Linux.make test(Makefile,Makefile.d/functions.mk): Darwin now uses thin-LTO, libc++, Homebrew libomp, andAccelerate.frameworkfor both test and native binary link paths. GNU/ELF-only flags (-static,-z,-ffat-lto-objects, OpenBLAS/gfortran) remain on Linux only. The cgo build macros useNATIVE_LTO_FLAGS, preserve the Homebrew libomp include in all binary/example/e2e command-local cgo flags, and apply DarwinCGO_LDFLAGSto both arm64 and amd64. Darwin HDF5 flags also omit the unnecessary-ldl; Linux retains it.internal/core/algorithm/faiss/Capi.cpp(source portability bug):faiss::idx_tisint64_t=longon Linux butlong longon Darwin. The C wrapper passedlong*(matching Go's*C.long), which type-mismatches on Darwin. Cast tofaiss::idx_t*at the 6 faiss call sites (add_with_ids,search,IDSelectorArray) — safereinterpret_cast(both 64-bit, identical layout on LP64).Helper vars generalized and shared:
OPENMP_PREFIX,OPENMP_CFLAGS,NATIVE_LTO_FLAGS. macOS setup is documented indocs/contributing/development.md, including an architecture-neutral optional LLVM path via$(brew --prefix llvm)/bin.Related Issue
#3570 (extends #2694 /
VALD-359).Versions
Checklist
Special notes for your reviewer
make ngt/install,make faiss/install,make hdf5/installall green;libngt.a/libfaiss.alink against libomp + Accelerate and return correct ANN search results (functional C/C++ tests);go build ./...compiles the entire codebase; NGT + faiss cgo unit tests run green; 246/254 unit-test packages pass viamake test.brew install llvmis NOT required (verified by building with brewllvmremoved from PATH).0d801156aand01a819de0:make GOOS=darwin GOARCH=arm64 cmd/agent/core/ngt/ngt cmd/agent/core/faiss/faisspassed after each flag change; both Mach-O binaries reported cgo enabled and linked Homebrewlibompplus AppleAccelerate.framework. The final compile commands include Homebrew’slibomp/include; Darwin arm64/amd64 and Linux arm64/amd64 flag expansions were also checked.make -pnconfirms the original GNU LTO, OpenBLAS/LAPACK, gfortran, static, and ELF linker flags remain forGOOS=linuxon arm64 and amd64; the Dockerfile'sNGT_EXTRA_CMAKE_FLAGSoverride still applies.make dockerfilecodegen / ruleguard — fail on Linux too);internal/file/watchis a fsnotify kqueue/goleak behavior; and 3 NGT-agent property tests surface a pre-existing, platform-independent vqueue correctness bug filed separately as vqueue newer() timestamp collision: GetVector reports not-exists on fast CPUs (delete+insert in same nanosecond) #3571 (timestamp collision innewer()).build(ngt),build(faiss),build(cgo,test),docs(development), plus two focusedbuild(darwin)review-follow-up commits.Summary by CodeRabbit
New Features
Bug Fixes
ldconfigis unavailable.Documentation
Chores