Closed Chilledheart closed 1 month ago
For the external LLVM root's usage, copying the dylib to LIBCLANG_DIR
is wrong because the dylib is bound with the clang
subdirectory (called resource dir in clang's terminology) under /path/to/llvm/root/lib
directory which contains important things such as builtin headers which might not match the one inside LIBCLANG_DIR
.
Merging #1729 (40e3c81) into master (7bc5d08) will not change coverage. The diff coverage is
n/a
.
@puremourning You're the one with a macOS. Can you take a look at this?
Can you provide a description of the problem this solves, and steps to reproduce
Steps to reproduce
~/clang+llvm-17.0.6-arm64-apple-darwin22.0
Modify the build.py to support linking with this external llvm (libclang inside)
diff --git a/build.py b/build.py
index 1e0f9a20..b84e3c81 100755
--- a/build.py
+++ b/build.py
@@ -618,7 +618,7 @@ def GetCmakeArgs( parsed_args ):
cmake_args.append( '-DUSE_CLANG_TIDY=ON' )
if parsed_args.system_libclang:
- cmake_args.append( '-DUSE_SYSTEM_LIBCLANG=ON' )
+ cmake_args.append( '-DPATH_TO_LLVM_ROOT=/Users/hky/clang+llvm-17.0.6-arm64-apple-darwin22.0' )
if parsed_args.enable_debug:
cmake_args.append( '-DCMAKE_BUILD_TYPE=Debug' )
./build.py --ninja --verbose --clang-completer --go-completer --system-libclang
otool -l
and you will find the libclang.dylib doesn't present inside the first rpath.does this issue occur when using EXTRA_CMAKE_ARGS as described here, as opposed to hacking build.py
: https://github.com/ycm-core/YouCompleteMe/wiki/Troubleshooting-steps-for-ycmd-server-SHUT-DOWN#if-you-used-the-full-installation-guide ?
I think it is the fastest way to use external llvm path cmake option by modifying build.py while building ycm doesn't depend on build.py (just a helper to build faster), so it is possible to run cmake directly for end users.
I didn't try extra cmake flags because if it downloads the libclang archive that will conflict with external llvm path option.
Typed from my phone.
I can't reproduce the issue. Here's what I did (Intel Mac):
5012 EXTRA_CMAKE_ARGS="-DPATH_TO_LLVM_ROOT=$(brew --prefix llvm)" ./build.py --clang-completer --system-libclang --verbose
5013 dtvim --cmd 'let g:ycm_use_clangd=0' test.cpp
Completion works.
-- Resolve completions: On demand
-- Client logfile: /var/folders/mr/p_8nf2d90gvd72mh012fqlg00000gn/T/ycm_bhqk74eq.log
-- Server Python interpreter: /usr/local/opt/python@3.11/bin/python3.11
-- Server Python version: 3.11.6
-- Server has Clang support compiled in: True
The reason I am nitpicking here is that we have had a number of changes in this area and every now and again someone comes along and says it doesn't work, but never fully explains why, and I am very keen not to regress it.
Also, I'm keen not to spend too much time on the legacy libclang support due to it being deprecated in favour of clangd which doesn't have this issue.
It is fine if you don't merge it. I saw a mess in the code conflicting with each other. As far as I can tell, on msvc it is expected to have libclang copy from external llvm path but it isn't the case for other platforms.
It is a minimal change I could do. But for a better future and more maintainable code base, it might be greater to reduce the conflicting code and do a code refactor. After that, running full tests on every platform to make sure things working.
It's not that I'm against merging it (and TBH I don't quite understand your point about the codebase), but that when we merge a change to the build it's our responsiblity to understand that it works, how it works, and why because we need to maintain it forever. I'm very grateful for the contribution, but I need to understand it better before moving on.
For clangd, I guess it is a different story. From my experience, Libclang (c interface) is much faster and uses less memory for me(e.g. Xcode still ships libclang).
In the latest releases of llvm, it even ships with libclang(cpp) dylib (c++ interface) in the releases tarball. Maybe it is encouraging to adopt c++ interface directly?
libclang lacks significant features vs clangd; we have not intention to invest further in it.
The impact on current build.py usage is minimal because build.py doesn't support external LLVM root. You can modify build.py to support it by add something like this
cmake_args.append( '-DPATH_TO_LLVM_ROOT=/Users/hky/clang+llvm-17.0.6-arm64-apple-darwin22.0' )
1) tested with downloaded libclang
Okay, because the dylib lies inside
LIBCLANG_DIR
2) tested with system libclangOkay, because the dylib lies outside
LIBCLANG_DIR
and next RPATH is correct. 3) tested with external libclangOkay, because the dylib lies outside
LIBCLANG_DIR
and next RPATH is correct.This change is![Reviewable](https://reviewable.io/review_button.svg)