[READY] Improve LLVM root path search for prebuilt binaries

[micbou]

This PR simplifies the way we determine PATH_TO_LLVM_ROOT for Clang prebuilt binaries by using a glob pattern.

Fixes ycm-core/YouCompleteMe#2756.

---

This change is 

[bstaletic]

---

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

---

Comments from Reviewable

[codecov-io]

Codecov Report

Merging #820 into master will increase coverage by 0.05%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master     #820      +/-   ##
==========================================
+ Coverage   94.78%   94.84%   +0.05%     
==========================================
  Files          79       79              
  Lines        5373     5373              
  Branches      170      169       -1     
==========================================
+ Hits         5093     5096       +3     
+ Misses        233      231       -2     
+ Partials       47       46       -1

[puremourning]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

cpp/ycm/CMakeLists.txt, line 132 at r1 (raw file):
So, while I'm sure this works, I can't find any evidence in the (abysmal) cmake docs that ** is portably supported in glob expressions. Or at least, whether it has a special meaning. I suspect this is actually representing 2 consecutive * matches, which seems redundant.

I quickly checked the man pages on my OS (macOS) for:

• man -s 3 glob - the c glob function
• man bash - the bash shell expansion
• man tcsh - the csh shell expansion

The only indication of special behaviour i found was in tcsh:

   The globstar shell variable can be set to allow `**' or `***' as a file glob pattern that matches any string of characters including `/', recursively traversing any existing sub-directories.  For example, `ls **.c' will list  all

   the  .c  files  in  the  current  directory  tree.   If  used by itself, it will match match zero or more sub-directories (e.g. `ls /usr/include/**/time.h' will list any file named `time.h' in the /usr/include directory tree; `ls
   /usr/include/**time.h' will match any file in the /usr/include directory tree ending in `time.h'; and `ls /usr/include/**time**.h' will match any .h file with `time' either in a subdirectory name or in the filename  itself).   To
   prevent problems with recursion, the `**' glob-pattern will not descend into a symbolic link containing a directory.  To override this, use `***' (+)

So I'm unsure how the ** is being interpreted here, and I struggled to find out. So the question is, are you confident in this? If so, meh :)

---

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

  # CMake build folder.
  file( GLOB_RECURSE PATH_TO_LLVM_ROOT ${CMAKE_BINARY_DIR}/**/libclang.* )
  list( GET PATH_TO_LLVM_ROOT 0 PATH_TO_LLVM_ROOT )

Because I don't grok cmake (and the documentation for cmake is appalling), what the behaviour in the case that this list has 0 elements?

---

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

  file( GLOB_RECURSE PATH_TO_LLVM_ROOT ${CMAKE_BINARY_DIR}/**/libclang.* )
  list( GET PATH_TO_LLVM_ROOT 0 PATH_TO_LLVM_ROOT )
  get_filename_component( PATH_TO_LLVM_ROOT "${PATH_TO_LLVM_ROOT}/../.."

It's not super obvious to me why we need ../.. here or why that's (canonically) correct. I'm sure it is (e.g. it represents usr/lib or something), but perhaps we need a comment?

---

Comments from Reviewable

[micbou]

Thanks for the review.

---

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

---

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

Previously, puremourning (Ben Jackson) wrote…

So, while I'm sure this works, I can't find any evidence in the (abysmal) cmake docs that ** is portably supported in glob expressions. Or at least, whether it has a special meaning. I suspect this is actually representing 2 consecutive * matches, which seems redundant.

I quickly checked the man pages on my OS (macOS) for:

• man -s 3 glob - the c glob function
• man bash - the bash shell expansion
• man tcsh - the csh shell expansion

The only indication of special behaviour i found was in tcsh:

   The globstar shell variable can be set to allow `**' or `***' as a file glob pattern that matches any string of characters including `/', recursively traversing any existing sub-directories.  For example, `ls **.c' will list  all

   the  .c  files  in  the  current  directory  tree.   If  used by itself, it will match match zero or more sub-directories (e.g. `ls /usr/include/**/time.h' will list any file named `time.h' in the /usr/include directory tree; `ls
   /usr/include/**time.h' will match any file in the /usr/include directory tree ending in `time.h'; and `ls /usr/include/**time**.h' will match any .h file with `time' either in a subdirectory name or in the filename  itself).   To
   prevent problems with recursion, the `**' glob-pattern will not descend into a symbolic link containing a directory.  To override this, use `***' (+)

So I'm unsure how the ** is being interpreted here, and I struggled to find out. So the question is, are you confident in this? If so, meh :)

For some reason, I thought I could use ** like in Zsh. In fact, it's totally useless when using GLOB_RECURSE (see the /dir/*.py example in the docs). I've modified the pattern.

---

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

Previously, puremourning (Ben Jackson) wrote…

Because I don't grok cmake (and the documentation for cmake is appalling), what the behaviour in the case that this list has 0 elements?

If the list is empty, CMake returns the following error:

CMake Error at CMakeLists.txt:7 (list):
  list GET given empty list

but continues to execute the script. This results in PATH_TO_LLVM_ROOT being set to / in the next line. Probably harmless but still better to check if the list is empty and abort the script if so.

---

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

Previously, puremourning (Ben Jackson) wrote…

It's not super obvious to me why we need ../.. here or why that's (canonically) correct. I'm sure it is (e.g. it represents usr/lib or something), but perhaps we need a comment?

Added a comment to clarify the /../.. part.

---

Comments from Reviewable

[puremourning]

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[Valloric]

Thanks for the PR!

@zzbot r=bstaletic

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[zzbot]

📌 Commit db63cef has been approved by bstaletic

[zzbot]

⌛ Testing commit db63cef with merge 0afdba4...

[zzbot]

☀️ Test successful - status-travis
Approved by: bstaletic
Pushing 0afdba4 to master...