CMake: drop redundant simdjson include dir, rely on simdjson::simdjson link#3077
CMake: drop redundant simdjson include dir, rely on simdjson::simdjson link#3077tomjn wants to merge 1 commit into
Conversation
|
This seems very brittle. This seems like an issue with DeVIL find package. Manually prepending certain paths breaks the abstraction that CMake provides here. |
was this meant for another PR? This change has nothing to do with DeVIL and isn't hardcoding paths |
|
In your description you said that this problem was introduced by DevIL find package bringing in the homebrew prefix. That seems like the issue here. |
|
Oh, so this would have been an issue eitherway it's just the DevIL was what threw it up. I'm not sure how this is any different from globally installed libraries on Linux though, just that here I installed via homebrew on a mac, and technically if someone had used homebrew on linux they'd get the same problem. Wouldn't we always want our local vendored library includes to always take precedence over system libraries? |
p2004a
left a comment
There was a problem hiding this comment.
This include line should not exist in the first place. Every target that needs simd json must link simdjson::simdjson, that will pull correct flags for compilation and linking. That must also handle correctly header resolution.
If this line is needed, it means that some target is missing linking of simdjson::simdjson.
Every <simdjson.h> consumer already links simdjson::simdjson (GLTFParser via engineCommonLibraries; fastgltf links it privately), and that in-tree target exports the vendored include as a normal -I dir. DevIL's imported target is the only thing putting /opt/homebrew/include on the line, and as an imported target its include is emitted -isystem, which the compiler searches after every -I dir - so the vendored simdjson header wins on its own. The manual include_directories entry was redundant. Per review feedback (p2004a).
ae0ebc9 to
635e305
Compare
|
@p2004a I've been following that thread and it's lead to a much smaller changeset which has me suspicious 🤔 |
What
Removes the manual
include_directories(.../lib/simdjson/include)entry fromrts/CMakeLists.txtand lets header resolution ride on thesimdjson::simdjsonlink edge instead.Why
Per @p2004a's review: a target that links
simdjson::simdjsonalready gets the vendored include path as a usage requirement, so the explicitinclude_directoriesline should not be needed. I verified that every<simdjson.h>consumer does link the target -GLTFParser.cppthrough the engine'sengineCommonLibraries, andfastgltflinkssimdjson::simdjsonprivately in its own CMake. Nothing was relying on the global include line for header resolution.The reason the line looked load-bearing on macOS is DevIL: its Homebrew headers live at
/opt/homebrew/include/IL/il.h, soFindDevILresolvesIL_INCLUDE_DIRto the broad/opt/homebrew/includeprefix, which also contains a stray Homebrewsimdjson.h. ButDevIL::ILis an imported target, so CMake emits its include as-isystem, and the compiler searches every-Idir (including the vendored simdjson path from the link) before any-isystemdir. So the vendored header wins on its own and the manual entry is redundant. This supersedes the earlier include-order approach.Verification
Reasoned from the link graph and CMake's
-I/-isystemordering; not yet confirmed with a macOS build. Needs a build on macOS (Homebrew DevIL + a Homebrewsimdjsonpresent) to confirm<simdjson.h>still resolves to the vendored copy after the line is removed. Linux/Windows are unaffected (DevIL there resolves to an implicit system dir that CMake drops entirely).If a macOS build shows the vendored header no longer winning, the correct root-cause follow-up is to stop
DevIL::ILfrom advertising the broad/opt/homebrew/includeprefix (or force it system), rather than reinstating a simdjson-specific include override.AI disclosure
Analysis and patch prepared with AI assistance (Claude); the reasoning about link edges and
-I/-isystemordering was verified against the CMake by a human, and the macOS build check is outstanding.