Fix a issue that libclang.dylib won't be loaded when it is linked with libLLVM dynamically

[ghost]

resent PR #65, comments are welcome.

[Valloric]

Sadly, this PR breaks YCM compilation for me. Here's what I get:

FAILED: : && /usr/bin/c++   -stdlib=libc++ -fcolor-diagnostics -Wall -Wextra -Werror -Wc++98-compat -O3 -DNDEBUG  -dynamiclib -Wl,-headerpad_max_install_names  -o /Users/valloric/repos/YouCompleteMe/third_party/ycmd/ycm_core.so -install_name @rpath/ycm_core.so ycm/CMakeFiles/ycm_core.dir/Candidate.cpp.o ycm/CMakeFiles/ycm_core.dir/CandidateRepository.cpp.o ycm/CMakeFiles/ycm_core.dir/ClangCompleter/ClangCompleter.cpp.o ycm/CMakeFiles/ycm_core.dir/ClangCompleter/ClangHelpers.cpp.o ycm/CMakeFiles/ycm_core.dir/ClangCompleter/ClangUtils.cpp.o ycm/CMakeFiles/ycm_core.dir/ClangCompleter/CompilationDatabase.cpp.o ycm/CMakeFiles/ycm_core.dir/ClangCompleter/CompletionData.cpp.o ycm/CMakeFiles/ycm_core.dir/ClangCompleter/Range.cpp.o ycm/CMakeFiles/ycm_core.dir/ClangCompleter/TranslationUnit.cpp.o ycm/CMakeFiles/ycm_core.dir/ClangCompleter/TranslationUnitStore.cpp.o ycm/CMakeFiles/ycm_core.dir/CustomAssert.cpp.o ycm/CMakeFiles/ycm_core.dir/IdentifierCompleter.cpp.o ycm/CMakeFiles/ycm_core.dir/IdentifierDatabase.cpp.o ycm/CMakeFiles/ycm_core.dir/IdentifierUtils.cpp.o ycm/CMakeFiles/ycm_core.dir/LetterNode.cpp.o ycm/CMakeFiles/ycm_core.dir/LetterNodeListMap.cpp.o ycm/CMakeFiles/ycm_core.dir/PythonSupport.cpp.o ycm/CMakeFiles/ycm_core.dir/Result.cpp.o ycm/CMakeFiles/ycm_core.dir/Utils.cpp.o ycm/CMakeFiles/ycm_core.dir/versioning.cpp.o ycm/CMakeFiles/ycm_core.dir/ycm_core.cpp.o  BoostParts/libBoostParts.a /usr/lib/libpython2.7.dylib /Library/Developer/CommandLineTools/usr/lib/libclang.dylib -Wl,-rpath,/Library/Developer/CommandLineTools/usr/lib && cd /Users/valloric/builds/ycm_ninja/ycm && /usr/local/Cellar/cmake/3.0.2/bin/cmake -E copy /Library/Developer/CommandLineTools/usr/lib/libclang.dylib /Users/valloric/repos/YouCompleteMe/third_party/ycmd && cd /Users/valloric/builds/ycm_ninja/ycm && install_name_tool -change @rpath/libclang.dylib @loader_path/libclang.dylib /Users/valloric/repos/YouCompleteMe/third_party/ycmd/ycm_core.so && install_name_tool -add_rpath /Library/Developer/CommandLineTools/usr/lib /Users/valloric/repos/YouCompleteMe/third_party/ycmd/ycm_core.so
error: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/install_name_tool: for: /Users/valloric/repos/YouCompleteMe/third_party/ycmd/ycm_core.so (for architecture x86_64) option "-add_rpath /Library/Developer/CommandLineTools/usr/lib" would duplicate path, file already has LC_RPATH for: /Library/Developer/CommandLineTools/usr/lib
ninja: build stopped: subcommand failed.

I have a standard setup (nothing custom) so if this breaks for me, it will break for others. Feel free to update this PR with changes and ping me then. I recommend trying out your PR on a vanilla Mac setup (not the custom one you have) to ensure everything still works.

[ghost]

Yeah, I knew the possibility of this failure, but never thought it came so quickly. I will resubmit with a new patch which uses -Wl,-rpath instead of install_name_tool soon.

[ghost]

Updated. The new patch should have the same effect as the previous one, but more error-tolerant.

[Valloric]

Nope, still doesn't work:

