[CMake] Fixes and improvements for buildng with builtin_clang=OFF
#18386
Conversation
The `theanos` library is not used in any test anyway, and looking for it will pollute the CMake output with "not found" messages.
Disable linking against shared LLVM also for `builtin_clang=OFF`. We have to do this after find_package(Clang). Finding Clang internally calls find_package(LLVM), which overwrites LLVM_LINK_LLVM_DYLIB to what it was set when building LLVM.
Assume that the Clang versions alsways matches the LLVM version to simplify the code, and also make it possible to find Clang installations separate from the LLVM directory by considering `CLANG_INSTALL_PREFIX`.
guitargeek
force-pushed
the
builtin_clang_off
branch
from
d52329b to
82b1a49
Compare
|
we may even think to a build exercising this functionality at this point. |
guitargeek
changed the title
builtin_clang=OFF.builtin_clang=OFF
Thanks for the review! I migrated my personal workflows to always build with |
guitargeek
changed the title
builtin_clang=OFFbuiltin_clang=OFF
| list(GET CHILDREN 0 CLANG_RESOURCE_DIR_VERSION) | ||
| # If no explicit Clang prefix is set, assume it lives inside LLVM prefix. | ||
| if(NOT DEFINED CLANG_INSTALL_PREFIX) | ||
| set(CLANG_INSTALL_PREFIX ${LLVM_DIR}) |
There was a problem hiding this comment.
LLVM_DIR is the wrong variable, it is only a hint specified by the user. The right approach is to take one of the variables set by LLVMConfig.cmake, see also commit bb4fde0
There was a problem hiding this comment.
You're right, thanks! I'll fix this up, going back to the ${LLVM_LIBRARY_DIR} that was used before
There was a problem hiding this comment.
PR opened: #18407
|
I would appreciate if at least one person involved in the interpreter / LLVM business is added as reviewer for this kind of PRs... |
| # We have to do this after find_package(Clang). Finding Clang internally | ||
| # calls find_package(LLVM), which overwrites LLVM_LINK_LLVM_DYLIB to what it | ||
| # was set when building LLVM. | ||
| set(LLVM_LINK_LLVM_DYLIB FALSE) |
There was a problem hiding this comment.
Revisiting the code, this is also not ideal: The right solution that we have chosen in Cling is to pass DISABLE_LLVM_LINK_LLVM_DYLIB to not use the shared library even if present. @guitargeek can you please revert this patch and implement the proper solutions?
There was a problem hiding this comment.
It's not as easy, as we have discussed in private. Also, the exact same thing is done in the builtin_clang code path:
https://github.com/root-project/root/blob/master/interpreter/CMakeLists.txt#L380
So this PR here only made the configuration code less inconsistent, which is always good IMHO. So I don't see the reason to revert if the supposedly "right" solution is not implemented in our main configuration code path either.
There was a problem hiding this comment.
It's not as easy, as we have discussed in private.
I disagree with this summary of our discussions. For me it is clear, and always has been, that the right solution is implemented on the linking side for libCling.
Also, the exact same thing is done in the
builtin_clangcode path: https://github.com/root-project/root/blob/master/interpreter/CMakeLists.txt#L380
Yes, though it is slightly different because there we are actually building Clang, not just modifying a variable after find_package(Clang). But I agree that it should also be reviewed.
So this PR here only made the configuration code less inconsistent, which is always good IMHO.
(for a bit more consistency in the file, it should have been OFF instead of FALSE...)
So I don't see the reason to revert if the supposedly "right" solution is not implemented in our main configuration code path either.
This PR was proposed on a Sunday afternoon, on the day before locking the branch, and merged within 2 hours, without review from any of the developers recently involved in this area before. I have pointed out one immediate mistake, and I maintain that fiddling with LLVM_LINK_LLVM_DYLIB is not the correct solution. The statement about "main configuration code path" is incorrect since the default, and only supported configuration for production use as far as I'm concerned, is builtin_llvm=ON builtin_clang=ON.
As changes to the build system this late in the cycle can have a significant blast area, I maintain that this PR should be reverted.
There was a problem hiding this comment.
For me it is clear, and always has been, that the right solution is implemented on the linking side for libCling.
You seemed to have no problem approving and merging the precedent for the builtin_clang=ON case: #13420 (review). What's really different now?
What you say seems contradictory to me. In the issue I opened you said that builtin_llvm=OFF && builtin_clang=OFF are maintained on a best effort basis, and now you argue that changes in these configuration paths can have a significant blast area. Why don't you just let me do the best effort then even though you think it's not perfect 🙂 Better than having something that doesn't work. And it's only natural that these things are discovered late in the dev cycle, because that's when we start thinking about how the new release can be built and packaged.
The statement about "main configuration code path" is incorrect since the default, and only supported configuration for production use as far as I'm concerned, is builtin_llvm=ON builtin_clang=ON
You're right, I apologize. I forgot that the precedent was for builtin_llvm=OFF builtin_clang=ON
There was a problem hiding this comment.
- In that code path we are building Clang, which I assumed is influenced by setting this variable. If I understand your upstream PR correctly, setting it before building Clang actually doesn't make a difference. So maybe it doesn't serve a purpose in the end, to be tested.
- The PR added
DISABLE_LLVM_LINK_LLVM_DYLIBfor linking, which I think is the right solution. The setting ofLLVM_LINK_LLVM_DYLIBmay have been an oversight among the other changes in the PR. - I delved deeper into
interpreter/CMakeLists.txtsince then, in particular last December / January when I cleaned it up quite a bit. This required some reverse engineering to understand what is going on, when I also discovered that some things are not working as I thought. - After realizing that one path is not the right solution, I think it is bad practice to approve merging more code. If code is already there, it should be reworked instead of added to.
Beyond that, I don't consider it good style to link a PR from almost two years ago and say "you accepted it back then"...
now you argue that changes in these configuration paths can have a significant blast area
What I argue is that build system changes in general can have a significant blast radius. This PR contains changes on the main configuration code path in core/clingutils/CMakeLists.txt.
Why don't you just let me do the best effort then even though you think it's not perfect
Because "best effort" doesn't mean that I don't care, and it doesn't mean that the use cases can be broken at will.
Better than having something that doesn't work.
YMMV.
And it's only natural that these things are discovered late in the dev cycle, because that's when we start thinking about how the new release can be built and packaged.
Then this should be changed, there will be a next cycle.
There was a problem hiding this comment.
I've submitted a revert in #18446
…clang=OFF" This reverts commit 3ef1193. The taken approach is not correct, this should be handled on the linking side of libCling as discussed post-merge in the pull request: root-project#18386
LLVM_LINK_LLVM_DYLIBafter finding ClangMore detail in the commit descriptions.