-
Notifications
You must be signed in to change notification settings - Fork 766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix rpath to libclang archive on mac with external llvm #1729
Conversation
1) tested with downloaded libclang ➜ ycmd git:(master) otool -l ycm_core.cpython-310-darwin.so|grep -A3 LC_RPATH cmd LC_RPATH cmdsize 96 path /Users/hky/.vim/plugged/YouCompleteMe/third_party/ycmd/cpp/../third_party/clang/lib (offset 12) Load command 18 2) tested with system libclang ➜ ycmd git:(master) otool -l ycm_core.cpython-310-darwin.so|grep -A3 LC_RPATH cmd LC_RPATH cmdsize 96 path /Users/hky/.vim/plugged/YouCompleteMe/third_party/ycmd/cpp/../third_party/clang/lib (offset 12) Load command 18 cmd LC_RPATH cmdsize 104 path /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib (offset 12) Load command 19 3) tested with external libclang ➜ ycmd git:(master) ✗ otool -l ycm_core.cpython-310-darwin.so|grep -A3 LC_RPATH cmd LC_RPATH cmdsize 96 path /Users/hky/.vim/plugged/YouCompleteMe/third_party/ycmd/cpp/../third_party/clang/lib (offset 12) Load command 18 cmd LC_RPATH cmdsize 72 path /Users/hky/clang+llvm-17.0.6-arm64-apple-darwin22.0/lib (offset 12) Load command 19
For the external LLVM root's usage, copying the dylib to |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1729 +/- ##
=======================================
Coverage 95.44% 95.44%
=======================================
Files 83 83
Lines 8166 8166
Branches 163 163
=======================================
Hits 7794 7794
Misses 322 322
Partials 50 50 |
@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
|
does this issue occur when using EXTRA_CMAKE_ARGS as described here, as opposed to hacking |
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):
Completion works.
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' )
Okay, because the dylib lies inside
LIBCLANG_DIR
2) tested with system libclang
Okay, because the dylib lies outside
LIBCLANG_DIR
and next RPATH is correct.3) tested with external libclang
Okay, because the dylib lies outside
LIBCLANG_DIR
and next RPATH is correct.This change is