Fix --system-libclang on Gentoo amd64, if llvm is compiled with ABI_X86="32 64" #891

DarthGandalf · 2017-12-22T23:48:12Z

$ ls -ld /usr/lib64/llvm/4/l*
drwxr-xr-x 3 root root 20K Dec 16 02:37 lib32
drwxr-xr-x 3 root root 20K Dec 16 02:37 lib64
drwxr-xr-x 2 root root 4.0K Dec 16 02:37 libexec

This results in GLOB finding the wrong library.

$ ./install.py --clang-completer --system-libclang
...
Using external libclang: /usr/lib64/llvm/4/lib32/libclang.so.4.0
...
/usr/lib64/llvm/4/lib32/libclang.so.4.0: error adding symbols: File in wrong format

FTR, I had to use --system-libclang because of libtinfo.so.5 issue (ycm-core/YouCompleteMe#778)

This change is

codecov-io · 2017-12-23T00:45:53Z

Codecov Report

Merging #891 into master will increase coverage by 1.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #891      +/-   ##
==========================================
+ Coverage   94.69%   95.87%   +1.17%     
==========================================
  Files          41       79      +38     
  Lines        4018     5478    +1460     
==========================================
+ Hits         3805     5252    +1447     
- Misses        213      226      +13

bstaletic · 2017-12-23T02:44:51Z

Of course I forgot about multilib llvm when I posted the first Gentoo specific oull request.

I've confirmed that this works on a non-multilb llvm, so only a few questions.

Isllvm-config a Gentoo specific tool?
- If llvm-config exists on other distros, can others confirm this doesn't break anything else?
Seems to me like llvm-config makes the hand-written glob redundant. Am I correct?

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

DarthGandalf · 2017-12-23T09:12:39Z

llvm-config is not specific to Gentoo. But e.g. on Ubuntu it's named llvm-config-3.8. I'm not fan of hardcoding all possible versions here, as some people do.
And I don't see it on macos, though I didn't install clang from homebrew there, maybe that one has it. (I didn't try to install YCM on macos yet at all)

So... some of the other paths are probably not necessary anymore, but I don't know which.

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

bstaletic · 2017-12-23T09:18:18Z

I believe debian based distributions have something like update_alternatives which would make a symlink from llvm-config to llvm-config-<version>.

I see this change as one that could clean up out CMakeLists.txt a bit, so it's a welcome change. I just want to make sure we get it right, so I want to hear from other maintainers too.

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

puremourning · 2017-12-23T13:25:49Z

Thanks for sending a PR!

Re: macOS:

AFAIKllvm-config is not shipped with Apple clang, which is the clang that should be used with --sytem-libclang on macOS, as this is the only way for modules to work (which is required for iOS).

I wonder if this change will make it inadvertently start using a homebred-installed version of libclang when it didn't before? TBH I'm not sure, and testing it out would be a serious pain.

IN principle, I think using llvm-config is a good-thing(TM), though we have to be a little careful not to break people who previously didn't realise they were relying on us not doing that (however nefariously).

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

bstaletic · 2017-12-23T13:41:02Z

I think we should test this carefull on all the platforms we have access to.

I can test Mint, Windows and maybe RHEL.
Which leave *BSD land.

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

cpp/ycm/CMakeLists.txt, line 311 at r1 (raw file):

                  ${SYS_LLVM_PATHS}
                  ${FREEBSD_LLVM_PATHS}
                  ${GENTOO_LLVM_PATHS}

I believe ${GENTOO_LLVM_PATHS} can be removed if we use llvm-config.

Comments from Reviewable

bstaletic · 2017-12-25T09:21:29Z

I've just tried llvm-config on Mint. It successfully found the default libclang.so.

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

…86="32 64" $ ls -ld /usr/lib64/llvm/4/l* drwxr-xr-x 3 root root 20K Dec 16 02:37 lib32 drwxr-xr-x 3 root root 20K Dec 16 02:37 lib64 drwxr-xr-x 2 root root 4.0K Dec 16 02:37 libexec This results in GLOB finding the wrong library. $ ./install.py --clang-completer --system-libclang ... Using external libclang: /usr/lib64/llvm/4/lib32/libclang.so.4.0 ... /usr/lib64/llvm/4/lib32/libclang.so.4.0: error adding symbols: File in wrong format

DarthGandalf · 2017-12-25T14:54:33Z

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/CMakeLists.txt, line 311 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I believe ${GENTOO_LLVM_PATHS} can be removed if we use llvm-config.

Removed.

Comments from Reviewable

Valloric · 2017-12-26T21:38:48Z

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/CMakeLists.txt, line 295 at r2 (raw file):

    file( GLOB SYS_LLVM_PATHS "/usr/lib/llvm*/lib" )
    # On FreeBSD , llvm install into /usr/local/llvm-xy
    file ( GLOB FREEBSD_LLVM_PATHS "/usr/local/llvm*/lib")

Do we still need the FreeBSD paths if we're now using llvm-config? Does anyone have access to a FreeBSD machine to try it out?

Comments from Reviewable

puremourning · 2017-12-27T02:53:43Z

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/CMakeLists.txt, line 295 at r2 (raw file):

Previously, Valloric (Val Markovic) wrote…

Do we still need the FreeBSD paths if we're now using llvm-config? Does anyone have access to a FreeBSD machine to try it out?

I installed a FreeBSD 11.1 VM. There is no llvm-config by default. The default compiler is clang 4.0.0 which is way too old for YCM now anyway. --system-libclang isn't strictly needed on FreeBSD as the precompiled binaries work fine.

I installed llvm 5 pkg install llvm50 and this put a llvm-config50 in the path, which this doesn't appear to search for.

However, the existing path (/usr/local/llvm*/lib) does find the llvm50:

Using external libclang: /usr/local/llvm50/lib/libclang.so.5.0

So, in conclusion: Yes, the BSD paths are still needed, because the required llvm-config on FreeBSD is llvm-config50.

Comments from Reviewable

bstaletic · 2017-12-27T09:21:39Z

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/CMakeLists.txt, line 295 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I installed a FreeBSD 11.1 VM. There is no llvm-config by default. The default compiler is clang 4.0.0 which is way too old for YCM now anyway. --system-libclang isn't strictly needed on FreeBSD as the precompiled binaries work fine.

I installed llvm 5 pkg install llvm50 and this put a llvm-config50 in the path, which this doesn't appear to search for.

However, the existing path (/usr/local/llvm*/lib) does find the llvm50:
Using external libclang: /usr/local/llvm50/lib/libclang.so.5.0
So, in conclusion: Yes, the BSD paths are still needed, because the required llvm-config on FreeBSD is llvm-config50.

Well, that changes things a bit.

The llvm-config on Mint 18 is 3.8, so it found libclang.so.3.8. There is llvm-config-4.0, but no llvm-config-5.0. Mint 18 is based on Ubuntu 16.04.0.
Maybe we could try different llvm-config<whatever> instead of globs that match required paths. Not sure how well that would work.

Something like:

find_program( LLVM_CONFIG_EXECUTABLE_BSD llvm-config50 )
find_program( LLVM_CONFIG_EXECUTABLE_DEBIAN_BASED llvm-config-5.0 )
...
find_program( LLVM_CONFIG_EXECUTABLE_FALLBACK llvm-config )

And if we end up using the last one we print a warning that we have no idea what version we might find and what dragons be there.

Comments from Reviewable

DarthGandalf · 2017-12-27T10:35:23Z

I've got another idea: use find_package ( Clang ) to make it use ClangConfig.cmake which comes with clang itself, instead of calling llvm-config. It might be better in some cases, but I'll try and see what happens.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/CMakeLists.txt, line 295 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Well, that changes things a bit.

The llvm-config on Mint 18 is 3.8, so it found libclang.so.3.8. There is llvm-config-4.0, but no llvm-config-5.0. Mint 18 is based on Ubuntu 16.04.0.
Maybe we could try different llvm-config<whatever> instead of globs that match required paths. Not sure how well that would work.

Something like:
find_program( LLVM_CONFIG_EXECUTABLE_BSD llvm-config50 )
find_program( LLVM_CONFIG_EXECUTABLE_DEBIAN_BASED llvm-config-5.0 )
...
find_program( LLVM_CONFIG_EXECUTABLE_FALLBACK llvm-config )
And if we end up using the last one we print a warning that we have no idea what version we might find and what dragons be there.

I wanted to avoid hardcoding versions... but if that's unavoidable, find_program ( LLVM_CONFIG_EXECUTABLE NAMES llvm-config-5.0 llvm-config50 ) is cleaner than several different find_programs. Also someone needs to update the list of versions whenever a new version is released.
I thought --system-libclang is where dragons live anyway; is using some particular version found on system more supported than using other versions found on system? Also, why is 4.0 "way too old"? Seems to work fine for me.

Comments from Reviewable

bstaletic · 2017-12-27T11:09:07Z

Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/CMakeLists.txt, line 295 at r2 (raw file):

Previously, DarthGandalf (Alexey Sokolov) wrote…

I wanted to avoid hardcoding versions... but if that's unavoidable, find_program ( LLVM_CONFIG_EXECUTABLE NAMES llvm-config-5.0 llvm-config50 ) is cleaner than several different find_programs. Also someone needs to update the list of versions whenever a new version is released.
I thought --system-libclang is where dragons live anyway; is using some particular version found on system more supported than using other versions found on system? Also, why is 4.0 "way too old"? Seems to work fine for me.

Hard coding versions was just for the sake of simplicity, nothing more than that. It's definitely not a good idea.

4.0 is old, because the version ycmd downloads is 5.0.1, so who knows when something might break because of different versions.

Comments from Reviewable

puremourning · 2017-12-27T18:54:35Z

Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/CMakeLists.txt, line 295 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Hard coding versions was just for the sake of simplicity, nothing more than that. It's definitely not a good idea.

4.0 is old, because the version ycmd downloads is 5.0.1, so who knows when something might break because of different versions.

I'm not sure how much effort we should go to in order to support --system-libclang which is strongly discouraged, and barely supported. Ultimately users can use the full installation guide and -DEXTERNAL_LIBCLANG_PATH and point it wherever they like in those instances.

Comments from Reviewable

bstaletic · 2017-12-27T18:57:31Z

Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/CMakeLists.txt, line 295 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I'm not sure how much effort we should go to in order to support --system-libclang which is strongly discouraged, and barely supported. Ultimately users can use the full installation guide and -DEXTERNAL_LIBCLANG_PATH and point it wherever they like in those instances.

There's always an option of merging this PR as is and only benefit Gentoo.
I just thought it was a nice oportunitty to tidy up the CMakeLists.txt.

Comments from Reviewable

DarthGandalf · 2018-01-07T15:57:32Z

ClangConfig.cmake still requires path to that file, which is different per distro, so doesn't work very well.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

cpp/ycm/CMakeLists.txt, line 295 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

There's always an option of merging this PR as is and only benefit Gentoo.
I just thought it was a nice oportunitty to tidy up the CMakeLists.txt.

Let's merge it as is...

Comments from Reviewable

bstaletic · 2018-01-07T16:53:24Z

Alright, seems like utilising llvm-config for every available platform is going to be hard.

This is if you ask me.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

puremourning · 2018-01-07T18:19:33Z

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

micbou · 2018-01-07T18:22:50Z

@zzbot r+

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from Reviewable

zzbot · 2018-01-07T18:22:51Z

📌 Commit f912ca7 has been approved by micbou

zzbot · 2018-01-07T18:22:53Z

⌛ Testing commit f912ca7 with merge 381e58b...

Fix --system-libclang on Gentoo amd64, if llvm is compiled with ABI_X86="32 64" $ ls -ld /usr/lib64/llvm/4/l* drwxr-xr-x 3 root root 20K Dec 16 02:37 lib32 drwxr-xr-x 3 root root 20K Dec 16 02:37 lib64 drwxr-xr-x 2 root root 4.0K Dec 16 02:37 libexec This results in GLOB finding the wrong library. $ ./install.py --clang-completer --system-libclang ... Using external libclang: /usr/lib64/llvm/4/lib32/libclang.so.4.0 ... /usr/lib64/llvm/4/lib32/libclang.so.4.0: error adding symbols: File in wrong format FTR, I had to use --system-libclang because of libtinfo.so.5 issue (ycm-core/YouCompleteMe#778)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/891)

zzbot · 2018-01-07T20:43:39Z

💔 Test failed - status-appveyor

bstaletic · 2018-01-07T20:44:27Z

@zzbot retry

zzbot · 2018-01-07T20:44:30Z

⌛ Testing commit f912ca7 with merge 60cb22c...

Fix --system-libclang on Gentoo amd64, if llvm is compiled with ABI_X86="32 64" $ ls -ld /usr/lib64/llvm/4/l* drwxr-xr-x 3 root root 20K Dec 16 02:37 lib32 drwxr-xr-x 3 root root 20K Dec 16 02:37 lib64 drwxr-xr-x 2 root root 4.0K Dec 16 02:37 libexec This results in GLOB finding the wrong library. $ ./install.py --clang-completer --system-libclang ... Using external libclang: /usr/lib64/llvm/4/lib32/libclang.so.4.0 ... /usr/lib64/llvm/4/lib32/libclang.so.4.0: error adding symbols: File in wrong format FTR, I had to use --system-libclang because of libtinfo.so.5 issue (ycm-core/YouCompleteMe#778)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/891)

zzbot · 2018-01-08T00:32:53Z

💔 Test failed - status-appveyor

bstaletic · 2018-01-08T09:34:24Z

Third time's the charm?
@zzbot retry

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

zzbot · 2018-01-08T09:34:27Z

⌛ Testing commit f912ca7 with merge 41cb36b...

Fix --system-libclang on Gentoo amd64, if llvm is compiled with ABI_X86="32 64" $ ls -ld /usr/lib64/llvm/4/l* drwxr-xr-x 3 root root 20K Dec 16 02:37 lib32 drwxr-xr-x 3 root root 20K Dec 16 02:37 lib64 drwxr-xr-x 2 root root 4.0K Dec 16 02:37 libexec This results in GLOB finding the wrong library. $ ./install.py --clang-completer --system-libclang ... Using external libclang: /usr/lib64/llvm/4/lib32/libclang.so.4.0 ... /usr/lib64/llvm/4/lib32/libclang.so.4.0: error adding symbols: File in wrong format FTR, I had to use --system-libclang because of libtinfo.so.5 issue (ycm-core/YouCompleteMe#778)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/891)

zzbot · 2018-01-08T12:26:25Z

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 41cb36b to master...

[READY] Update ycmd Include the following changes: - PR ycm-core/ycmd#789: add support for Windows flags when --driver-mode=cl is given; - PR ycm-core/ycmd#848: hide C++ symbols by default; - PR ycm-core/ycmd#857: add Java support using jdt.ls; - PR ycm-core/ycmd#861: translate libclang error codes to exceptions; - PR ycm-core/ycmd#880: support downloading Clang binaries on ARM systems; - PR ycm-core/ycmd#883: handle zero column diagnostic from OmniSharp; - PR ycm-core/ycmd#884: specify Platform property when compiling OmniSharp; - PR ycm-core/ycmd#886: use current working directory in JavaScript completer; - PR ycm-core/ycmd#887: update Boost to 1.66.0; - PR ycm-core/ycmd#888: update JediHTTP; - PR ycm-core/ycmd#889: update Clang to 5.0.1; - PR ycm-core/ycmd#891: fix building with system libclang on Gentoo amd64; - PR ycm-core/ycmd#904: drop Python 2.6 and Python 3.3 support; - PR ycm-core/ycmd#905: calculate the start column when items are not resolved in the language server completer; - PR ycm-core/ycmd#912: download Clang binaries from HTTPS; - PR ycm-core/ycmd#914: do not try to symlink libclang on Windows.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2902)

DarthGandalf force-pushed the llvm branch from a4e8b0e to f912ca7 Compare December 25, 2017 14:53

zzbot merged commit f912ca7 into ycm-core:master Jan 8, 2018

micbou mentioned this pull request Feb 9, 2018

[READY] Update ycmd ycm-core/YouCompleteMe#2902

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix --system-libclang on Gentoo amd64, if llvm is compiled with ABI_X86="32 64" #891

Fix --system-libclang on Gentoo amd64, if llvm is compiled with ABI_X86="32 64" #891

DarthGandalf commented Dec 22, 2017 •

edited by Valloric

Loading

codecov-io commented Dec 23, 2017 •

edited

Loading

bstaletic commented Dec 23, 2017

DarthGandalf commented Dec 23, 2017

bstaletic commented Dec 23, 2017

puremourning commented Dec 23, 2017

bstaletic commented Dec 23, 2017

bstaletic commented Dec 25, 2017

DarthGandalf commented Dec 25, 2017

Valloric commented Dec 26, 2017

puremourning commented Dec 27, 2017

bstaletic commented Dec 27, 2017

DarthGandalf commented Dec 27, 2017

bstaletic commented Dec 27, 2017

puremourning commented Dec 27, 2017

bstaletic commented Dec 27, 2017

DarthGandalf commented Jan 7, 2018

bstaletic commented Jan 7, 2018

puremourning commented Jan 7, 2018

micbou commented Jan 7, 2018

zzbot commented Jan 7, 2018

zzbot commented Jan 7, 2018

zzbot commented Jan 7, 2018

bstaletic commented Jan 7, 2018

zzbot commented Jan 7, 2018

zzbot commented Jan 8, 2018

bstaletic commented Jan 8, 2018

zzbot commented Jan 8, 2018

zzbot commented Jan 8, 2018

Fix --system-libclang on Gentoo amd64, if llvm is compiled with ABI_X86="32 64" #891

Fix --system-libclang on Gentoo amd64, if llvm is compiled with ABI_X86="32 64" #891

Conversation

DarthGandalf commented Dec 22, 2017 • edited by Valloric Loading

codecov-io commented Dec 23, 2017 • edited Loading

Codecov Report

bstaletic commented Dec 23, 2017

DarthGandalf commented Dec 23, 2017

bstaletic commented Dec 23, 2017

puremourning commented Dec 23, 2017

bstaletic commented Dec 23, 2017

bstaletic commented Dec 25, 2017

DarthGandalf commented Dec 25, 2017

Valloric commented Dec 26, 2017

puremourning commented Dec 27, 2017

bstaletic commented Dec 27, 2017

DarthGandalf commented Dec 27, 2017

bstaletic commented Dec 27, 2017

puremourning commented Dec 27, 2017

bstaletic commented Dec 27, 2017

DarthGandalf commented Jan 7, 2018

bstaletic commented Jan 7, 2018

puremourning commented Jan 7, 2018

micbou commented Jan 7, 2018

zzbot commented Jan 7, 2018

zzbot commented Jan 7, 2018

zzbot commented Jan 7, 2018

bstaletic commented Jan 7, 2018

zzbot commented Jan 7, 2018

zzbot commented Jan 8, 2018

bstaletic commented Jan 8, 2018

zzbot commented Jan 8, 2018

zzbot commented Jan 8, 2018

DarthGandalf commented Dec 22, 2017 •

edited by Valloric

Loading

codecov-io commented Dec 23, 2017 •

edited

Loading