[145/165] Linking CXX shared library /Users/valloric/repos/YouCompleteMe/third_party/ycmd/ycm_client_support.so
FAILED: : && /usr/bin/c++   -stdlib=libc++ -fcolor-diagnostics -Wall -Wextra -Werror -Wc++98-compat -O3 -DNDEBUG  -dynamiclib -Wl,-headerpad_max_install_names ;-Wl,-rpath,/Library/Developer/CommandLineTools/usr/lib -o /Users/valloric/repos/YouCompleteMe/third_party/ycmd/ycm_client_support.so -install_name @rpath/ycm_client_support.so ycm/CMakeFiles/ycm_client_support.dir/Candidate.cpp.o ycm/CMakeFiles/ycm_client_support.dir/CandidateRepository.cpp.o ycm/CMakeFiles/ycm_client_support.dir/CustomAssert.cpp.o ycm/CMakeFiles/ycm_client_support.dir/IdentifierCompleter.cpp.o ycm/CMakeFiles/ycm_client_support.dir/IdentifierDatabase.cpp.o ycm/CMakeFiles/ycm_client_support.dir/IdentifierUtils.cpp.o ycm/CMakeFiles/ycm_client_support.dir/LetterNode.cpp.o ycm/CMakeFiles/ycm_client_support.dir/LetterNodeListMap.cpp.o ycm/CMakeFiles/ycm_client_support.dir/PythonSupport.cpp.o ycm/CMakeFiles/ycm_client_support.dir/Result.cpp.o ycm/CMakeFiles/ycm_client_support.dir/Utils.cpp.o ycm/CMakeFiles/ycm_client_support.dir/versioning.cpp.o ycm/CMakeFiles/ycm_client_support.dir/ycm_client_support.cpp.o  BoostParts/libBoostParts.a /usr/lib/libpython2.7.dylib && :
/bin/sh: -Wl,-rpath,/Library/Developer/CommandLineTools/usr/lib: No such file or directory
[145/165] Building CXX object ycm/CMakeFiles/ycm_core.dir/PythonSupport.cpp.o
ninja: build stopped: subcommand failed.

[ghost]

In a short survey, it worked perfectly with cmake 3.1.0 and 3.0.2 (forgive me if I am always using the latest softwares for testing).

I updated a patch in a different approach. Maybe you want to try with it.

[Valloric]

This now works.

There's a comment right above the section you have edited:

# Things are a bit different on Macs when using an external libclang.dylib; here
# we want to make sure we use @loader_path/libclang.dylib instead of
# @rpath/libclang.dylib in the final ycm_core.so. If we use the
# @rpath version, then it may load the system libclang which the user
# explicitely does not want (otherwise the user would specify
# USE_SYSTEM_LIBCLANG). With @loader_path, we make sure that only the
# libclang.dylib present in the same directory as our ycm_core.so
# is used.

If I'm not mistaken, what you are doing here is going against that comment. Doesn't your change mean that a system libclang could be loaded before a custom libclang.dylib placed right next to the ycm_core.dylib? If so, we can't merge this. We don't want the system libclang accidentally overwriting the custom libclang.

[ghost]

No, it won't load the system's version. Since the path of @loader_path/libclang.dylib is hard-coded, dlopen will try to load the loader_path's version first. What's more, if @loader_path/libclang.dylib is dynamically linked with libLLVM-3.5.dylib or others, this patch will make sure it can find the existing library (or libraries). It behaves very different from Linux.

To confirm it, we can do a small test.

• write a small python script called test.py under ycmd directory:

import ycm_core
print  'Clang version: ' + ycm_core.ClangVersion()

• run the script with some debug environment variable on

DYLD_PRINT_LIBRARIES=1 python test.py

In my case (with llvm 3.5.1-rc2 built from source, shared libraries enabled), I have:

...
dyld: loaded: /Users/chilledheart/ycmd_master/ycm_core.so
dyld: loaded: /Users/chilledheart/ycmd_master/libclang.dylib
RPATH failed to expanding     @rpath/libclangIndex.dylib to: /System/Library/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/../lib/libclangIndex.dylib
dyld: loaded: /Users/chilledheart/build/lib/libclangIndex.dylib
RPATH successful expansion of @rpath/libclangIndex.dylib to: /Users/chilledheart/build/lib/libclangIndex.dylib
RPATH failed to expanding     @rpath/libclangARCMigrate.dylib to: /System/Library/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/../lib/libclangARCMigrate.dylib
...

while without this patch, it is:

...
dyld: loaded: /Users/chilledheart/ycmd_master/ycm_core.so
dyld: loaded: /Users/chilledheart/ycmd_master/libclang.dylib
dyld: unloaded: /Users/chilledheart/ycmd_master/ycm_core.so
dyld: unloaded: /Users/chilledheart/ycmd_master/libclang.dylib
Traceback (most recent call last):
  File "test.py", line 1, in <module>
    import ycm_core
ImportError: dlopen(/Users/chilledheart/ycmd_master/ycm_core.so, 2): Library not loaded: @rpath/libclangIndex.dylib
  Referenced from: /Users/chilledheart/ycmd_master/libclang.dylib
  Reason: image not found
...

What we know here:

1. if the @loader_path/libclang.dylib exists, dlopen will find and load it without problems.
2. if we add the extra rpath, dlopen will try to expend the path with it to find the missing libraries.

[Valloric]

Thanks for the explanation! Could you add a note about @library_path taking precedence over rpath in that comment (and make the rest of it non-obsolete)? With that, this would be ready to merge.

[ghost]

I have added some comments to the CMakeLists.txt. I am not sure if it is good enough but still hoping they explain the rule to find libclang.dylib clearly.

[Valloric]

Thanks for the PR!

[ghost]

Thank you too.