fix: resolve docker/CI build failures from cmake flag mixing and toolchain export pollution - #3520
fix: resolve docker/CI build failures from cmake flag mixing and toolchain export pollution#3520kpango wants to merge 1 commit into
Conversation
Deploying vald with
|
| Latest commit: |
0eef215
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fc300c6f.vald.pages.dev |
| Branch Preview URL: | https://bugfix-agent-investigate-seg.vald.pages.dev |
082d835 to
52c477b
Compare
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
Bot
added
area/internal
area/makefile
actions/backport/release/v1.7
actions/build/amd64
9812744 to
b10fad5
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to address a SIGABRT occurring in the NGT-based agent by adjusting build/link settings (toolchain, LTO, OpenMP/static linking) and tightening CGO memory pinning when passing Go slices to NGT’s C API.
Changes:
- Add
runtime.Pinnerusage in NGT CGO calls (Search/LinearSearch/Insert) and stop manually nil’ing vectors after calls. - Rework build configuration for NGT (clang/lld toolchain defaults, ThinLTO/OpenMP/static link flags, NGT CMake options).
- Add new Rust cargo target configuration and introduce a new (currently empty) e2e asset entry.
Reviewed changes
Copilot reviewed 8 out of 25 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/.cargo/config.toml | Adds clang/lld linker config and CPU tuning for Rust targets. |
| Makefile.d/tools.mk | Whitespace/indent normalization in a sed-based target. |
| Makefile.d/tls.mk | Whitespace/indent normalization in cert cleanup target. |
| Makefile.d/test.mk | Whitespace/indent normalization in a test helper target. |
| Makefile.d/proto.mk | Whitespace/indent normalization in proto replace target. |
| Makefile.d/minikube.mk | Whitespace/indent normalization in restart target. |
| Makefile.d/kind.mk | Whitespace/indent normalization in restart target. |
| Makefile.d/k8s.mk | Whitespace/indent normalization in kube-linter install target. |
| Makefile.d/k3d.mk | Whitespace/indent normalization in delete target. |
| Makefile.d/k0s.mk | Whitespace/indent normalization in cleanup step. |
| Makefile.d/helm.mk | Whitespace/indent normalization in schema generation target. |
| Makefile.d/git.mk | Whitespace/indent normalization in hooks init target. |
| Makefile.d/functions.mk | Updates CGO flags used by build macros (ThinLTO + dataset macro). |
| Makefile.d/e2e.mk | Whitespace/indent normalization in e2e cleanup step. |
| Makefile.d/docker.mk | Whitespace/indent normalization in docker build target. |
| Makefile.d/dependencies.mk | Whitespace/indent normalization in template update target. |
| Makefile.d/build.mk | Updates NGT agent build to pass -extld '$(CXX)' and minor whitespace. |
| Makefile.d/bench.mk | Whitespace/indent normalization in bench kill pipeline. |
| Makefile.d/actions.mk | Whitespace/indent normalization in actions update target. |
| Makefile | Major build/link toolchain and NGT install/link flag changes (clang/lld, ThinLTO, OpenMP/libgomp, AVX2 flag, sudo install). |
| internal/core/algorithm/ngt/option.go | Adds -L/usr/local/lib to cgo LDFLAGS for libngt. |
| internal/core/algorithm/ngt/ngt.go | Pins Go slice memory before passing to CGO; updates cgo LDFLAGS. |
| internal/core/algorithm/ngt/Makefile | Whitespace/indent normalization in bench target. |
| dockers/agent/core/ngt/Dockerfile | Installs clang/lld/libc++ toolchain and uses it for NGT + agent build. |
| .gitfiles | Adds Rust cargo config and a new e2e asset path. |
| tests/v2/e2e/assets/standalone_crud.yaml | Adds a new e2e asset file (currently empty). |
Comments suppressed due to low confidence (1)
.gitfiles:2403
.gitfilesincludestests/v2/e2e/assets/standalone_crud.yaml, but that asset file is currently empty. Either populate the YAML with the intended scenario configuration or drop it from.gitfilesuntil it’s actually needed.
tests/v2/e2e/assets/multi_crud.yaml
tests/v2/e2e/assets/readreplica.yaml
tests/v2/e2e/assets/rollout.yaml
tests/v2/e2e/assets/stream_crud.yaml
tests/v2/e2e/assets/unary_crud.yaml
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rustflags = ["-C", "link-arg=-fuse-ld=lld", "-C", "target-cpu=native"] | ||
|
|
||
| [target.aarch64-unknown-linux-gnu] | ||
| linker = "clang" | ||
| rustflags = ["-C", "link-arg=-fuse-ld=lld", "-C", "target-cpu=native"] |
There was a problem hiding this comment.
target-cpu=native in a repo-wide .cargo/config.toml makes release artifacts non-portable and can break cross-compilation (e.g., producing binaries that require the build machine’s CPU features). Consider removing it from the shared config and instead setting CPU tuning via CI/local RUSTFLAGS, or using an explicit baseline CPU (e.g., aarch64 generic / x86-64-v2) if you need predictable optimization.
| rustflags = ["-C", "link-arg=-fuse-ld=lld", "-C", "target-cpu=native"] | |
| [target.aarch64-unknown-linux-gnu] | |
| linker = "clang" | |
| rustflags = ["-C", "link-arg=-fuse-ld=lld", "-C", "target-cpu=native"] | |
| rustflags = ["-C", "link-arg=-fuse-ld=lld"] | |
| [target.aarch64-unknown-linux-gnu] | |
| linker = "clang" | |
| rustflags = ["-C", "link-arg=-fuse-ld=lld"] |
| NGT_REPO = github.com/NGT-labs/NGT | ||
|
|
||
| NGT_EXTRA_CMAKE_FLAGS ?= | ||
| NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON |
There was a problem hiding this comment.
Defaulting NGT_EXTRA_CMAKE_FLAGS to -DNGT_AVX2=ON can make the produced binaries unusable on hosts without AVX2 support. To avoid runtime crashes on older CPUs, consider keeping this empty by default and enabling AVX2 only via an opt-in build flag or a CPU-feature check.
| NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON | |
| # Keep AVX2 disabled by default so generated binaries remain compatible with | |
| # hosts that do not support AVX2. Enable it explicitly by overriding | |
| # NGT_EXTRA_CMAKE_FLAGS when building for known-compatible CPUs. | |
| NGT_EXTRA_CMAKE_FLAGS ?= |
| CC = clang | ||
| CXX = clang++ | ||
| AR = llvm-ar | ||
| NM = llvm-nm | ||
| RANLIB = llvm-ranlib |
There was a problem hiding this comment.
These toolchain variables are assigned with = which overrides any user/CI-provided CC/CXX/AR/NM/RANLIB. If the intent is to provide defaults, use ?= so downstream builds can still override (especially important for non-Linux environments and cross-compilation).
| CC = clang | |
| CXX = clang++ | |
| AR = llvm-ar | |
| NM = llvm-nm | |
| RANLIB = llvm-ranlib | |
| CC ?= clang | |
| CXX ?= clang++ | |
| AR ?= llvm-ar | |
| NM ?= llvm-nm | |
| RANLIB ?= llvm-ranlib |
| LDFLAGS_BASE = -fuse-ld=lld -fPIC -pthread -lm -z relro -z now -flto=thin -march=native -mtune=native -fno-plt -O3 -ffast-math -ffp-contract=fast -fmerge-all-constants -funroll-loops -falign-functions=32 -ffunction-sections -fdata-sections -Wl,--whole-archive -lpthread -Wl,--no-whole-archive -Wl,--export-dynamic -fopenmp=libgomp | ||
| LDFLAGS = -static $(LDFLAGS_BASE) | ||
| NGT_LDFLAGS = -L/usr/local/lib -fopenmp=libgomp -lopenblas -llapack -lgfortran -L/usr/lib/gcc/x86_64-linux-gnu/15/ | ||
| FAISS_LDFLAGS = $(NGT_LDFLAGS) | ||
| HDF5_LDFLAGS = -lhdf5 -lhdf5_hl -lsz -laec -lz -ldl -lm | ||
| CGO_LDFLAGS = $(FAISS_LDFLAGS) $(HDF5_LDFLAGS) | ||
| # TEST_LDFLAGS without -static to avoid conflicts with CGO and glibc dynamic linking requirements | ||
| TEST_LDFLAGS_BASE = -fPIC -pthread -std=gnu++23 -lstdc++ -lm -z relro -z now -flto=auto -ffat-lto-objects -march=native -mtune=native -fno-plt -O3 -ffast-math -fvisibility=hidden -ffp-contract=fast -fomit-frame-pointer -fmerge-all-constants -funroll-loops -falign-functions=32 -ffunction-sections -fdata-sections | ||
| TEST_LDFLAGS = $(TEST_LDFLAGS_BASE) $(CGO_LDFLAGS) | ||
| CGO_LDFLAGS = -fuse-ld=lld $(FAISS_LDFLAGS) $(HDF5_LDFLAGS) | ||
| TEST_LDFLAGS = $(LDFLAGS_BASE) $(CGO_LDFLAGS) |
There was a problem hiding this comment.
Hardcoding GCC-version-specific paths (/usr/lib/gcc/x86_64-linux-gnu/15/) into NGT_LDFLAGS makes builds fragile across Ubuntu/Debian versions and CI images. Prefer discovering the required library path at build time (e.g., via $(shell $(CC) -print-file-name=...) / pkg-config) or making this path an overrideable variable.
| ifeq ($(GOARCH),amd64) | ||
| CFLAGS ?= -mno-avx512f -mno-avx512dq -mno-avx512cd -mno-avx512bw -mno-avx512vl | ||
| CXXFLAGS ?= $(CFLAGS) | ||
| CFLAGS = -mno-avx512f -mno-avx512dq -mno-avx512cd -mno-avx512bw -mno-avx512vl -I$(ROOTDIR)/libs/omp_include -D_OPENMP=202011 |
There was a problem hiding this comment.
-I$(ROOTDIR)/libs/omp_include points to a directory that does not exist in this repository, which can create noisy compiler warnings and makes the build configuration harder to reason about. If an extra OpenMP include path is needed, it should reference an existing location or be derived from the toolchain (or removed if not required).
| CFLAGS = -mno-avx512f -mno-avx512dq -mno-avx512cd -mno-avx512bw -mno-avx512vl -I$(ROOTDIR)/libs/omp_include -D_OPENMP=202011 | |
| CFLAGS = -mno-avx512f -mno-avx512dq -mno-avx512cd -mno-avx512bw -mno-avx512vl -D_OPENMP=202011 |
| -DCMAKE_C_FLAGS="$(CFLAGS) $(LDFLAGS_BASE)" \ | ||
| -DCMAKE_CXX_FLAGS="$(CXXFLAGS) $(LDFLAGS_BASE)" \ |
There was a problem hiding this comment.
LDFLAGS_BASE contains linker-only flags (-Wl,..., --whole-archive, etc.) but it is being appended to CMAKE_C_FLAGS / CMAKE_CXX_FLAGS. This mixes compile and link flags and can lead to confusing warnings or misconfiguration. Consider moving linker options into CMAKE_EXE_LINKER_FLAGS (and related CMake linker flag vars) and keeping CMAKE_*_FLAGS to compilation flags only.
| -DCMAKE_C_FLAGS="$(CFLAGS) $(LDFLAGS_BASE)" \ | |
| -DCMAKE_CXX_FLAGS="$(CXXFLAGS) $(LDFLAGS_BASE)" \ | |
| -DCMAKE_C_FLAGS="$(CFLAGS)" \ | |
| -DCMAKE_CXX_FLAGS="$(CXXFLAGS)" \ | |
| -DCMAKE_EXE_LINKER_FLAGS="$(LDFLAGS_BASE)" \ |
| -DOpenMP_C_LIB_NAMES="gomp" \ | ||
| -DOpenMP_CXX_FLAGS="-fopenmp" \ | ||
| -DOpenMP_CXX_LIB_NAMES="gomp" \ | ||
| -DOpenMP_gomp_LIBRARY="/usr/lib/gcc/x86_64-linux-gnu/15/libgomp.a" \ |
There was a problem hiding this comment.
This CMake OpenMP configuration hardcodes a specific libgomp static library path (.../x86_64-linux-gnu/15/libgomp.a), which is unlikely to exist across different distributions / compiler versions. Consider discovering the OpenMP runtime via CMake’s FindOpenMP (no hardcoded path), or make the library path configurable and derived from the installed toolchain.
| -DOpenMP_gomp_LIBRARY="/usr/lib/gcc/x86_64-linux-gnu/15/libgomp.a" \ |
| make -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build -j$(CORES) ngt | ||
| make -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build install | ||
| sudo make -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build install | ||
| cd $(ROOTDIR) | ||
| rm -rf $(TEMP_DIR)/NGT-$(NGT_VERSION) | ||
| ldconfig | ||
| sudo ldconfig |
There was a problem hiding this comment.
Using sudo inside the ngt/install Makefile target is brittle (many CI/Docker environments run as root and don’t have sudo installed). Consider removing sudo here, or gating it behind a check (e.g., only use sudo when not already running as root).
b10fad5 to
73d6042
Compare
|
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:
📝 WalkthroughWalkthroughSwitches many build artifacts to Clang/LLVM (clang/clang++/lld), centralizes LTO/linker flags (thin-LTO, -fuse-ld=lld), propagates explicit toolchain env vars through Make/CMake/go/cargo, adds LLVM OpenMP version/install targets, enables NGT AVX2, pins NGT Go binding memory with runtime.Pinner, adds Rust per-target cargo config, and normalizes many EOF newlines. Changes
Sequence Diagram(s)(omitted — changes are primarily build/tooling and formatting) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/algorithm/ngt/Makefile (1)
17-25: 🧹 Nitpick | 🔵 TrivialLGTM — EOF normalization only.
The benchmark targets remain functionally unchanged.
Note:
benchandbench-litelack.PHONYdeclarations, which could cause unexpected behavior if files with those names exist. This is pre-existing, but worth addressing if you're touching this file. As per coding guidelines: "Verify that.PHONYis correctly used for non-file targets."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/algorithm/ngt/Makefile` around lines 17 - 25, The Makefile defines non-file targets bench and bench-lite but lacks a .PHONY declaration; add a .PHONY rule listing bench and bench-lite (e.g., add a line like ".PHONY: bench bench-lite") so these targets are treated as phony by make; update the Makefile near the bench/bench-lite target definitions and ensure the .PHONY entry includes both target names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 870-873: The ngt/install Makefile steps are using sudo
unnecessarily; edit the ngt/install target to remove the "sudo" prefix from the
two commands shown (the make -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build install
invocation and the ldconfig call) so they run as plain commands (since Docker
builder runs as root), leaving the cd and rm -rf lines unchanged; update the
Makefile entries that call these commands (look for the ngt/install target or
the exact lines containing "make -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build
install" and "ldconfig") to remove sudo.
- Line 65: The NGT_EXTRA_CMAKE_FLAGS variable is set unconditionally to enable
AVX2; make it conditional on the target architecture by checking GOARCH so AVX2
is only enabled for amd64 builds: update the Makefile to set
NGT_EXTRA_CMAKE_FLAGS to "-DNGT_AVX2=ON" when GOARCH equals "amd64" and to an
empty/default value otherwise (use the existing GOARCH-based ifeq pattern
already used elsewhere in the file to locate where to add this conditional).
In `@rust/.cargo/config.toml`:
- Around line 1-7: The rustflags currently force "-C target-cpu=native" for both
target.x86_64-unknown-linux-gnu and target.aarch64-unknown-linux-gnu which can
produce instructions not supported on older deployment hosts; replace or remove
the "target-cpu=native" setting in the rustflags for both sections
(target.x86_64-unknown-linux-gnu and target.aarch64-unknown-linux-gnu) and
instead use a conservative CPU baseline (e.g., a generic/specified CPU like
"x86-64" or a stable microarchitecture such as "cortex-a53" for aarch64) or make
the flag configurable via an env var so native optimizations are only enabled
when you know build and deploy hardware match.
---
Outside diff comments:
In `@internal/core/algorithm/ngt/Makefile`:
- Around line 17-25: The Makefile defines non-file targets bench and bench-lite
but lacks a .PHONY declaration; add a .PHONY rule listing bench and bench-lite
(e.g., add a line like ".PHONY: bench bench-lite") so these targets are treated
as phony by make; update the Makefile near the bench/bench-lite target
definitions and ensure the .PHONY entry includes both target names.
🪄 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: 551a34b2-5bec-4b7a-9195-313a1f45df46
⛔ Files ignored due to path filters (1)
hack/docker/gen/main.gois excluded by!**/gen/**
📒 Files selected for processing (25)
.gitfilesMakefileMakefile.d/actions.mkMakefile.d/bench.mkMakefile.d/build.mkMakefile.d/dependencies.mkMakefile.d/docker.mkMakefile.d/e2e.mkMakefile.d/functions.mkMakefile.d/git.mkMakefile.d/helm.mkMakefile.d/k0s.mkMakefile.d/k3d.mkMakefile.d/k8s.mkMakefile.d/kind.mkMakefile.d/minikube.mkMakefile.d/proto.mkMakefile.d/test.mkMakefile.d/tls.mkMakefile.d/tools.mkdockers/agent/core/ngt/Dockerfileinternal/core/algorithm/ngt/Makefileinternal/core/algorithm/ngt/ngt.gointernal/core/algorithm/ngt/option.gorust/.cargo/config.toml
| NGT_REPO = github.com/NGT-labs/NGT | ||
|
|
||
| NGT_EXTRA_CMAKE_FLAGS ?= | ||
| NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if NGT CMake properly handles AVX2 flag on arm64
rg -n "AVX2" --type cmakeRepository: vdaas/vald
Length of output: 202
🏁 Script executed:
# Find NGT-related cmake or build files
fd -type f -name "CMakeLists.txt" -o -name "*cmake*" | head -20Repository: vdaas/vald
Length of output: 224
🏁 Script executed:
# Check the ngt/install target in the Makefile to see how NGT_EXTRA_CMAKE_FLAGS is used
rg -n "ngt/install" -A 20 --type makefileRepository: vdaas/vald
Length of output: 86
🏁 Script executed:
# Check if GOARCH is already used in the Makefile
rg -n "GOARCH" --type makefileRepository: vdaas/vald
Length of output: 86
🏁 Script executed:
# Search for AVX2 references in the entire codebase to understand current handling
rg -i "avx2" -B 2 -A 2 | head -50Repository: vdaas/vald
Length of output: 50368
🏁 Script executed:
# Find the ngt/install target to see how NGT_EXTRA_CMAKE_FLAGS is used
grep -n "ngt/install" Makefile | head -5Repository: vdaas/vald
Length of output: 143
🏁 Script executed:
# Check if GOARCH is used elsewhere in the Makefile
grep -n "GOARCH" MakefileRepository: vdaas/vald
Length of output: 175
🏁 Script executed:
# Look for architecture-conditional patterns already in Makefile
grep -n "ifeq\|ifneq" Makefile | head -20Repository: vdaas/vald
Length of output: 291
🏁 Script executed:
# Check the ngt/install target definition more carefully
sed -n '850,880p' MakefileRepository: vdaas/vald
Length of output: 1192
🏁 Script executed:
# Check if there's any NGT-related documentation or comments about architecture handling
grep -n "AVX" MakefileRepository: vdaas/vald
Length of output: 95
🏁 Script executed:
# Look at the full context around lines 185-210 to see the pattern
sed -n '185,210p' MakefileRepository: vdaas/vald
Length of output: 808
Make NGT_AVX2 flag conditional on amd64 architecture.
The flag -DNGT_AVX2=ON is set unconditionally, but your Makefile already establishes per-architecture configuration patterns (lines 185–210 use ifeq ($(GOARCH),amd64) and ifeq ($(GOARCH),arm64) for platform-specific settings). For clarity and to align with dual-architecture support (amd64 with AVX2, arm64), set this flag only when building for amd64:
Suggested pattern
ifeq ($(GOARCH),amd64)
NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON
else
NGT_EXTRA_CMAKE_FLAGS ?=
endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 65, The NGT_EXTRA_CMAKE_FLAGS variable is set
unconditionally to enable AVX2; make it conditional on the target architecture
by checking GOARCH so AVX2 is only enabled for amd64 builds: update the Makefile
to set NGT_EXTRA_CMAKE_FLAGS to "-DNGT_AVX2=ON" when GOARCH equals "amd64" and
to an empty/default value otherwise (use the existing GOARCH-based ifeq pattern
already used elsewhere in the file to locate where to add this conditional).
73d6042 to
c64d107
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dockers/agent/core/agent/Dockerfile (1)
68-83:⚠️ Potential issue | 🟡 MinorDuplicate package installation:
clangandlldare installed twice.Lines 68 and 70 already install
clangandlld, but lines 82-83 install them again. This adds unnecessary overhead to the image build (though apt handles duplicates gracefully).Proposed fix to remove duplicates
pkgconf \ protobuf-compiler \ libprotobuf-dev \ - clang \ - lld \ && ldconfig \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dockers/agent/core/agent/Dockerfile` around lines 68 - 83, The Dockerfile's apt package list contains duplicate entries for "clang" and "lld" (they appear in the earlier group and again at the end); remove the duplicate occurrences (keep the first instances) from the package installation list so each package is listed only once to avoid unnecessary repetition during image build.Makefile.d/build.mk (1)
56-64: 🧹 Nitpick | 🔵 TrivialEnsure Faiss build uses explicit external linker specification for consistency with NGT.
The NGT build now specifies
-extld '$(CXX)'to ensure Go's external linking uses the same C++ compiler (clang++) as the CMake build. However, the Faiss build at line 64 lacks both:
- Explicit
-DCMAKE_C_COMPILERand-DCMAKE_CXX_COMPILERin its CMake configuration (lines 882–893)- The
-extld '$(CXX)'flag in its Go build callThis creates a risk: Faiss may be compiled with the system default compiler (potentially gcc) while NGT is compiled with clang++, leading to linker/ABI incompatibilities. For consistent LTO and LLD behavior across both libraries, Faiss should:
- Add
-DCMAKE_C_COMPILER="$(CC)" -DCMAKE_CXX_COMPILER="$(CXX)"to its CMake configuration- Add
-extld '$(CXX)'to its Go build call (line 64)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile.d/build.mk` around lines 56 - 64, The Faiss build is missing explicit C/C++ compiler and external linker flags causing possible ABI mismatch; update the Faiss CMake invocation to include -DCMAKE_C_COMPILER="$(CC)" and -DCMAKE_CXX_COMPILER="$(CXX)" (where the Faiss CMake args are configured, currently around the faiss/install CMake options) and modify the go-build invocation for the cmd/agent/core/faiss/faiss target to add -extld '$(CXX)' (matching the NGT call) so the go-build call uses $(CXX) as the external linker.
♻️ Duplicate comments (3)
rust/.cargo/config.toml (1)
18-18:⚠️ Potential issue | 🟠 MajorAvoid
target-cpu=nativein shared build pipelines.Line 18 and Line 22 hard-code host-specific CPU tuning, which can produce binaries that fail on older or different deployment CPUs in multi-arch/container builds. Use a conservative baseline (or make it opt-in via env).
Suggested patch
[target.x86_64-unknown-linux-gnu] linker = "clang" -rustflags = ["-C", "link-arg=-fuse-ld=lld", "-C", "target-cpu=native"] +rustflags = ["-C", "link-arg=-fuse-ld=lld", "-C", "target-cpu=x86-64-v2"] [target.aarch64-unknown-linux-gnu] linker = "clang" -rustflags = ["-C", "link-arg=-fuse-ld=lld", "-C", "target-cpu=native"] +rustflags = ["-C", "link-arg=-fuse-ld=lld", "-C", "target-cpu=generic"]#!/bin/bash set -euo pipefail echo "== .cargo target-cpu settings ==" rg -n --no-heading 'target-cpu=' rust/.cargo/config.toml echo echo "== Cargo build invocations without explicit --target (context) ==" rg -n --no-heading 'cargo build' Makefile.d/build.mk dockers/agent/core/agent/Dockerfile echo echo "== Buildx target args presence (context) ==" rg -n --no-heading 'ARG TARGETARCH|ARG TARGETOS' dockers/agent/core/agent/DockerfileBased on learnings: Dockerfile changes must support both
amd64(with AVX2 for NGT) andarm64architectures.Also applies to: 22-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/.cargo/config.toml` at line 18, The rustflags entry currently hard-codes host-specific tuning ("target-cpu=native") which can produce non-portable binaries; update the rustflags configuration (the rustflags = [...] line) to remove or replace "target-cpu=native" with a conservative baseline (e.g., generic or not specifying target-cpu) and/or make it opt-in via an environment variable (check and expand the rustflags array from env like RUST_TARGET_CPU if set) so shared CI/container builds produce portable artifacts; ensure the change affects the rustflags line in rust/.cargo/config.toml and document/guard AVX2-specific builds separately for amd64 vs arm64 contexts.Makefile (2)
65-65:⚠️ Potential issue | 🟠 MajorGuard
NGT_AVX2behindGOARCH == amd64.
-DNGT_AVX2=ONis still unconditional here, so the same x86-only SIMD knob is sent to arm64 builds as well. This needs to follow the existingGOARCHsplit instead of being global.Suggested fix
-NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON +NGT_EXTRA_CMAKE_FLAGS ?= ... ifeq ($(GOARCH),amd64) +NGT_EXTRA_CMAKE_FLAGS = -DNGT_AVX2=ON CFLAGS = -mno-avx512f -mno-avx512dq -mno-avx512cd -mno-avx512bw -mno-avx512vl CXXFLAGS = $(CFLAGS) -std=gnu++23Based on learnings: Dockerfile changes must support both
amd64(with AVX2 for NGT) andarm64architectures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 65, The Makefile currently sets NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON unconditionally; change this to set the AVX2 flag only when GOARCH == amd64 by making NGT_EXTRA_CMAKE_FLAGS conditional on GOARCH (check the existing GOARCH split pattern in the Makefile) and include -DNGT_AVX2=ON only in that amd64 branch, leaving the arm64/default branch without the AVX2 flag; update any references to NGT_EXTRA_CMAKE_FLAGS accordingly so arm64 builds do not receive the x86-only -DNGT_AVX2=ON flag.
870-873:⚠️ Potential issue | 🟠 MajorDrop
sudohere and guardldconfigfor the host environment.
ngt/installis part ofmake init, but this Makefile also carries explicit Darwin branches. Requiringsudomakes the target fail in minimal builder images, and unconditionalldconfigkeeps it Linux-only.Suggested fix
- sudo make -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build install + make -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build install cd $(ROOTDIR) rm -rf $(TEMP_DIR)/NGT-$(NGT_VERSION) - sudo ldconfig + if command -v ldconfig >/dev/null 2>&1; then ldconfig; fiAs per coding guidelines: Shell commands are POSIX-compliant or guarded for environment (no bash-specific syntax if
/bin/shis used).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 870 - 873, Remove the hard-coded sudo and make ldconfig conditional: replace the lines invoking "sudo make -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build install" and "sudo ldconfig" so the install runs without sudo (allow caller to escalate if needed) and invoke ldconfig only on Linux hosts (e.g., test uname -s for "Linux" or check for existence of /sbin/ldconfig) so Darwin/minimal builder images don't fail; ensure the shell checks use POSIX /bin/sh-compatible syntax and preserve the existing cd and rm -rf $(TEMP_DIR)/NGT-$(NGT_VERSION) behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dockers/agent/core/faiss/Dockerfile`:
- Around line 70-74: The faiss/install make target doesn't set explicit
compilers so CMake defaults to GCC even though Clang is installed; update the
faiss/install cmake invocation (same place as the ngt/install target) to pass
-DCMAKE_C_COMPILER="$(CC)" and -DCMAKE_CXX_COMPILER="$(CXX)" so Faiss is built
with the intended Clang toolchain (refer to the faiss/install target and the
CC/CXX variables used elsewhere).
In `@go.mod`:
- Around line 114-122: The go.mod has mismatched versions between require and
replace for OpenTelemetry modules (e.g.,
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc and
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc); update the
require lines to match the replace versions (or remove the unnecessary replace
directives) and then run go mod tidy to synchronize dependencies so require and
replace for otlpmetricgrpc and otlptracegrpc (and any other otel modules like
go.opentelemetry.io/otel, metric, sdk, trace) are consistent.
In `@Makefile`:
- Around line 171-183: The Makefile now defaults to an LLVM/OpenMP toolchain
(CC/ CXX set to clang/clang++, use of -fuse-ld=lld, LDFLAGS_BASE with
-fopenmp=libgomp, and NGT_LDFLAGS/FAISS_LDFLAGS/CGO_LDFLAGS referencing
OpenBLAS/LAPACK/HDF5), so add a documented build contract to AGENTS.md or the
build README that lists required compilers (clang/clang++), linker (lld),
runtime OpenMP library (libgomp), expected system libs (openblas, lapack,
gfortran, hdf5, lz, dl), and the rationale for TEST_LDFLAGS/LDFLAGS choices;
include explicit instructions for troubleshooting hidden deps and a rule to
update this doc whenever Makefile variables CC, CXX, LDFLAGS_BASE, NGT_LDFLAGS,
CGO_LDFLAGS, or new Makefile.d/*.mk patterns change.
- Around line 177-179: LDFLAGS_BASE currently contains codegen/ISA flags (e.g.,
-march=native -mtune=native and other optimization flags) which get appended
into CMAKE_C_FLAGS via CMAKE_C_FLAGS="$(CFLAGS) $(LDFLAGS)" and reintroduce
host-specific ISA; remove machine/optimization/codegen flags (at minimum
-march=native and -mtune=native, and preferably related flags like -O3
-ffast-math -ffp-contract=fast -fmerge-all-constants -funroll-loops
-falign-functions=32 -ffunction-sections -fdata-sections -flto=thin, etc.) from
LDFLAGS_BASE and put them into CFLAGS and CXXFLAGS instead, leaving
LDFLAGS/LDFLAGS_BASE with only link-time and linker-specific options (-static,
-fuse-ld=lld, -z relro, -z now, -Wl,--*, -lpthread, -fopenmp=libgomp, etc.) so
the amd64 ISA guard can control code generation uniformly (relevant symbols:
LDFLAGS_BASE, LDFLAGS, CFLAGS, CXXFLAGS and the CMAKE_C_FLAGS usage in the NGT
build).
In `@rust/libs/kvs/Cargo.toml`:
- Line 27: The tokio dependency in this crate is specified as "1.51" which
differs in format from the observability crate's "1.51.0"; update the tokio
entry in Cargo.toml from tokio = { version = "1.51", features = ["full"] } to
use an explicit patch version (e.g., "1.51.0") so the tokio dependency version
format matches the observability crate and yields deterministic patch
resolution.
---
Outside diff comments:
In `@dockers/agent/core/agent/Dockerfile`:
- Around line 68-83: The Dockerfile's apt package list contains duplicate
entries for "clang" and "lld" (they appear in the earlier group and again at the
end); remove the duplicate occurrences (keep the first instances) from the
package installation list so each package is listed only once to avoid
unnecessary repetition during image build.
In `@Makefile.d/build.mk`:
- Around line 56-64: The Faiss build is missing explicit C/C++ compiler and
external linker flags causing possible ABI mismatch; update the Faiss CMake
invocation to include -DCMAKE_C_COMPILER="$(CC)" and
-DCMAKE_CXX_COMPILER="$(CXX)" (where the Faiss CMake args are configured,
currently around the faiss/install CMake options) and modify the go-build
invocation for the cmd/agent/core/faiss/faiss target to add -extld '$(CXX)'
(matching the NGT call) so the go-build call uses $(CXX) as the external linker.
---
Duplicate comments:
In `@Makefile`:
- Line 65: The Makefile currently sets NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON
unconditionally; change this to set the AVX2 flag only when GOARCH == amd64 by
making NGT_EXTRA_CMAKE_FLAGS conditional on GOARCH (check the existing GOARCH
split pattern in the Makefile) and include -DNGT_AVX2=ON only in that amd64
branch, leaving the arm64/default branch without the AVX2 flag; update any
references to NGT_EXTRA_CMAKE_FLAGS accordingly so arm64 builds do not receive
the x86-only -DNGT_AVX2=ON flag.
- Around line 870-873: Remove the hard-coded sudo and make ldconfig conditional:
replace the lines invoking "sudo make -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build
install" and "sudo ldconfig" so the install runs without sudo (allow caller to
escalate if needed) and invoke ldconfig only on Linux hosts (e.g., test uname -s
for "Linux" or check for existence of /sbin/ldconfig) so Darwin/minimal builder
images don't fail; ensure the shell checks use POSIX /bin/sh-compatible syntax
and preserve the existing cd and rm -rf $(TEMP_DIR)/NGT-$(NGT_VERSION) behavior.
In `@rust/.cargo/config.toml`:
- Line 18: The rustflags entry currently hard-codes host-specific tuning
("target-cpu=native") which can produce non-portable binaries; update the
rustflags configuration (the rustflags = [...] line) to remove or replace
"target-cpu=native" with a conservative baseline (e.g., generic or not
specifying target-cpu) and/or make it opt-in via an environment variable (check
and expand the rustflags array from env like RUST_TARGET_CPU if set) so shared
CI/container builds produce portable artifacts; ensure the change affects the
rustflags line in rust/.cargo/config.toml and document/guard AVX2-specific
builds separately for amd64 vs arm64 contexts.
🪄 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: e0484e98-c47c-45b3-ae84-4f78c1c9fa92
⛔ Files ignored due to path filters (8)
example/client/go.sumis excluded by!**/*.sum,!**/go.sumgo.sumis excluded by!**/*.sum,!**/go.sumhack/docker/gen/main.gois excluded by!**/gen/**rust/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lockrust/bin/agent/Cargo.tomlis excluded by!**/bin/**rust/bin/meta/Cargo.tomlis excluded by!**/bin/**versions/PROMETHEUS_STACK_VERSIONis excluded by!versions/**versions/actions/DOCKER_LOGIN_ACTIONis excluded by!versions/**
📒 Files selected for processing (36)
.gitfiles.github/workflows/_docker-image.yamlMakefileMakefile.d/actions.mkMakefile.d/bench.mkMakefile.d/build.mkMakefile.d/dependencies.mkMakefile.d/docker.mkMakefile.d/e2e.mkMakefile.d/functions.mkMakefile.d/git.mkMakefile.d/helm.mkMakefile.d/k0s.mkMakefile.d/k3d.mkMakefile.d/k8s.mkMakefile.d/kind.mkMakefile.d/minikube.mkMakefile.d/proto.mkMakefile.d/test.mkMakefile.d/tls.mkMakefile.d/tools.mkdockers/agent/core/agent/Dockerfiledockers/agent/core/faiss/Dockerfiledockers/agent/core/ngt/Dockerfiledockers/dev/Dockerfiledockers/example/client/Dockerfiledockers/tests/v2/e2e/Dockerfiledockers/tools/benchmark/job/Dockerfileexample/client/go.modgo.modinternal/core/algorithm/ngt/Makefileinternal/core/algorithm/ngt/ngt.gointernal/core/algorithm/ngt/option.gorust/.cargo/config.tomlrust/libs/kvs/Cargo.tomlrust/libs/observability/Cargo.toml
| CC = clang | ||
| CXX = clang++ | ||
| AR = llvm-ar | ||
| NM = llvm-nm | ||
| RANLIB = llvm-ranlib | ||
|
|
||
| NGT_LDFLAGS = -fopenmp -lopenblas -llapack -lgfortran | ||
| LDFLAGS_BASE = -fuse-ld=lld -fPIC -pthread -lm -z relro -z now -flto=thin -march=native -mtune=native -fno-plt -O3 -ffast-math -ffp-contract=fast -fmerge-all-constants -funroll-loops -falign-functions=32 -ffunction-sections -fdata-sections -Wl,--whole-archive -lpthread -Wl,--no-whole-archive -Wl,--export-dynamic -fopenmp=libgomp | ||
| LDFLAGS = -static $(LDFLAGS_BASE) | ||
| NGT_LDFLAGS = -L/usr/local/lib -fopenmp=libgomp -lopenblas -llapack -lgfortran | ||
| FAISS_LDFLAGS = $(NGT_LDFLAGS) | ||
| HDF5_LDFLAGS = -lhdf5 -lhdf5_hl -lsz -laec -lz -ldl -lm | ||
| CGO_LDFLAGS = $(FAISS_LDFLAGS) $(HDF5_LDFLAGS) | ||
| # TEST_LDFLAGS without -static to avoid conflicts with CGO and glibc dynamic linking requirements | ||
| TEST_LDFLAGS_BASE = -fPIC -pthread -std=gnu++23 -lstdc++ -lm -z relro -z now -flto=auto -ffat-lto-objects -march=native -mtune=native -fno-plt -O3 -ffast-math -fvisibility=hidden -ffp-contract=fast -fomit-frame-pointer -fmerge-all-constants -funroll-loops -falign-functions=32 -ffunction-sections -fdata-sections | ||
| TEST_LDFLAGS = $(TEST_LDFLAGS_BASE) $(CGO_LDFLAGS) | ||
| CGO_LDFLAGS = -fuse-ld=lld $(FAISS_LDFLAGS) $(HDF5_LDFLAGS) | ||
| TEST_LDFLAGS = $(LDFLAGS_BASE) $(CGO_LDFLAGS) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the new LLVM/OpenMP build contract.
These changes make clang/clang++/lld plus the NGT BLAS/OpenMP stack the default build path. Please capture the intended compiler/linker/runtime assumptions in AGENTS.md or the relevant build README so follow-up Makefile changes do not silently drift.
Based on learnings: Whenever you resolve a complex error, discover a hidden dependency, or identify a new Makefile and Makefile.d/*.mk pattern, you MUST propose an update to this document to prevent future agents from repeating the same mistake.
Also applies to: 861-866
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 171 - 183, The Makefile now defaults to an LLVM/OpenMP
toolchain (CC/ CXX set to clang/clang++, use of -fuse-ld=lld, LDFLAGS_BASE with
-fopenmp=libgomp, and NGT_LDFLAGS/FAISS_LDFLAGS/CGO_LDFLAGS referencing
OpenBLAS/LAPACK/HDF5), so add a documented build contract to AGENTS.md or the
build README that lists required compilers (clang/clang++), linker (lld),
runtime OpenMP library (libgomp), expected system libs (openblas, lapack,
gfortran, hdf5, lz, dl), and the rationale for TEST_LDFLAGS/LDFLAGS choices;
include explicit instructions for troubleshooting hidden deps and a rule to
update this doc whenever Makefile variables CC, CXX, LDFLAGS_BASE, NGT_LDFLAGS,
CGO_LDFLAGS, or new Makefile.d/*.mk patterns change.
c64d107 to
637c992
Compare
|
/format |
|
[FORMAT] Updating license headers and formatting go codes triggered by kpango. |
|
[FORMAT] Failed to format. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dockers/agent/core/agent/Dockerfile (1)
68-83:⚠️ Potential issue | 🟡 MinorDuplicate package installs:
clangandlldare listed twice.Lines 68 and 82 both install
clang; lines 70 and 83 both installlld. This inflates the layer cache and wastes build time. Remove the duplicates at lines 82-83.🧹 Proposed fix
protobuf-compiler \ libprotobuf-dev \ - clang \ - lld \ && ldconfig \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dockers/agent/core/agent/Dockerfile` around lines 68 - 83, The Dockerfile package list contains duplicate entries for the packages "clang" and "lld"; remove the second occurrences (the trailing "clang" and "lld" in the same apt install list) so each package appears only once, keeping the original first instances and deleting the duplicate tokens to avoid redundant installs and unnecessary layer/cache inflation.
♻️ Duplicate comments (6)
rust/libs/kvs/Cargo.toml (1)
27-27: 🧹 Nitpick | 🔵 TrivialVersion format inconsistency remains unresolved.
The
kvscrate specifies tokio as"1.51"whileobservability,meta, andagentcrates use"1.51.0". In Cargo,"1.51"resolves to the latest1.51.xpatch, whereas"1.51.0"pins exactly to that patch version. This workspace-wide inconsistency can lead to different patch versions being resolved across crates, potentially causing subtle build or runtime differences.Standardize on the explicit three-part semver format (
"1.51.0") to match the rest of the workspace and ensure deterministic dependency resolution.📌 Proposed fix to align version format
-tokio = { version = "1.51", features = ["full"] } +tokio = { version = "1.51.0", features = ["full"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/libs/kvs/Cargo.toml` at line 27, The tokio dependency in the kvs crate uses a two-part semver ("1.51") while other workspace crates use the explicit three-part format; update the tokio entry in the kvs Cargo.toml (the tokio = { version = "1.51", features = ["full"] } line) to use "1.51.0" so it matches the rest of the workspace and ensures deterministic patch resolution.go.mod (1)
114-122:⚠️ Potential issue | 🟡 MinorAlign OTLP exporter versions between
requireandreplace.
requirestill pinsotlpmetricgrpcandotlptracegrpctov1.38.0whilereplaceforcesv1.43.0. Keep them aligned to reduce maintenance risk.🔧 Proposed fix
- go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v1.38.0 + go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v1.43.0 ... - go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.38.0 + go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.43.0#!/bin/bash set -euo pipefail echo "Checking OTLP exporter version skew between require/replace in go.mod" awk ' BEGIN{sec=""} $1=="replace" && $2=="(" {sec="replace"; next} $1=="require" && $2=="(" {sec="require"; next} sec!="" && $1==")" {sec=""; next} sec=="replace" && $1 ~ /^go\.opentelemetry\.io\/otel\/exporters\/otlp\// {rpl[$1]=$3} sec=="require" && $1 ~ /^go\.opentelemetry\.io\/otel\/exporters\/otlp\// {req[$1]=$2} END{ for (k in rpl) { if (k in req && req[k] != rpl[k]) { printf "%s require=%s replace=%s\n", k, req[k], rpl[k] } } } ' go.mod🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 114 - 122, The go.mod shows a version skew: the require entries for go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc and go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc are pinned to v1.38.0 while replace forces v1.43.0; update the require lines for those two packages to v1.43.0 (or change the replace to match, but prefer updating require) so require and replace agree, then run go mod tidy to refresh module files.rust/.cargo/config.toml (1)
18-22:⚠️ Potential issue | 🟠 MajorAvoid
target-cpu=nativefor release portability.Line 18 and Line 22 can bake host-specific instructions into artifacts, which risks runtime
SIGILLon older nodes. Prefer a stable baseline (or make CPU tuning opt-in via environment-specificRUSTFLAGS).Proposed fix
[target.x86_64-unknown-linux-gnu] linker = "clang" -rustflags = ["-C", "link-arg=-fuse-ld=lld", "-C", "target-cpu=native"] +rustflags = ["-C", "link-arg=-fuse-ld=lld"] [target.aarch64-unknown-linux-gnu] linker = "clang" -rustflags = ["-C", "link-arg=-fuse-ld=lld", "-C", "target-cpu=native"] +rustflags = ["-C", "link-arg=-fuse-ld=lld"]#!/bin/bash set -euo pipefail echo "== rust/.cargo/config.toml ==" sed -n '1,120p' rust/.cargo/config.toml echo echo "== Cargo build/test invocations (check for --target usage) ==" rg -n --type=make 'cargo (build|test)' Makefile.d Makefile || true echo echo "== Explicit --target usage (expected: usually none for default flows) ==" rg -n --type=make -- '--target' Makefile.d Makefile || trueExpected verification result:
target-cpu=nativeappears inrust/.cargo/config.toml.- Default Cargo flows used in Makefiles do not consistently pass
--target, so host-target config is applied during normal builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/.cargo/config.toml` around lines 18 - 22, The rustflags entries in rust/.cargo/config.toml currently include "-C", "target-cpu=native" which bakes host-specific CPU tuning into release artifacts; remove "target-cpu=native" from both the global rustflags and the [target.aarch64-unknown-linux-gnu] rustflags, and instead document or default to a stable baseline (e.g., omit CPU tuning or use a conservative target like "x86-64" / platform-appropriate option) and recommend making CPU tuning opt-in via environment-specific RUSTFLAGS so host builds remain portable; update the rustflags lines and add a short comment explaining to set RUSTFLAGS externally for performance tuning.Makefile (3)
65-65:⚠️ Potential issue | 🟠 MajorGate
NGT_AVX2onGOARCH.Line 65 makes the AVX2 path the default for every build, even though the same file still has explicit
amd64/arm64branches starting at Line 187. That makes the NGT configuration x86-specific by default and risks breaking the arm64 build path.To verify the unconditional default against the existing arch branches, inspect these ranges together:
#!/bin/bash set -eu echo "[NGT AVX2 default]" sed -n '65p' Makefile echo echo "[GOARCH branches]" sed -n '187,205p' Makefile echo echo "[NGT cmake invocation]" sed -n '866,873p' Makefile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 65, The Makefile currently sets NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON unconditionally; change this so the AVX2 flag is only enabled for x86 builds by gating it on GOARCH (e.g., leave NGT_EXTRA_CMAKE_FLAGS empty by default and append or set -DNGT_AVX2=ON only when GOARCH is amd64/x86_64). Modify the Makefile places that reference NGT_EXTRA_CMAKE_FLAGS (symbol: NGT_EXTRA_CMAKE_FLAGS and flag name NGT_AVX2) to conditionally set or extend the variable inside a GOARCH check (matching the existing amd64/arm64 branch style) so arm64 builds do not get the AVX2 option by default.
881-887:⚠️ Potential issue | 🟠 MajorRemove
sudofrom these install recipes.Hardcoding
sudoat Lines 881, 890, 923, 960, and 993 makes the build targets depend on host-specific privilege setup and breaks minimal builder images that intentionally do not shipsudo. Privilege escalation should stay with the caller; the recipe itself should just run the install step.Suggested cleanup
- sudo $(MAKE) -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build \ + $(MAKE) -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build \ @@ - sudo ldconfig + ldconfig @@ - sudo $(MAKE) -C $(TEMP_DIR)/faiss-$(FAISS_VERSION)/build \ + $(MAKE) -C $(TEMP_DIR)/faiss-$(FAISS_VERSION)/build \ @@ - sudo cmake --install $(TEMP_DIR)/usearch-$(USEARCH_VERSION)/build --prefix=$(USR_LOCAL) + cmake --install $(TEMP_DIR)/usearch-$(USEARCH_VERSION)/build --prefix=$(USR_LOCAL) @@ - sudo $(MAKE) -C $(TEMP_DIR)/CMAKE-$(CMAKE_VERSION)/build \ + $(MAKE) -C $(TEMP_DIR)/CMAKE-$(CMAKE_VERSION)/build \Also applies to: 890-890, 923-929, 960-960, 993-999
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 881 - 887, Remove the hardcoded sudo from the install recipe invocations (e.g., the lines that currently read "sudo $(MAKE) -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build ... install"); change them to run "$(MAKE) -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build ... install" (and the equivalent install invocations for the other libraries/blocks that use the same pattern) so the recipe no longer depends on host-specific privilege tools and leaves privilege escalation to the caller.
179-180:⚠️ Potential issue | 🔴 CriticalSeparate compile flags from link flags.
LDFLAGS_BASEnow carries codegen switches like-march=nativeand-mtune=native, then Lines 863-864 and Line 912 feed$(LDFLAGS)back into CMake compile flags. That bypasses the AVX-512 floor at Lines 188-189 and makes the NGT/Faiss artifacts depend on the builder CPU instead of the deployment baseline. The same flag set also reachescmd/agent/core/ngt/ngtthroughMakefile.d/build.mk:57-60, so this is not isolated to the subbuilds.To confirm the propagation path, inspect the flag definitions and their consumers together:
#!/bin/bash set -eu echo "[Core flag definitions]" sed -n '179,189p' Makefile echo echo "[NGT CMake compile flags]" sed -n '863,864p' Makefile echo echo "[Faiss CMake compile flags]" sed -n '912p' Makefile echo echo "[agent/core/ngt go-build flags]" sed -n '57,60p' Makefile.d/build.mkAlso applies to: 188-189, 863-864, 912-912
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 179 - 180, LDFLAGS_BASE is mixing compiler codegen switches (-march, -mtune, -O3, -ffast-math, etc.) into what should be linker flags (LDFLAGS), causing compile-time vectorization choices to leak into subbuilds; remove all CPU/codegen/optimization flags from LDFLAGS_BASE and put them into the proper compile variables (CFLAGS/CXXFLAGS or a new COMPILE_FLAGS), keep LDFLAGS only for linker-specific flags (ld options and -l flags), then update consumers that currently inject $(LDFLAGS) into CMake/compile invocations (the spots that feed CMake compile flags and the go-build flags in build.mk) to use the new COMPILE_FLAGS or CFLAGS/CXXFLAGS for compilation and only use LDFLAGS at link time so artifacts respect the AVX-512 floor and deployment baseline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dockers/dev/Dockerfile`:
- Around line 31-32: Replace the current base image in the Dockerfile (the FROM
line that currently uses mcr.microsoft.com/devcontainers/base:ubuntu24.04) with
the mandated devcontainer Go image mcr.microsoft.com/vscode/devcontainers/go:1;
keep the existing build environment variables (ENV CC=clang and ENV CXX=clang++)
and any subsequent install steps compatible with the new base image, and remove
or adjust any Ubuntu-specific setup that duplicates what the Go devcontainer
already provides.
In `@Makefile`:
- Around line 179-181: The Makefile currently forces GNU OpenMP causing
runtime/linker mismatches; update LDFLAGS_BASE, LDFLAGS and NGT_LDFLAGS to align
with LLVM OpenMP by replacing any occurrences of -fopenmp=libgomp with
-fopenmp=libomp (or remove the explicit -fopenmp flag so clang chooses the
default) and change the CMake OpenMP variables from DOpenMP_C_LIB_NAMES="gomp"
and DOpenMP_CXX_LIB_NAMES="gomp" to DOpenMP_C_LIB_NAMES="omp" and
DOpenMP_CXX_LIB_NAMES="omp" so the build uses libomp; ensure references to
LDFLAGS_BASE, LDFLAGS and NGT_LDFLAGS are updated consistently.
In `@Makefile.d/functions.mk`:
- Around line 141-153: The go-e2e-build target sets CGO_CXXFLAGS="$2" and
CGO_FFLAGS="$2" while go-build sets CGO_CXXFLAGS="$(CXXFLAGS) -flto=thin
-DNGT_LARGE_DATASET", causing e2e binaries to miss thin-LTO and NGT flags;
update the go-e2e-build variable assignments (CGO_CXXFLAGS and CGO_FFLAGS) to
follow the same pattern as go-build (include $(CXXFLAGS) -flto=thin
-DNGT_LARGE_DATASET or the equivalent HDF5/NGT flags) or add a comment near
go-e2e-build documenting the intentional omission if e2e should differ, ensuring
the change references CGO_CXXFLAGS, CGO_FFLAGS, go-e2e-build and go-build so
reviewers can locate it.
In `@Makefile.d/proto.mk`:
- Line 61: The make invocation is passing toolchain variables (CC, CXX, AR, NM,
RANLIB) to the proto/replace target even though proto/replace only runs sed on
generated .go files; remove those unnecessary variable assignments from the
$(MAKE) call so it becomes a plain recursive make to proto/replace (refer to the
$(MAKE) invocation and the proto/replace target name) to improve clarity while
leaving behavior unchanged.
---
Outside diff comments:
In `@dockers/agent/core/agent/Dockerfile`:
- Around line 68-83: The Dockerfile package list contains duplicate entries for
the packages "clang" and "lld"; remove the second occurrences (the trailing
"clang" and "lld" in the same apt install list) so each package appears only
once, keeping the original first instances and deleting the duplicate tokens to
avoid redundant installs and unnecessary layer/cache inflation.
---
Duplicate comments:
In `@go.mod`:
- Around line 114-122: The go.mod shows a version skew: the require entries for
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc and
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc are pinned to
v1.38.0 while replace forces v1.43.0; update the require lines for those two
packages to v1.43.0 (or change the replace to match, but prefer updating
require) so require and replace agree, then run go mod tidy to refresh module
files.
In `@Makefile`:
- Line 65: The Makefile currently sets NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON
unconditionally; change this so the AVX2 flag is only enabled for x86 builds by
gating it on GOARCH (e.g., leave NGT_EXTRA_CMAKE_FLAGS empty by default and
append or set -DNGT_AVX2=ON only when GOARCH is amd64/x86_64). Modify the
Makefile places that reference NGT_EXTRA_CMAKE_FLAGS (symbol:
NGT_EXTRA_CMAKE_FLAGS and flag name NGT_AVX2) to conditionally set or extend the
variable inside a GOARCH check (matching the existing amd64/arm64 branch style)
so arm64 builds do not get the AVX2 option by default.
- Around line 881-887: Remove the hardcoded sudo from the install recipe
invocations (e.g., the lines that currently read "sudo $(MAKE) -C
$(TEMP_DIR)/NGT-$(NGT_VERSION)/build ... install"); change them to run "$(MAKE)
-C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build ... install" (and the equivalent install
invocations for the other libraries/blocks that use the same pattern) so the
recipe no longer depends on host-specific privilege tools and leaves privilege
escalation to the caller.
- Around line 179-180: LDFLAGS_BASE is mixing compiler codegen switches (-march,
-mtune, -O3, -ffast-math, etc.) into what should be linker flags (LDFLAGS),
causing compile-time vectorization choices to leak into subbuilds; remove all
CPU/codegen/optimization flags from LDFLAGS_BASE and put them into the proper
compile variables (CFLAGS/CXXFLAGS or a new COMPILE_FLAGS), keep LDFLAGS only
for linker-specific flags (ld options and -l flags), then update consumers that
currently inject $(LDFLAGS) into CMake/compile invocations (the spots that feed
CMake compile flags and the go-build flags in build.mk) to use the new
COMPILE_FLAGS or CFLAGS/CXXFLAGS for compilation and only use LDFLAGS at link
time so artifacts respect the AVX-512 floor and deployment baseline.
In `@rust/.cargo/config.toml`:
- Around line 18-22: The rustflags entries in rust/.cargo/config.toml currently
include "-C", "target-cpu=native" which bakes host-specific CPU tuning into
release artifacts; remove "target-cpu=native" from both the global rustflags and
the [target.aarch64-unknown-linux-gnu] rustflags, and instead document or
default to a stable baseline (e.g., omit CPU tuning or use a conservative target
like "x86-64" / platform-appropriate option) and recommend making CPU tuning
opt-in via environment-specific RUSTFLAGS so host builds remain portable; update
the rustflags lines and add a short comment explaining to set RUSTFLAGS
externally for performance tuning.
In `@rust/libs/kvs/Cargo.toml`:
- Line 27: The tokio dependency in the kvs crate uses a two-part semver ("1.51")
while other workspace crates use the explicit three-part format; update the
tokio entry in the kvs Cargo.toml (the tokio = { version = "1.51", features =
["full"] } line) to use "1.51.0" so it matches the rest of the workspace and
ensures deterministic patch resolution.
🪄 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: 46e7f363-d1b3-4172-930f-4c068427c7eb
⛔ Files ignored due to path filters (8)
example/client/go.sumis excluded by!**/*.sum,!**/go.sumgo.sumis excluded by!**/*.sum,!**/go.sumhack/docker/gen/main.gois excluded by!**/gen/**rust/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lockrust/bin/agent/Cargo.tomlis excluded by!**/bin/**rust/bin/meta/Cargo.tomlis excluded by!**/bin/**versions/PROMETHEUS_STACK_VERSIONis excluded by!versions/**versions/actions/DOCKER_LOGIN_ACTIONis excluded by!versions/**
📒 Files selected for processing (36)
.gitfiles.github/workflows/_docker-image.yamlMakefileMakefile.d/actions.mkMakefile.d/bench.mkMakefile.d/build.mkMakefile.d/dependencies.mkMakefile.d/docker.mkMakefile.d/e2e.mkMakefile.d/functions.mkMakefile.d/git.mkMakefile.d/helm.mkMakefile.d/k0s.mkMakefile.d/k3d.mkMakefile.d/k8s.mkMakefile.d/kind.mkMakefile.d/minikube.mkMakefile.d/proto.mkMakefile.d/test.mkMakefile.d/tls.mkMakefile.d/tools.mkdockers/agent/core/agent/Dockerfiledockers/agent/core/faiss/Dockerfiledockers/agent/core/ngt/Dockerfiledockers/dev/Dockerfiledockers/example/client/Dockerfiledockers/tests/v2/e2e/Dockerfiledockers/tools/benchmark/job/Dockerfileexample/client/go.modgo.modinternal/core/algorithm/ngt/Makefileinternal/core/algorithm/ngt/ngt.gointernal/core/algorithm/ngt/option.gorust/.cargo/config.tomlrust/libs/kvs/Cargo.tomlrust/libs/observability/Cargo.toml
| ENV CC=clang | ||
| ENV CXX=clang++ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current base image line:"
rg -n '^FROM ' dockers/dev/Dockerfile
echo
echo "Check required base image match:"
rg -n '^FROM mcr\.microsoft\.com/vscode/devcontainers/go:1$' dockers/dev/DockerfileRepository: vdaas/vald
Length of output: 169
Use the mandated devcontainer Go base image for this file.
The Dockerfile uses mcr.microsoft.com/devcontainers/base:ubuntu24.04 (line 22) instead of the required mcr.microsoft.com/vscode/devcontainers/go:1 base image specified in the development container guidelines.
🔧 Proposed fix
-FROM mcr.microsoft.com/devcontainers/base:ubuntu24.04
+FROM mcr.microsoft.com/vscode/devcontainers/go:1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dockers/dev/Dockerfile` around lines 31 - 32, Replace the current base image
in the Dockerfile (the FROM line that currently uses
mcr.microsoft.com/devcontainers/base:ubuntu24.04) with the mandated devcontainer
Go image mcr.microsoft.com/vscode/devcontainers/go:1; keep the existing build
environment variables (ENV CC=clang and ENV CXX=clang++) and any subsequent
install steps compatible with the new base image, and remove or adjust any
Ubuntu-specific setup that duplicates what the Go devcontainer already provides.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
Makefile (3)
65-65:⚠️ Potential issue | 🟠 MajorMake AVX2 opt-in by architecture.
-DNGT_AVX2=ONis unconditional, so non-amd64 builds can be forced into an invalid SIMD config.Suggested fix
-NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON +ifeq ($(GOARCH),amd64) +NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON +else +NGT_EXTRA_CMAKE_FLAGS ?= +endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 65, The Makefile currently sets NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON unconditionally; change this to set the AVX2 flag only when building on an x86_64/amd64 architecture by detecting the host architecture (e.g., via shell uname -m or an existing ARCH/GOARCH variable) and appending -DNGT_AVX2=ON to NGT_EXTRA_CMAKE_FLAGS only in that branch; update references to NGT_EXTRA_CMAKE_FLAGS so non-amd64 builds keep the variable empty (or default) and are not forced into an invalid SIMD config.
886-895:⚠️ Potential issue | 🟠 MajorAvoid
sudoinside build/install targets.These targets now require
sudo, which is often unavailable in CI/minimal containers and breaks reproducible automation flows.Suggested fix
- sudo $(MAKE) -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build \ + $(MAKE) -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build \ ... - sudo ldconfig + ldconfig- sudo $(MAKE) -C $(TEMP_DIR)/faiss-$(FAISS_VERSION)/build \ + $(MAKE) -C $(TEMP_DIR)/faiss-$(FAISS_VERSION)/build \ ... - sudo cmake --install $(TEMP_DIR)/usearch-$(USEARCH_VERSION)/build --prefix=$(USR_LOCAL) + cmake --install $(TEMP_DIR)/usearch-$(USEARCH_VERSION)/build --prefix=$(USR_LOCAL)- sudo $(MAKE) -C $(TEMP_DIR)/CMAKE-$(CMAKE_VERSION)/build \ + $(MAKE) -C $(TEMP_DIR)/CMAKE-$(CMAKE_VERSION)/build \Also applies to: 928-934, 965-965, 998-1004
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 886 - 895, Remove hardcoded sudo in Makefile install lines by replacing direct "sudo" invocations with a configurable variable (e.g., $(SUDO)) and default it to empty; update the commands that call "$(MAKE) -C $(TEMP_DIR)/NGT-$(NGT_VERSION)/build ..." and the "ldconfig" invocation to use $(SUDO) (or remove entirely) so CI/minimal containers won’t require root—ensure you add a top-level "SUDO ?=" definition (empty by default) so callers can opt-in to sudo when needed.
180-183:⚠️ Potential issue | 🟠 MajorOpenMP runtime choice is inconsistent (
libgompvslibomp).The main linker/CMake path still forces GNU OpenMP (
gomp), while the new target builds LLVM OpenMP (libomp.a). This split can cause link/runtime instability and undermines the intended fix path.Also applies to: 873-876, 1009-1033
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 180 - 183, The OpenMP runtime is inconsistent across variables (LDFLAGS_BASE, LDFLAGS, NGT_LDFLAGS, FAISS_LDFLAGS): some use GNU libgomp (-fopenmp=libgomp / -lgomp) while other targets build/link LLVM OpenMP (libomp). Pick a single runtime (preferably libomp for the LLVM path) and make the flags consistent by replacing all occurrences of -fopenmp=libgomp and any -lgomp with the chosen one (e.g., -fopenmp=libomp and the appropriate linker flag for libomp), and ensure the static linking options still work with the chosen runtime across LDFLAGS_BASE, LDFLAGS, NGT_LDFLAGS and FAISS_LDFLAGS so all targets link the same OpenMP runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 868-869: The Makefile currently injects $(LDFLAGS) into compile
flags (DCMAKE_C_FLAGS and DCMAKE_CXX_FLAGS) for the NGT and FAISS builds; remove
$(LDFLAGS) from DCMAKE_C_FLAGS and DCMAKE_CXX_FLAGS and instead pass linker-only
flags via DCMAKE_EXE_LINKER_FLAGS (and/or DCMAKE_SHARED_LINKER_FLAGS) so linker
options like -fuse-ld=lld, -z relro, -z now, and -Wl,... are not used at compile
time; update the NGT-related CMake invocation that sets
CMAKE_C_FLAGS/CMAKE_CXX_FLAGS and the FAISS section (where CMAKE_C_FLAGS
currently equals only $(LDFLAGS)) to move $(LDFLAGS) into the appropriate linker
flag variables while keeping actual compile flags in
CMAKE_C_FLAGS/CMAKE_CXX_FLAGS.
In `@Makefile.d/dependencies.mk`:
- Around line 258-263: The update/llvm-openmp make target uses curl piped to
grep -Po which is non‑portable; replace the grep pipeline with jq to extract and
strip the "llvmorg-" prefix from the JSON tag_name (e.g. use jq -r '.tag_name |
sub("^llvmorg-"; "")') and write that output to the same
versions/LLVM_OPENMP_VERSION file so the target remains portable and consistent
with other JSON parsing in the makefile.
---
Duplicate comments:
In `@Makefile`:
- Line 65: The Makefile currently sets NGT_EXTRA_CMAKE_FLAGS ?= -DNGT_AVX2=ON
unconditionally; change this to set the AVX2 flag only when building on an
x86_64/amd64 architecture by detecting the host architecture (e.g., via shell
uname -m or an existing ARCH/GOARCH variable) and appending -DNGT_AVX2=ON to
NGT_EXTRA_CMAKE_FLAGS only in that branch; update references to
NGT_EXTRA_CMAKE_FLAGS so non-amd64 builds keep the variable empty (or default)
and are not forced into an invalid SIMD config.
- Around line 886-895: Remove hardcoded sudo in Makefile install lines by
replacing direct "sudo" invocations with a configurable variable (e.g., $(SUDO))
and default it to empty; update the commands that call "$(MAKE) -C
$(TEMP_DIR)/NGT-$(NGT_VERSION)/build ..." and the "ldconfig" invocation to use
$(SUDO) (or remove entirely) so CI/minimal containers won’t require root—ensure
you add a top-level "SUDO ?=" definition (empty by default) so callers can
opt-in to sudo when needed.
- Around line 180-183: The OpenMP runtime is inconsistent across variables
(LDFLAGS_BASE, LDFLAGS, NGT_LDFLAGS, FAISS_LDFLAGS): some use GNU libgomp
(-fopenmp=libgomp / -lgomp) while other targets build/link LLVM OpenMP (libomp).
Pick a single runtime (preferably libomp for the LLVM path) and make the flags
consistent by replacing all occurrences of -fopenmp=libgomp and any -lgomp with
the chosen one (e.g., -fopenmp=libomp and the appropriate linker flag for
libomp), and ensure the static linking options still work with the chosen
runtime across LDFLAGS_BASE, LDFLAGS, NGT_LDFLAGS and FAISS_LDFLAGS so all
targets link the same OpenMP runtime.
🪄 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: 2193f3f4-728e-43b2-95d4-9e3f5ddcee92
⛔ Files ignored due to path filters (1)
versions/LLVM_OPENMP_VERSIONis excluded by!versions/**
📒 Files selected for processing (2)
MakefileMakefile.d/dependencies.mk
b08b751 to
04f6daa
Compare
f678914 to
e9d89f2
Compare
6f7120e to
40d3fe4
Compare
40d3fe4 to
f5cb038
Compare
f5cb038 to
4c73001
Compare
|
[WARNING:INTCFG] Changes in |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3520 +/- ##
==========================================
- Coverage 27.06% 26.33% -0.73%
==========================================
Files 527 571 +44
Lines 49064 50486 +1422
==========================================
+ Hits 13280 13298 +18
- Misses 34802 36206 +1404
Partials 982 982 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: kpango <kpango@vdaas.org>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| k8s/agent/clusterrolebinding.yaml | ||
| k8s/agent/daemonset.yaml | ||
| k8s/agent/deployment.yaml | ||
| k8s/agent/faiss/configmap.yaml |
There was a problem hiding this comment.
[file name cspell] reported by reviewdog 🐶
Unknown word (typedarray) Suggestions: []
| k8s/discoverer/daemonset.yaml | ||
| k8s/discoverer/deployment.yaml | ||
| k8s/discoverer/hpa.yaml | ||
| k8s/discoverer/networkpolicy.yaml |
There was a problem hiding this comment.
[file name cspell] reported by reviewdog 🐶
Unknown word (importmeta) Suggestions: [iupoutmeta, iUpoutmeta, iupoutMeta, iupOutmeta, iUpoutMeta]
| k8s/index/job/deletion/cronjob.yaml | ||
| k8s/index/job/deletion/networkpolicy.yaml | ||
| k8s/index/job/deletion/serviceaccount.yaml | ||
| k8s/index/job/readreplica/rotate/clusterrole.yaml |
There was a problem hiding this comment.
[file name cspell] reported by reviewdog 🐶
Unknown word (runkit) Suggestions: [reknit, runt, runic, ranket, rankin]
| rust/Cargo.lock | ||
| rust/Cargo.toml | ||
| rust/bin/agent/Cargo.toml | ||
| rust/bin/agent/build.rs |
There was a problem hiding this comment.
[file name cspell] reported by reviewdog 🐶
Unknown word (arraybuffer) Suggestions: []
| rust/Cargo.toml | ||
| rust/bin/agent/Cargo.toml | ||
| rust/bin/agent/build.rs | ||
| rust/bin/agent/src/glibc_compat.c |
There was a problem hiding this comment.
[file name cspell] reported by reviewdog 🐶
Unknown word (arraybuffer) Suggestions: []
| rust/Cargo.toml | ||
| rust/bin/agent/Cargo.toml | ||
| rust/bin/agent/build.rs | ||
| rust/bin/agent/src/glibc_compat.c |
There was a problem hiding this comment.
[file name cspell] reported by reviewdog 🐶
Unknown word (typedarray) Suggestions: []
Docker builds fail because
LDFLAGS(containing-static,-Wl,--whole-archive,-fuse-ld=lld, etc.) was passed intoCMAKE_C_FLAGS/CMAKE_CXX_FLAGS, causing cmake compiler checks to fail. Additionally,export CC CXX AR NM RANLIB LDFLAGS CFLAGS CXXFLAGSpolluted subprocess environments, breakingcargo install(ring crate) in format CI where clang isn't installed.cmake flag separation
CMAKE_C_FLAGSnow gets only compile flags ($(CFLAGS) -flto=thin), linker flags go toCMAKE_EXE_LINKER_FLAGS-DOpenMP_*_LIB_NAMES="gomp"— let cmake's FindOpenMP handle discoverytoolchain variable handling
CC ?= clanginstead ofCC = clang(and CXX/AR/NM/RANLIB) — allows CI environments to overrideexport SUDOonly — removed global export of CC/CXX/AR/NM/RANLIB/LDFLAGS/CFLAGS/CXXFLAGS; these are already passed explicitly per-command ingo-build, cmake invocations, etc.portability
-fopenmpinstead of-fopenmp=libgomp(toolchain-agnostic, works with both GCC and clang)NGT_EXTRA_CMAKE_FLAGS ?=empty default instead of-DNGT_AVX2=ONtarget-cpu=nativefromrust/.cargo/config.toml