Repository navigation
[CMake] do not let builtin_clang search again for LLVM if builtin_llvm=OFF - #23371
Merged
Merged
Conversation
…in_llvm=OFF Alternative to root-project#23160 that does not require any upstream or downstream patches In ROOT, for builtin_llvm=OFF we have the sequence ``` find_package(LLVM) # searchs for system package, which sets DYLIB to ON set(LLVM_LINK_LLVM_DYLIB OFF) # override DYLIB to OFF, we want static add_subdirectory(clang) # this will call find_package LLVM again ``` See root-project#13420 Since LLVM22 though, this fails: Even if we call for a second time `set(LLVM_LINK_LLVM_DYLIB OFF)` after the `add_subdirectory` line, it's not enough, because that variable was ON all throughout the child `clang/CMakeLists.txt` and has already configured some stuff with the wrong flag. Avoid the issue by preventing the double-search of LLVM and thus the double overwrite, by temporarily overwriting the find_package(LLVM) calls. Fixes root-project#18387 Fixes root-project#23155
Test Results 22 files 22 suites 3d 11h 49m 21s ⏱️ Results for commit 80875d8. ♻️ This comment has been updated with latest results. |
guitargeek
requested changes
Sep 16, 2026
guitargeek
left a comment
Contributor
There was a problem hiding this comment.
Thanks a lot for this new solution attempt, looks great!
- Fixes the
builtin_llvm=OFFconfiguration without an additional LLVM/Clang patch, handling the problem from the ROOT side. @hahnjo is explicitly fine with that (#18387 (comment)) - Overriding
find_packageis a common practice, but it might cause problems for vcpkg o n Windows (see #8633). But since we're restricting this override to thebuiltin_llvm=OFFconfiguration that is typically not used on Windows, this is fine for a temporary workaround that we'll carry until we can link against shared LLVM
I just have a few minor change requests that should be addressed before I can approve this PR.
ferdymercury
commented
Sep 16, 2026
ferdymercury
commented
Sep 16, 2026
to clarify that this is a temporary workaround Co-authored-by: Jonas Rembser <jonas.rembser@cern.ch>
1 of 2 tasks
guitargeek
approved these changes
Sep 16, 2026
guitargeek
left a comment
Contributor
There was a problem hiding this comment.
LGTM! See my previous comment for thorough review (#23371 (review))
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This Pull request:
Changes or fixes:
Alternative to #23160 that does not require any upstream or downstream patches to Clang.
This is a temporary solution, since building with LLVM.so is broken at the moment. (#20903 (comment)). It can be removed later if at some point that issue gets fixed.
In ROOT, for builtin_llvm=OFF we have the sequence
See #13420
Since LLVM22 though, this fails:
Even if we call for a second time
set(LLVM_LINK_LLVM_DYLIB OFF)after theadd_subdirectoryline, it's not enough, because that variable was ON all throughout the childclang/CMakeLists.txtand has already configured some stuff with the wrong flag.Avoid the issue by preventing the double-search of LLVM and thus the double overwrite, by temporarily overwriting the find_package(LLVM) calls.
Reproducer
Checklist:
Fixes #18387
Fixes #23155