cmake: mark the Homebrew include path SYSTEM so it cannot shadow vendored headers - #5399
Conversation
…ored headers
On macOS with Homebrew's ggml installed, a default build fails:
third_party/whisper.cpp/ggml/src/ggml.c:684:6:
error: use of undeclared identifier 'GGML_TYPE_Q2_0'
plus three more in ggml-quants.c. ggml.c includes "ggml.h" with quotes,
which searches the including file's own directory and then the -I list.
CMakeLists.txt:84 put /opt/homebrew/include at the head of that list for
every target in the project, so the compiler picked up Homebrew's ggml
0.15.2 header instead of the vendored one the rest of the vendored
sources were written against. GGML_TYPE_Q2_0 is declared in the vendored
header (ggml/include/ggml.h:432) and does not exist in 0.15.2.
The defect in one sentence: the build deliberately chooses the vendored
copy — USE_SYSTEM_LIBWHISPER defaults OFF — and the include path
silently reverses that choice.
A directory-scoped include_directories() at the top level is inherited
by every add_subdirectory() and INITIALISES each target's
INCLUDE_DIRECTORIES, so it lands ahead of anything a target adds for
itself. SYSTEM emits -isystem, which the compiler searches after all -I
paths: Homebrew stays findable for everything that needs it, and stops
outranking the tree's own headers.
Confirmed with the compiler rather than by reading. cc -H on ggml.c
resolves /opt/homebrew/include/ggml.h before this change and
third_party/whisper.cpp/ggml/src/../include/ggml.h after it, and the
error count goes 4 -> 0 on that file.
This is a class, not an instance. 21 vendored public headers share a
basename with one in /opt/homebrew/include on a stock dev machine —
ggml's family, gguf.h, parakeet.h, and notably whisper.h and fftw3.h.
Only ggml.h fails loudly; the others compile because the versions happen
to agree closely enough, which is the dangerous case: a skew in any of
them mismatches silently and shows up as wrong numbers rather than an
error. The set also changes with whatever the developer brew-installs
next, so a build that works today breaks on a machine differing only by
an unrelated package.
Scope: macOS only, inside the existing if(APPLE). No behaviour change on
Linux or Windows, and none on macOS beyond include precedence and
third-party warning suppression.
NOT YET COMPLETE: the full ENABLE_ASR-default build that proves this is
paused partway (see hl2-lab pr/ggml-include-order.md). Committed now so
the paused build's premise is recorded rather than living in an
uncommitted working tree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SvGM4eqSng7aX62aVCFyh
Ozy311
left a comment
There was a problem hiding this comment.
Issue fit
There is no linked issue. Against the PR's own stated intent, yes: the top-level plain Homebrew include directory is inherited ahead of target-local vendored directories, while marking it SYSTEM moves it behind ordinary -I directories without making Homebrew-only headers unavailable. This is a bug fix with a clear root cause, not an architectural change or dependency addition, so the absence of an RFC is consistent with GOVERNANCE.md. I could not reproduce the exact ggml 0.15.2 collision because this Mac does not currently have Homebrew ggml/whisper headers installed, but I reproduced the load-bearing CMake/AppleClang ordering behavior directly.
Scope
| File | What it changes | Claimed by title/body? | Verdict |
|---|---|---|---|
CMakeLists.txt |
Marks the existing Apple/Homebrew include path SYSTEM and documents why |
Yes | In scope; one comment nit below |
The semantic change is one keyword inside the existing if(APPLE) block. No Linux/Windows behavior, dependency selection, find_package, CI image, vendored source, test, UI, settings, protocol, or CHANGELOG.md surface changes.
Blockers
None.
Nits
- Non-blocking: the added source comment says
fftw3.his shadowed on a stock Mac, while the current PR body correctly demonstrates the opposite: the vendored FFTW include path is Windows-only and macOS intentionally resolves FFTW from its installed package. Inline atCMakeLists.txt:91; make the permanent comment agree with the corrected analysis. - Non-blocking: the PR body still contains an “orchestrator, not for the PR body” section saying the full build is not yet done, while its Testing section now claims the 3102-object full build passed. Removing or updating that stale internal note would leave one unambiguous evidence record. The commit message's historical state can remain historical.
What I tried to break
- Built a minimal CMake 3.25-style parent/child project on this arm64 Mac with AppleClang 21. Without
SYSTEM, CMake emitted the inherited directory first (-I.../system -I.../vendor/include) and the compile failed on the wrong collision header. With the PR's keyword, it emitted-I.../vendor/include -isystem .../system; the vendored header won and a second header available only in the system directory still compiled. This verifies both load-bearing claims independently of the PR's prose. - Read the actual vendored graph:
USE_SYSTEM_LIBWHISPERdefaults OFF; the vendored tree is added withadd_subdirectory;ggml-baseaddsggml/includeafter the inherited root property;ggml.candwhisper.cppuse quoted basename includes. The affected reachability matches the reproduced CMake shape. - Checked the platform boundary: the only hunk is under
if(APPLE), andinclude_directories(SYSTEM ...)changes compile classification/order, not CMake's package-discovery paths. Homebrew-only headers remain searchable in the empirical probe. - Compared merge base
10a847b7with current main0ad8ca80. One intervening sanitizer commit touchesCMakeLists.txtin other regions; the Homebrew block is unchanged, GitHub reports MERGEABLE, andgit diff --checkis clean. - Current machine evidence:
arm64, Homebrew prefix/opt/homebrew; no installedggml,whisper-cpp,ggml*.h,whisper.h, orfftw3.hwas found under that prefix. Per the review boundary I installed nothing, so the author's exact 7-error before/full-build-after corpus remains unverified here. The commit signature is valid and this head has no CI runs. No GUI, hardware, sockets, or app build was used.
Recommendation
Approve with nits. The one-line fix survives the include-order, inheritance, system-header-findability, platform-scope, and current-main overlap attacks. Correct the fftw3.h sentence when convenient and clean the stale PR-body note; neither changes the correctness of the build fix. Normal human infrastructure/CODEOWNERS review is still required.
|
Taking over completion at Pat’s explicit request, following the completed review above. I will correct the remaining FFTW comment, verify the include-order behavior, and complete the normal CI and merge gates. |
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed 00bc3d7a560abfde1646ed67ecde673bab84ec96 following Pat’s requested takeover.
The change fits the stated build defect (no linked issue). No source blockers found. Merge recommendation remains pending fresh required CI.
| File | Scope | Verdict |
|---|---|---|
| CMakeLists.txt | Classify the existing Apple/Homebrew include path as SYSTEM; explain precedence | In scope; existing FFTW comment nit corrected in 00bc3d7 |
The entire diff is confined to the existing Apple block. Vendored whisper/ggml selection still defaults to the vendored implementation; no package discovery, dependency, radio, GUI, persistence, test registration, or changelog changes. The previous FFTW thread is addressed and resolved. The stale PR-body testing note is already absent.
Verification: fresh current-main merge-tree succeeds; diff whitespace check passes; GitHub verifies both commit signatures. A scratch CMake parent/child project on AppleClang 21 proves both claims: the vendored collision header wins and a system-only header remains available. It compiled and passed 1/1 tests; removing SYSTEM fails the intended compile-time header assertion; restoring SYSTEM compiled and passed 1/1 again. The generated command places the vendor -I before system -isystem. This is an include-order probe, not an AetherSDR application build or reproduction with the author’s installed ggml 0.15.2. No app, socket peer, or hardware execution was needed.
Fresh CI runs 34044533165 (Static Checks) and 34044533292 (CI) are action_required pending fork-workflow approval. They have not executed; earlier-head results do not establish current-head CI. Automatic approval review rejected that workflow approval and requires explicit operator authorization. No merge or clean CI verdict is claimed.
Cost: approximately 8 minutes; three small probe configure/build calls (pass, deliberate failure, restored pass), one selected test passed on each passing configuration; zero AetherSDR builds, app launches, or subagents. Prior source review was used as a lead, with fresh diff and compiler verification; tokens unavailable.
jensenpat
left a comment
There was a problem hiding this comment.
Final approval following the completed source review and comment correction. All five current-head checks passed: Linux, macOS, Windows, Static checks, and sanitizer configuration. CI tested synthetic merge 6248d86 (head 00bc3d7 into 8b101e5); its macOS selections passed 2/2 and Windows selections passed 5/5. Fresh latest-main merge-tree and whitespace checks pass; all review threads are resolved. The local include-order probe previously passed 1/1, failed with SYSTEM removed, and passed after restoration. No source blockers remain.
|
@jensenpat — thank you for finishing this one. The correction in Why it was wrong, rather than just that it was: the That is the second time this week on my PRs that a correction landed in the I have read the merged |
On macOS with Homebrew's
ggmlinstalled, a default build fails:ggml.cincludes"ggml.h"with quotes. That searches the including file's owndirectory first —
ggml/src/, which does not contain it — and then the-Ilist in order.
CMakeLists.txt:84puts/opt/homebrew/includeat the head ofthat list for every target in the project, so the compiler picks up Homebrew's
ggml 0.15.2 header instead of the vendored one that the rest of the vendored
sources were written against.
GGML_TYPE_Q2_0is declared in the vendoredheader (
ggml/include/ggml.h:432,= 42) and does not exist in 0.15.2.The build has deliberately chosen the vendored copy —
USE_SYSTEM_LIBWHISPERdefaults
OFF— and is then compiling it against the system's headers. That isthe actual defect: the choice is made in CMake and silently reversed by the
include path.
Confirmed by the compiler, not by reading
-Hprints the resolved include tree. Same source, same target, one flagchanged:
and the error count goes from 7 to 0 across the two files.
The fix
One keyword.
-isystemdirectories are searched after every-Idirectory, soHomebrew remains available for everything that genuinely needs it (portaudio,
fftw3, hidapi, and the
USE_SYSTEM_*=ONpaths) while no longer outranking thetree's own headers. Suppressing warnings from third-party headers is a
side-benefit, not the reason.
This is a class, not an instance — with the count stated honestly
ggml.his the one that fails loudly. It is not the only header this affects,but the numbers need separating, because "21 headers fixed" would be a claim
this evidence does not support.
21 vendored public headers share a basename with one in
/opt/homebrew/includeon this machine. That is a basename collision count,obtained by intersecting two directory listings — it is the size of the hazard,
not the size of the demonstrated fix.
7 are demonstrated shadowed and fixed, from
-Htraces on the real compilelines of
ggml.c,ggml-quants.c,ggml.cppandwhisper.cpp:ggml.h/opt/homebrew/includeggml-alloc.h/opt/homebrew/includeggml-backend.h/opt/homebrew/includeggml-cpp.h/opt/homebrew/includeggml-cpu.h/opt/homebrew/includegguf.h/opt/homebrew/includewhisper.h/opt/homebrew/includewhisper.his the one worth pausing on: whisper.cpp compiled cleanly againstHomebrew's header, so nothing announced it. It is the silent case, and it is the
reason this is worth fixing rather than working around.
The remaining 14 are not claimed.
ggml-cuda.h,-metal.h,-vulkan.h,-blas.h,-cann.h,-sycl.h,-rpc.h,-openvino.h,-webgpu.h,-zendnn.h,-virtgpu.h,-opt.handparakeet.hbelong to backends thisconfiguration does not build, so they never entered an include tree here and
their shadowing is latent, not demonstrated.
fftw3.his a different case entirely and is NOT a bug. On macOS FFTW comesfrom pkg-config (
CMakeLists.txt:239-243); the vendoredthird_party/fftw3/include/fftw3.his insideif(WIN32)and is never added tothe include path on this platform. So it resolving to Homebrew is by design,
and it was never shadowed here. It appeared in the basename intersection and
does not belong in the fixed list.
That distinction matters more than the count: a table with one wrong row invites
a reader to distrust the others, and this one had a row that looked like a bug
and was not.
Testing
Run on macOS 26.5, Apple clang, Qt 6.8.3, Homebrew
ggml 0.15.2installed andleft installed — nothing unlinked, nothing removed — with
ENABLE_ASRat itsdefault. Before and after in the same tree, so the comparison has no second
variable.
ggml-basefails with 7 errors — 4 inggml.c(
GGML_TYPE_Q2_0,GGML_FTYPE_MOSTLY_Q2_0) and 3 inggml-quants.c.[16/3102] Building C object …/ggml.c.oand[35/3102] …/ggml-quants.c.o—the two files that previously read
FAILED.-Hevidence above, taken from the build's owncompile_commands.jsonrather than reconstructed flags, so the claim is not"it builds now" but "it builds now because this header is being chosen".
Scope
macOS only — the block is inside
if(APPLE). No behaviour change on Linux orWindows, and none on macOS beyond include precedence and third-party warning
suppression.
🤖 Generated with Claude Code
https://claude.ai/code/session_01FtHEsQsghFwhUQEZdwVKUz