From e5236ed498d64df3b878671fb3c355f41f12d1cc Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 27 Jul 2026 04:39:32 +0000 Subject: [PATCH] backend(audio-cpp): make the Darwin fallbacks loud and the rpath walk complete Review follow-up on the Darwin Metal build. The OpenMP fallback was silent. If brew --prefix libomp ever comes back empty, CI produced a green Metal package with 108 #pragma omp directives across ~30 files compiled out, and clang says nothing about an ignored omp pragma without -Wsource-uses-openmp, so the only trace was one absent flag inside a set -x cmake line. That regression would have been blamed on Metal. It now warns. The @rpath arm of the dylib walk had no live candidate when it was written, on the reasoning that a Metal build links ggml statically. The OpenMP fix in the same commit made libomp.dylib one, and whether Homebrew records it as an absolute opt path or as @rpath/libomp.dylib is not observable from Linux. The walk now expands @rpath, @loader_path and @executable_path against the object's own LC_RPATH entries, and only fails when nothing on disk answers, printing the rpath list with the error so a failure on a machine nobody can attach to explains itself. Also: ADDITIONAL_LIBS now go through the closure rather than a bare cp, so they are deduplicated and their own dependencies bundled; build/darwin/lib is created explicitly instead of relying on package.sh pre-creating it; the libomp probe uses nested ifneq rather than $(and ...), which needs GNU make 3.81 and would otherwise expand empty and take the OFF branch on an older make; and -DOpenMP_ROOT is quoted like its CUDA sibling. Verified with a Linux harness that runs the script verbatim against a stubbed otool: a level-2 transitive dep, an @rpath dep reachable only through LC_RPATH, and an ADDITIONAL_LIBS dep are all bundled, a dependency cycle terminates, system libraries are skipped, the packaged tree has assets/ at the root beside grpc-server with the dylibs in lib/, and both failure paths exit non-zero. Assisted-by: Claude:claude-opus-5 [Claude Code] Signed-off-by: Ettore Di Giacinto --- backend/cpp/audio-cpp/Makefile | 30 +++++-- scripts/build/audio-cpp-darwin.sh | 133 +++++++++++++++++++++++------- 2 files changed, 125 insertions(+), 38 deletions(-) diff --git a/backend/cpp/audio-cpp/Makefile b/backend/cpp/audio-cpp/Makefile index 9b29cdc7d..30c88374c 100644 --- a/backend/cpp/audio-cpp/Makefile +++ b/backend/cpp/audio-cpp/Makefile @@ -68,8 +68,10 @@ else ifeq ($(CUDA_MAJOR_VERSION),12) else ifeq ($(CUDA_MAJOR_VERSION),) CUDA_ARCHITECTURES ?= native else ifeq ($(BUILD_TYPE),cublas) - # Gated on cublas so that `make clean` with a stray CUDA_MAJOR_VERSION in the - # environment still works; the variable means nothing to any other build. + # Gated on cublas because the variable means nothing to any other build, so a + # stray CUDA_MAJOR_VERSION in the environment must not break `make clean` or + # a CPU build. It does still error for `BUILD_TYPE=cublas make clean`, which + # is the right trade: that invocation is asking about a CUDA build tree. $(error CUDA_MAJOR_VERSION=$(CUDA_MAJOR_VERSION) has no architecture list here (12 and 13 do). Leave it empty for a native build, or pass CUDA_ARCHITECTURES explicitly.) endif @@ -94,14 +96,30 @@ else ifeq ($(UNAME_S),Darwin) # # If the keg is absent, turn OpenMP off rather than fail: the tree's only # include is guarded by #ifdef _OPENMP and a #pragma omp without - # -fopenmp is simply ignored, so an OpenMP-less build is correct, just - # single-threaded in the host-side DSP loops. + # -fopenmp is simply ignored, so an OpenMP-less build is CORRECT. It is not + # cheap, though: 108 `#pragma omp` directives across ~30 files (roformer, + # demucs, chatterbox, moss, supertonic, seed_vc, framework/audio/dsp) are + # compiled out, and clang says nothing about an ignored omp pragma unless + # -Wsource-uses-openmp is on. A green package that is quietly single-threaded + # in every host DSP loop gets blamed on Metal, not on packaging, so the + # fallback announces itself. ifeq ($(origin LIBOMP_PREFIX),undefined) LIBOMP_PREFIX := $(shell brew --prefix libomp 2>/dev/null) endif - ifneq ($(and $(wildcard $(LIBOMP_PREFIX)/lib/libomp.dylib),$(wildcard $(LIBOMP_PREFIX)/include/omp.h)),) - CMAKE_ARGS += -DOpenMP_ROOT=$(LIBOMP_PREFIX) + # Nested ifneq rather than $(and ...): $(and) needs GNU make 3.81, and while + # that is what Apple ships, an older make expands it to empty and would take + # the OpenMP-OFF branch with no way to tell that from a genuinely missing + # keg. Two plain conditionals cannot fail that way. + LIBOMP_USABLE := + ifneq ($(wildcard $(LIBOMP_PREFIX)/lib/libomp.dylib),) + ifneq ($(wildcard $(LIBOMP_PREFIX)/include/omp.h),) + LIBOMP_USABLE := yes + endif + endif + ifeq ($(LIBOMP_USABLE),yes) + CMAKE_ARGS += "-DOpenMP_ROOT=$(LIBOMP_PREFIX)" else + $(warning audio-cpp: libomp not found at '$(LIBOMP_PREFIX)'; building without OpenMP (single-threaded host DSP). Install it with `brew install libomp`, or set LIBOMP_PREFIX.) CMAKE_ARGS += -DENGINE_ENABLE_OPENMP=OFF endif else diff --git a/scripts/build/audio-cpp-darwin.sh b/scripts/build/audio-cpp-darwin.sh index b5badccf2..937005393 100755 --- a/scripts/build/audio-cpp-darwin.sh +++ b/scripts/build/audio-cpp-darwin.sh @@ -40,6 +40,11 @@ mkdir -p build/darwin backend-images # (libggml.dylib -> libggml.0.dylib -> ...), and the SONAME the binary asks for # has to keep naming a file here. cp -a backend/cpp/audio-cpp/package/. build/darwin/ +# lib/ arrives from package.sh, which mkdir -p's an empty one. Do not rely on +# that: everything below copies into it, and if package.sh ever stops +# pre-creating it the first copy fails with an opaque BSD-cp message instead of +# any of the assertions this script otherwise leans on. +mkdir -p build/darwin/lib # THE LAYOUT ASSERTION - the Darwin half of the one in # backend/cpp/audio-cpp/package.sh, and the reason that one is written at such @@ -63,17 +68,6 @@ for required in grpc-server run.sh assets/silero_vad assets/marblenet_vad; do fi done -# Apple Silicon: pick up Homebrew-installed protobuf utf8_validity if present -# (same as ds4-darwin.sh - it is a transitive dep otool may not surface). -if [[ "$(uname -s)" == "Darwin" && "$(uname -m)" == "arm64" ]]; then - ADDITIONAL_LIBS=${ADDITIONAL_LIBS:-$(ls /opt/homebrew/Cellar/protobuf/**/lib/libutf8_validity*.dylib 2>/dev/null || true)} -else - ADDITIONAL_LIBS=${ADDITIONAL_LIBS:-""} -fi -for file in $ADDITIONAL_LIBS; do - cp -fLv "$file" build/darwin/lib -done - # Bundle the FULL dylib closure, not just grpc-server's direct dependencies. # # ds4-darwin.sh and llama-cpp-darwin.sh walk one level. That is enough only @@ -94,6 +88,72 @@ done BUNDLED_LEAVES=" " UNRESOLVED=0 +# The LC_RPATH entries recorded in an object, one per line. +object_rpaths() { + otool -l "$1" | awk '/LC_RPATH/ { f = 1 } f && $1 == "path" { print $2; f = 0 }' +} + +# Resolve an @rpath / @loader_path / @executable_path dependency to a real file, +# printing the path and returning 0, or returning 1 if nothing matches. +# +# This is not hypothetical any more. The Metal build links ggml statically, so +# no ggml object arrives as an @rpath dep, but the OpenMP hint in the backend +# Makefile makes libomp.dylib a dependency, and whether it is recorded as an +# absolute /opt/homebrew/opt/libomp path or as @rpath/libomp.dylib is a property +# of Homebrew's fix_dynamic_linkage that cannot be observed from Linux. Skipping +# it (what ds4-darwin.sh does with a non-existent path) would ship a package +# missing its OpenMP runtime; failing without expanding rpaths first would turn +# a resolvable dependency into a red build. +resolve_at_path_dep() { + local object="$1" dep="$2" + local objdir rpath candidate + objdir=$(dirname "$object") + case "$dep" in + @loader_path/*) + candidate="$objdir/${dep#@loader_path/}" + if [ -e "$candidate" ]; then printf '%s\n' "$candidate"; return 0; fi + ;; + @executable_path/*) + # At run time the executable is grpc-server at the package root. + candidate="build/darwin/${dep#@executable_path/}" + if [ -e "$candidate" ]; then printf '%s\n' "$candidate"; return 0; fi + ;; + @rpath/*) + while read -r rpath; do + case "$rpath" in + @loader_path) rpath="$objdir" ;; + @loader_path/*) rpath="$objdir/${rpath#@loader_path/}" ;; + @executable_path) rpath="build/darwin" ;; + @executable_path/*) rpath="build/darwin/${rpath#@executable_path/}" ;; + esac + candidate="$rpath/${dep#@rpath/}" + if [ -e "$candidate" ]; then printf '%s\n' "$candidate"; return 0; fi + done < <(object_rpaths "$object") + ;; + esac + return 1 +} + +# Copy one dylib into lib/ and walk its own dependencies. A no-op if a file of +# the same leaf name is already bundled, which is both the deduplication and the +# cycle guard: mutual dependencies terminate because the second visit finds the +# leaf already recorded. +bundle_dylib() { + local path="$1" + local leaf + leaf=$(basename "$path") + case "$BUNDLED_LEAVES" in + *" $leaf "*) return 0 ;; + esac + BUNDLED_LEAVES="${BUNDLED_LEAVES}${leaf} " + # -L dereferences: Homebrew's unversioned names are symlinks into the + # Cellar, and copying the link would land a dangling relative symlink. The + # binary asks for whatever install name is recorded, so the file has to + # exist under that leaf. + cp -fLv "$path" build/darwin/lib/ + bundle_dylib_closure "$path" +} + bundle_dylib_closure() { local object="$1" local dep leaf src @@ -107,27 +167,28 @@ bundle_dylib_closure() { *" $leaf "*) continue ;; esac - src="$dep" case "$dep" in @*) - # @rpath / @loader_path / @executable_path are resolved against - # the package itself, so such a dependency is satisfiable only - # if the file is already inside it. Nothing produces one today - # (a Metal build sets neither BUILD_SHARED_LIBS nor - # GGML_BACKEND_DL, so ggml is static and package.sh copies no - # dylibs at all), but if that changes, a missing one has to be - # loud: dyld fails at load with nothing else to go on. + # Already inside the package (the ggml objects package.sh copies + # to the root would land here): record and walk in place. if [ -e "build/darwin/$leaf" ]; then - src="build/darwin/$leaf" + BUNDLED_LEAVES="${BUNDLED_LEAVES}${leaf} " + bundle_dylib_closure "build/darwin/$leaf" elif [ -e "build/darwin/lib/$leaf" ]; then - src="build/darwin/lib/$leaf" + BUNDLED_LEAVES="${BUNDLED_LEAVES}${leaf} " + bundle_dylib_closure "build/darwin/lib/$leaf" + elif src=$(resolve_at_path_dep "$object" "$dep"); then + bundle_dylib "$src" else - echo "audio-cpp-darwin.sh: unresolved @rpath dependency of ${object}: ${dep}" >&2 + # Nothing on disk answers this, so dyld will not find it + # either. Print the rpath list with it: this fails on a + # machine nobody can attach to, and without the rpaths the + # log says only that something was missing. + echo "audio-cpp-darwin.sh: unresolved dependency of ${object}: ${dep}" >&2 + echo "audio-cpp-darwin.sh: LC_RPATH entries of ${object}:" >&2 + object_rpaths "$object" | sed 's/^/audio-cpp-darwin.sh: /' >&2 UNRESOLVED=1 - continue fi - BUNDLED_LEAVES="${BUNDLED_LEAVES}${leaf} " - bundle_dylib_closure "$src" continue ;; esac @@ -137,19 +198,27 @@ bundle_dylib_closure() { # frameworks out of the package; there is no allow-list to maintain. [ -e "$dep" ] || continue - BUNDLED_LEAVES="${BUNDLED_LEAVES}${leaf} " - # -L dereferences: Homebrew's unversioned names are symlinks into the - # Cellar, and copying the link would land a dangling relative symlink. - # The binary asks for whatever install name is recorded, so the file has - # to exist under that leaf. - cp -fLv "$dep" build/darwin/lib/ - bundle_dylib_closure "$dep" + bundle_dylib "$dep" done < <(otool -L "$object" | awk 'NR > 1 { print $1 }') # Explicit, because `set -e` would abort on whatever status the loop # happened to end on. return 0 } +# Apple Silicon: pick up Homebrew-installed protobuf utf8_validity if present +# (same as ds4-darwin.sh - it is a transitive dep otool may not surface). +# Unlike ds4-darwin.sh these go through bundle_dylib rather than a bare cp, so +# they are seeded into the seen-set (no duplicate copy when the walk reaches +# them) and their own non-system dependencies are bundled too. +if [[ "$(uname -s)" == "Darwin" && "$(uname -m)" == "arm64" ]]; then + ADDITIONAL_LIBS=${ADDITIONAL_LIBS:-$(ls /opt/homebrew/Cellar/protobuf/**/lib/libutf8_validity*.dylib 2>/dev/null || true)} +else + ADDITIONAL_LIBS=${ADDITIONAL_LIBS:-""} +fi +for file in $ADDITIONAL_LIBS; do + bundle_dylib "$file" +done + bundle_dylib_closure build/darwin/grpc-server if compgen -G "build/darwin/*.dylib" > /dev/null; then for object in build/darwin/*.dylib; do