[RFC] Add system header directories to user flags #853
Conversation
|
On first glance I think this will mean that it isn’t possible to use the iPhone platform and tool chain anymore? |
|
The |
|
On first glance, this looks like a good idea, but since different toolchains are important to me, I'll wait for the weekend to properly test this. Review status: 0 of 186 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
So, it doesn't work when compiled with system libclang. In that case, which looks good (the See the two nonexistent directories If we compare to system Clang with Surprisingly, Same result as Given these results, I am tempted to use system Clang (without Reviewed 7 of 186 files at r1. Comments from Reviewable |
|
Here are my first test results:
Now can anyone explain why is Reviewed 186 of 186 files at r1. Comments from Reviewable |
|
That's probably because it looks for the GCC standard library. Try to add the Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
Codecov Report
@@ Coverage Diff @@
## master #853 +/- ##
=========================================
- Coverage 97.67% 97.6% -0.07%
=========================================
Files 90 90
Lines 7039 7019 -20
=========================================
- Hits 6875 6851 -24
- Misses 164 168 +4 |
|
Other than that, your results looks fine. Did you check that Review status: 185 of 186 files reviewed at latest revision, all discussions resolved. Comments from Reviewable |
|
Yes, I checked With arm-none-eabi things.
More info about the first point: What I'm expecting to be found for C: What I'm expecting to be found for C++: What is clang and ycmd finding with Reviewed 1 of 1 files at r2. Comments from Reviewable |
|
For the record, old behaviour was better for cross toolchains as it didn't require |
|
So I took a look. If I’m honest, my initial reaction is that i don’t really love it. I’m not super keen on shipping an arbitrary copy of the standard library with YCM and the fake clang driver + parsing the result still requires us to ship a (essentially) fake toolchain. I realise that we have to hack this irritating macOS standard library path thing once a year with each new macOS/Xcode. But i’m convinced there is a canonical way to determine what we need to use programatically from the installed Xcode or command line tools. For a start, I think we should ask Xcode-select which toolchain to use, then just use that one. That’s allows the user to determine whether to use Xcode or command line tools and avoids any conflict. The second part of the problem (which I thought i fixed last year) is that the Xcode installation is fiddly and requires some logic that’s built in to the binrary installation location combined with an -isysroot flag for the particular platform target (macOS, iOS, etc.). FWIW i did try YCM with high sierra when it was GM and it worked fine, and still does for me. So I’m a little perplexed by the current slew of issues. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
Are you currently adding the
Yes, I don't like shipping the standard library either. That's why I am thinking of using system Clang on macOS.
Do you mean running Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
Yes, I am currently adding As for bundling a standard library, I am not a fan of that either, but I don't know much about macOS and how fiddly it is. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
[READY] Only include one of the possible mac toolchains in the include paths This is the yearly update to fix the latest macOS incompatibilities. It seems with Xcode 9 (for reasons I really don't understand), having both the Xcode "command line tools" _and_ Xcode in your include paths leads to errors being raised in standard headers. This change simply only adds a single set of system headers from a single toolchain on macOS. Fixes ycm-core/YouCompleteMe#2790 Fixes #844 Fixes ycm-core/YouCompleteMe#2536 This is a quicker and probably less contentious solution than #853 This is sort of equivalent to #854, but the implementation is simpler and has working tests. # Testing Repro steps: * with both Xcode and CLT installed create a trivial .ycm_extra_conf.py (flags -x c++ -Wall) * create a trivial c++ file and `#include <sys/types.h>` and some other c++ headers. * `:YcmDiags` I've tested this: * with both Xcode and CLT * with just Xcode * with just CLT All work. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/855) <!-- Reviewable:end -->
|
☔ The latest upstream changes (presumably #855) made this pull request unmergeable. Please resolve the merge conflicts. |
micbou
force-pushed
the
fake-clang
branch
3 times, most recently
from
68599b5 to
f62045c
Compare
|
@micbou Your heart is in the right place, but I don't think this is the best approach to the problem. :( I'd be for this extra complexity if it would consistently improve the user experience on all OS's we support. But that doesn't appear to be the case. Review status: 7 of 12 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
☔ The latest upstream changes (presumably #860) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Since we were talking about reviving this PR, how about doing what this PR suggests only on Windows and Linux? Reviewed 4 of 179 files at r3, 1 of 1 files at r4, 1 of 1 files at r5. ycmd/completers/cpp/flags.py, line 548 at r5 (raw file):
Comments from Reviewable |
micbou
force-pushed
the
fake-clang
branch
2 times, most recently
from
6df29d2 to
8c33f43
Compare
There was a problem hiding this comment.
I revisited the PR and made some improvements:
- the
ycm_fake_clangexecutable now directly call the libclang routines so it's simpler to compile; - the
-stdliband-targetflags have been added to the list of flags to pass to Clang; -x<language>is supported (-xc,-xc++, etc.);- fully covered.
A summary of the benefits of this PR:
- add out-of-the-box completion of system headers in include statements;
- add out-of-the-box navigation to system headers on include statements;
- remove the system headers code specific to macOS;
- supersede
clang -v -E -x c++ -workaround.
For MacOS we can stick to the current heuristics and thus avoid bundling the standard library.
We don't need to bundle the standard library anymore on macOS since we are using system Clang on this platform to get the system headers.
Reviewed 4 of 186 files at r1, 5 of 179 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 8 of 13 files at r6.
Reviewable status: 0 of 2 LGTMs obtained
ycmd/completers/cpp/flags.py, line 548 at r5 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
-xc++is also a valid flag, so we would need it in theelifbranch too.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r7.
Reviewable status: 0 of 2 LGTMs obtained
ycmd/completers/cpp/flags.py, line 514 at r7 (raw file):
def _ExtraClangFlags(): flags = [ CLANG_RESOURCE_DIR ]
With these changes, adding -resource-dir=clang_includes to the list of flags may not be required anymore since all system headers are already included.
There was a problem hiding this comment.
Failures on CircleCI are unrelated.
Reviewable status: 0 of 2 LGTMs obtained
There was a problem hiding this comment.
The default system include paths can't easily change. So, instead of figuring out these paths for each file, maybe we should cache them.
Let's say we only encounter C++ files. The cache could hold only one set of these paths.
If later we encounter a C file, the clang -E -v should be called to add C include paths to the cache.
Another solution would be to eagerly populate the cache for all C-family languages, but that is complicated because cuda might not be set up and -xcuda will fail.
I'm not sure something along these lines is worth the effort, but it's something we should at least consider.
Reviewed 6 of 13 files at r6, 1 of 2 files at r7.
Reviewable status: 0 of 2 LGTMs obtained
ycmd/completers/cpp/flags.py, line 514 at r7 (raw file):
Previously, micbou wrote…
With these changes, adding
-resource-dir=clang_includesto the list of flags may not be required anymore since all system headers are already included.
Since we're making sure we have some sort of clang executable, we can extract the -resource-dir with clang '-###' -v -xc /dev/null 2>&1 | grep resource.
The output is a long list of strings. Among the quoted strings are "-resource-dir" and "/path/to/resource/dir/".
ycmd/completers/cpp/flags.py, line 664 at r7 (raw file):
iter_flags = iter( flags ) for flag in iter_flags: if flag in [ '-gcc-toolchain', '--stdlib', '--sysroot', '-target', '-x' ]:
This list isn't done yet.
-nostdincand-nostdinc++are definitely needed for when an embedded project doesn't want to use system headers.-nostdliband-nodefaultlibsmay not be needed. These change how an executable is linked, but don't seem to change the include path.
|
ycmd/completers/cpp/flags.py, line 673 at r11 (raw file):
I figured that replacing
Only if clang finds the CUDA library location. Otherwise, clang refuses to do anything without |
micbou
force-pushed
the
fake-clang
branch
3 times, most recently
from
f336694 to
fd485c0
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r13.
Reviewable status: 0 of 2 LGTMs obtained
ycmd/completers/cpp/flags.py, line 673 at r11 (raw file):
Previously, midchildan wrote…
Here we are talking about discovering default system include paths. Ones you don't specify explicitly, but would be added with -isystem, so replacing -xcuda with -xc++ is not right.
I figured that replacing
-xcudawith-xc++was a bad idea.Is it possible to get clang to parse a file with -nocudainc, but without -nocudalib?
Only if clang finds the CUDA library location. Otherwise, clang refuses to do anything without
-nocudalib.
I see @micbou added -nocudalib unconditionally in the latest revision. Can anyone confirm that it won't break when a user passes --cuda-path too? If doing that is okay thten this is LGTM. Even without the cache I mentioned.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r13, 1 of 1 files at r14.
Reviewable status: 0 of 2 LGTMs obtained
ycmd/completers/cpp/flags.py, line 673 at r11 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
I see @micbou added
-nocudalibunconditionally in the latest revision. Can anyone confirm that it won't break when a user passes--cuda-pathtoo? If doing that is okay thten this is LGTM. Even without the cache I mentioned.
I was just going to comment about it. This flag is ignored for other languages (Clang just prints a warning that the flag is not used) so it shouldn't be a problem to always add it.
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained
ycmd/completers/cpp/flags.py, line 673 at r11 (raw file):
Can anyone confirm that it won't break when a user passes --cuda-path too?
Good point. Let me check.
micbou
force-pushed
the
fake-clang
branch
2 times, most recently
from
25839f8 to
e4d61e6
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r15, 1 of 1 files at r16.
Reviewable status: 0 of 2 LGTMs obtained
ycmd/completers/cpp/flags.py, line 673 at r11 (raw file):
Previously, micbou wrote…
Can anyone confirm that it won't break when a user passes --cuda-path too?
Good point. Let me check.
Seems to work. In fact, if Clang cannot find the CUDA installation path, passing --cuda-path is mandatory with or without -nocudalib.
ycmd/completers/cpp/flags.py, line 742 at r16 (raw file):
if not match: _logger.error( 'Unable to parse system header directories from output ' '%s returned by the command %s', stderr, clang_command )
Added some logging when it fails to parse the paths.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r14.
Reviewable status: 0 of 2 LGTMs obtained
ycmd/completers/cpp/flags.py, line 673 at r11 (raw file):
Previously, micbou wrote…
Seems to work. In fact, if Clang cannot find the CUDA installation path, passing
--cuda-pathis mandatory with or without-nocudalib.
Are you sure? I'm asking because I can get clang to print some paths if I pass -nocudalib and -nostdlib and I certainly did not install a CUDA library. Besides, @midchildan also said clang can work without --cuda-path if -nocudalib and -nocudainc are supplied.
ycmd/completers/cpp/flags.py, line 742 at r16 (raw file):
Previously, micbou wrote…
Added some logging when it fails to parse the paths.
Good idea.
|
ycmd/completers/cpp/flags.py, line 673 at r11 (raw file): Previously, bstaletic (Boris Staletic) wrote…
|
There was a problem hiding this comment.
Reviewed 1 of 1 files at r16.
Reviewable status: 0 of 2 LGTMs obtained
ycmd/completers/cpp/flags.py, line 673 at r11 (raw file):
Previously, midchildan wrote…
--cuda-pathcan be omitted if both--nocudaliband-nocudaincis specified. This is especially useful when using thenvidia-cuda-toolkitpackage from Debian. Since Debian's version of CUDA change file locations to comply with FHS, supplying--cuda-pathwon't work.
@midchildan Could you give this PR a test and see if ycmd correctly discovers system include paths for valid CUDA flag configurations?
|
ycmd/completers/cpp/flags.py, line 673 at r11 (raw file): Previously, bstaletic (Boris Staletic) wrote…
I can confirm it works on:
|
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale)
ycmd/completers/cpp/flags.py, line 673 at r11 (raw file):
Previously, midchildan wrote…
I can confirm it works on:
- Ubuntu with the
nvidia-cuda-toolkitpackage- macOS
Thanks for checking.
That's good enough for me.
|
☔ The latest upstream changes (presumably #1103) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Fixed the conflicts.
Reviewed 2 of 2 files at r18.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale)
|
I'd like to request the ability to specify and/or deduce Clang. I have a non-standard clang installed by macports. It isn't available on the command line as anything intelligible. If I can specify clang, the includes will be correct for the compiler I'm actually using. Alternatively, clang can be deduced from my compile_commands.json and then the includes will be correct too. Thoughts? |
There was a problem hiding this comment.
First though: If your custom clang is the first clang in your $PATH everything works as is.
As for customizing the clang we use for flag discovery, here's an idea:
- For compilation database users use the first argument. Problem is Windows and first argument possibly being
cl.exe. - For extra conf users add another optional key whose value is the path to clang, falling back to the one in
$PATH.
This doesn't work for clang-cl on posix operating systems, but we already don't support that.
Reviewed 1 of 1 files at r17, 2 of 2 files at r18.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale)
|
☔ The latest upstream changes (presumably #1121) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #1118) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
I won't work on this PR anymore as I think the focus should be on the Clangd completer. Closing.
Reviewed 5 of 5 files at r19.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale)
ycmd uses its own engine to complete and jump in include statements since libclang does not return completions in that scenario and the
clang_getIncludedFilefunction behaves in a strange way (it returns nothing after the opening quote or angle bracket). This is done by looking at the include flags from the list of user flags. The issue is that system includes are generally not part of these flags and thus completion and jumping are not available for system headers. A workaround is to call Clang as follows:and copy the paths under the
#include <...> search starts here:line to the list of flags. Unfortunately, this can't be done programmatically because Clang may not be available on the user system. But what if we provide our own executable that behaves like Clang when passing the-Eand-vflags?This is what the PR essentially does. A small program called
ycm_fake_clang(name subject to change) is built along the YCM library. This program takes a list of flags and a filename as its arguments and create the corresponding translation unit. When retrieving the user flags, ycmd calls it like this:where
[flag ...]is a list of flags relevant to the system includes used to parse the translation unit and[.ext]is the extension of the current file (libclang deduces the language from the file extension when the-xflag is not given). ycmd then extracts the system header directories from the output of this command (as if Clang was called with the-Eand-vflags) and adds them to the list of user flags.This works great on Linux and Windows but is not enough on macOS. Additional include paths are needed on this platform:
clang_includes/include: this folder is not automatically added with the-resource-dirflag;mac_includes/include: this one is from the Clang prebuilt binaries.Fixes #844
Fixes ycm-core/YouCompleteMe#2536
Fixes ycm-core/YouCompleteMe#2790
This change is