MaskRay / ccls

C/C++/ObjC language server supporting cross references, hierarchies, completion and semantic highlighting
Apache License 2.0
3.77k stars 259 forks source link

ccls should error when the (default) resource directory does not exist #853

Open mrexodia opened 2 years ago

mrexodia commented 2 years ago

Observed behavior

The getDefaultResourceDirectory returns a compile-time value CLANG_RESOURCE_DIRECTORY. When using binary packages this will almost never point to the correct value. When this directory does not exist ccls silently continues.

Expected behavior

I would expect ccls to error out and tell the user to point it to an appropriate resource directory.

Steps to reproduce

  1. Use ccls with default settings from VS Code on Macos
  2. It doesn't work correctly
  3. Search for hours for a fix and find out that the 'automatically supplied value by ccls' is meaningless.

System information

$ ccls --version
Homebrew ccls version <unknown>
clang version 13.0.0
madscientist commented 2 years ago

I would expect ccls to execute the clang pointed to by compile_commands.json with -print-resource-dir and use that value instead.

That wouldn't be correct. ccls does not use the clang mentioned in compile_commands.json to do its parsing: in fact it's very possible that compile_commands.json doesn't mention any clang at all: it might compile with GCC, or with some other compiler.

ccls is compiled with libclang and that does the parsing. The resource directory you want to use with ccls is the one that goes with the libclang that ccls is compiled with. Even if compile_commands.json does use clang, that might be a completely different version of clang than the one ccls is using.

This problem often happens if you compile ccls with the version of libclang you have installed, which points to a known location, then you update clang without recompiling ccls and old directory is deleted when the new version of clang is installed. You need to either rebuild ccls when you update clang, or else use the resourceDir setting.

I understand the frustration of trying to figure out what's wrong for hours though. This setting is discussed in the install section: https://github.com/MaskRay/ccls/wiki/Install#clang-resource-directory but maybe it would be good to have a "troubleshooting" section in the wiki... we have Debugging and FAQ.

mrexodia commented 2 years ago

I already updated the wiki for VS code, but at the very least I would expect ccls to exit if it tries to use a resource directory that doesn’t exist. Currently it silently fails.

That wouldn’t be correct

You might be technically correct, but from the user’s perspective this explanation is completely unhelpful and irrelevant. I am a user of ccls and as a user you shouldn’t have to worry about which flags a tool that’s is part of an extension for my editor is compiled with (because nobody actually compiles it themselves, you get binaries from a package manager).

My perspective is as follows: if I can execute the commands in compile_commands.json ccls should be smart enough to do so too (if only as hardcoded logic specifically for macos).

Anecdotally I’ve read some issue reports and this kind of thinking seems to be common here. I really like ccls when it works, but some small user-friendly tweaks and sensible defaults could go a long way here to make things good for new users and increase adoption.

madscientist commented 2 years ago

I agree, the error handling should be better and if these directories are indeed always required then ccls should fail when they don't exist. However I'm not 100% sure it's really true that these headers are always required. If you're working on an embedded system, for example, then maybe these native system clang resource headers are not used at all anyway.

The explanation is clearly not irrelevant since it exactly describes the problem. You are looking at it based on your personal perspective but your perspective doesn't encompass everyone's situation: a solution needs to work for everyone. For example it's entirely possible that there is no clang at all installed anywhere on the system (all of my systems are like that for one).

If you are not building ccls yourself, but instead installing it as a package, then the problem is with the package and you should report it there, not here. Any packaged version of ccls should come with the correct resource files for the version of libclang that ccls was built with, and the ccls binary should be configured to find them there by default. Any package that does NOT provide the correct resource files (or doesn't configure the right resourceDir value) is incorrectly built.

mrexodia commented 2 years ago

I don't think any further discussion about automatically doing the right thing will be fruitful, and I see that multiple other issues with a similar idea were closed.

I think a hard error when the resource directory does not exist is appropriate and would fit everyone's model. When pointing to an invalid directory the user/package is at fault there, because if they don't have any resource files they should have passed an empty string in the setting. I will update the issue to ask for this instead of things working out of the box for the users.

madscientist commented 2 years ago

Sounds good. Please also file a bug against whatever package you are using, so that others using that package won't run into this issue. The GitHub ccls project is a source code project it doesn't provide binaries and packages. Errors in the packaging of binaries, for example files that are missing as in your situation, are problems to be solved by the package provider, not by the ccls source project.

As you say, it will be great if the source project can make it more obvious when a package is incorrectly installed however.

mrexodia commented 2 years ago

The package cannot fix this. AppleClang just happens to store certain system headers in the resource directory and there is no way for the package to embed the correct value because the path contains the SDK version number which can change when you update xcode (and it might be different on PCs not sure).

Without extra code in ccls that automatically detects the right value this is something the user always has to set themselves (together with an extra -I command for the standard library).

On Sun, 19 Dec 2021 at 18:59, Paul Smith @.***> wrote:

Sounds good. Please also file a bug against whatever package you are using, so that others using that package won't run into this issue. The GitHub ccls project is a source code project it doesn't provide binaries and packages. Errors in the packaging of binaries, for example files that are missing as in your situation, are problems to be solved by the package provider, not by the ccls source project.

As you say, it will be great if the source project can make it more obvious when a package is incorrectly installed however.

— Reply to this email directly, view it on GitHub https://github.com/MaskRay/ccls/issues/853#issuecomment-997434554, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASYFGNLL7GAWACZ7Y3E6RDURYMQ7ANCNFSM5KLIZYZQ . You are receiving this because you authored the thread.Message ID: @.***>

madscientist commented 2 years ago

My perspective is as follows: if I can execute the commands in compile_commands.json ccls should be smart enough to do so too (if only as hardcoded logic specifically for macos).

Sorry I don't want to beat a dead horse but I just want to be sure it's clear: ccls does NOT execute the commands in compile_commands.json. It doesn't run your compiler at all. It is a separate program and all parsing and interpretation of code is done inside of ccls. The contents of compile_commands.json are irrelevant except for certain options such as -I, -D, etc. that let ccls know how to resolve preprocessor operations, and a few other options it recognizes (like -std etc.)

The package cannot fix this. AppleClang just happens to store certain system headers in the resource directory and there is no way for the package to embed the correct value because the path contains the SDK version number which can change when you update xcode (and it might be different on PCs not sure).

I think there's still some confusion.

My point is that it's incorrect for ccls to go looking for the resource files that are kept in whatever current version of AppleClang you have installed on your system. That may work, but it may also fail in subtle ways. The resource files are part of the compiler, not part of the system, and as I mentioned above ccls is not invoking your compiler: ccls IS your compiler (when it's parsing your code). Thus, all compiler resource files should be specific to ccls, not specific to whatever compiler you are using whether it's clang, GCC, MSVC, Intel, etc.

If the ccls package you have installed (I don't know where you got it: macports? Brew?) installs only the ccls binary and not any resource header files, that package is built incorrectly. A proper and complete ccls package should include BOTH the ccls binary AND the contents of the resource directory for the libclang used to build the ccls binary, and that ccls binary should be configured to look for those resource files in the package that was installed and NOT look for the resource files in whatever AppleClang version you are using.

mrexodia commented 2 years ago

I understand how ccls works and how compile_commands.json works, but I don’t think it would have worked if the resource directory that ccls was compiled with was correctly shipped. The system library stdint.h (or something, I forgot) was in AppleClang’s resource directory and it was included from another standard library file. So unless ccls ships with those files they wouldn’t be found regardless?

madscientist commented 2 years ago

Exactly. As I said, a ccls package should provide those files as part of its package, and the ccls binary in the package should be configured to look for those files in the package directory. A correct package for ccls should have files something like:

bin/ccls
share/ccls/include/inttypes.h
share/ccls/include/stdarg.h
 ...

(of course the exact paths are not important) and the ccls binary here should have its resourceDir value hardcoded to look in share/ccls.

Every compiler comes with its own set of internal headers, and those internal headers are specific to that compiler, and even to that VERSION of that compiler. Because these headers are part of the compiler, they don't even have to be actual valid C code: they can be any form at all.

Consider this: suppose you compile ccls with libclang version 5 which does not have a feature F. Then you install AppleClang version 6 which does have feature F, and in fact the resource files that come with version 6 make use of feature F so that they won't compile properly with version 5. Now if you point ccls to the resource files for version 6 it will fail, because ccls is using version 5.

mrexodia commented 2 years ago

I looked at the clang packages and it makes sense now. On my system there were two issues at play simultaneously that got things mixed up for me:

  1. Some -isystem need to be manually (again, why?) specified on Macos to find C++ headers: https://github.com/MaskRay/ccls/issues/191#issuecomment-735217374
  2. My homebrew had a mismatch in the resource-dir. ccls says: 20:11:30 initialize.cc:329 I use -resource-dir=/usr/local/Cellar/llvm/13.0.0/lib/clang/13.0.0 The actual path: /usr/local/Cellar/llvm/13.0.0_2/lib/clang/13.0.0

I have no idea where the _2 is coming from, but how could a package possibly account for this? I found the homebrew formula at https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/ccls.rb and it (correctly) depends on llvm (13) which includes an installed clang with the resource folder. I'll open an issue in homebrew to see if this is fixable somehow.

I ran brew reinstall --build-from-source ccls and now it seems to have the right resource directory compiled in. Perhaps it would easier if ccls embedded the contents of the resource-dir in the binary and extracted them to the .ccls-cache folder or something?

madscientist commented 2 years ago

Yes, I thought of that as well. It might be a good idea. A downside is that the contents of that directory (for my ccls which is built with Clang 11 I think) is 4.4M or 108,000 lines of code in about 124 files. So, not insubstantial. Also it would add a bit of time to ccls startup.

Still it might be worthwhile, just to get rid of this confusion.

Alternatively maybe the problem is that the ccls build/install should not be attempting to re-use the resource directory from the clang it was built with, and should ALWAYS make a local copy (as part of its install step for example) and be using that instead.

Barring either of these enhancements, I think that package build control files (like the homebrew formula you point to) must do extra work. Simply running "make install" and calling it a package is insufficient for ccls.

mrexodia commented 2 years ago

I commented on the commit that added the brew bottle and there was a reply with some questions: https://github.com/Homebrew/homebrew-core/commit/553a6738ebd713ddffa6cb4aedf69c5f712b00c2#commitcomment-62189759

Perhaps a contributor could jump in here because I'm pretty much clueless.

With regards to the 4.4M and startup time: gzipping it made it 1.4M for me and you could make it so it is only extracted as a fallback (when the resource-dir does not exist). Additionally you would only extract it once per project when populating the .ccls-cache (or actually only one time if you have some kind of global location available). If this is something you think would be merged I can try to do some cmake hackery to make it happen, but I don't want to waste my time with it otherwise.

gromgit commented 2 years ago

Not a ccls user, but I help out with Homebrew stuff when I can. As I understand it, the build process calls clang -print-resource-dir to get the resource dir path that's current at build time, so any LLVM upgrades after that may break ccls. Is there a reason this path can't be retrieved at run time instead (and therefore guaranteed to always be correct)?

carlocab commented 2 years ago

Alternatives to the above:

  1. Is there a way to customise the value used by ccls for clang -print-resource-directory at build time? If so, we can set this to a path that won't change when Homebrew llvm is rebuilt. (This is where the _2 comes from.)
  2. Is there a way to query ccls about what it thinks the value of clang -print-resource-directory is? If so, we can validate this path every time Homebrew llvm is rebuilt to make sure it doesn't refer to a non-existent path.

1 is preferable to 2 since it avoids having to rebuild ccls every time llvm is, so if that's possible we should do that.

gromgit commented 2 years ago
  1. Is there a way to customise the value used by ccls for clang -print-resource-directory at build time? If so, we can set this to a path that won't change when Homebrew llvm is rebuilt. (This is where the _2 comes from.)

If I'm not mistaken, we can set the CLANG_RESOURCE_DIR CMake var at build time, but:

$ /usr/local/opt/llvm/bin/clang -print-resource-dir
/usr/local/Cellar/llvm/13.0.0_2/lib/clang/13.0.0

there's that pesky version number at the end to deal with, even if we use opt_prefix:

$ ls /usr/local/opt/llvm/lib/clang/13.0.0/
include/  lib/  share/
carlocab commented 2 years ago

there's that pesky version number at the end to deal with, even if we use opt_prefix

That's fine; ccls always needs to be rebuilt with major version bumps to LLVM anyway.

gromgit commented 2 years ago

Ah ok, then I'll put up a PR after my lunch. : 😁

madscientist commented 2 years ago

As for zipping: yes that would help but of course now you've introduced a new dependency to build the project to link in libz or some other compression library, which is annoying.

As for extraction, it's not enough to do it once per project because you might have upgraded your ccls version which would require a new set of resource files; it would have to be done each time ccls starts (or else put into some ccls version-specific directory or something).

As always when you start thinking about the details it gets messier.

My personal opinion is that this is not the most viable way forward. My preference would be to update the default build and install process in the CMake files to (a) make a copy of the resource directory & contents in the ccls install directory, and (b) set the default resource directory to look there. Of course, the problem with THAT is that it needs to support relative paths so that the installation is relocatable. I don't know if that can be done.

However, I don't maintain this code and I'm not sure @MaskRay would be interested in any of these changes: it seems to me like the way ccls is used by its maintainer is simply to build it in the git repo then run it from there as well, rebuilding when a new clang is installed, and not installing it as a separate package. Anything that made that use case more complex might not be so welcome.

madscientist commented 2 years ago

@gromgit @carlocab If you check the thread for this issue you'll see that it's not correct to look up the resource dir at runtime. The resource dir that ccls uses must be the one for the version of clang that ccls was built with. It's not correct to set it to whatever version of clang is currently installed on the system.

Usually it will work because usually the resource files work OK across different versions. But it could also break, and perhaps in subtle ways.

gromgit commented 2 years ago

Got a fix for the Homebrew ccls formula, so that it doesn't have to be rebuilt on every LLVM minor revision. However, I need to add a test as @carlocab mentioned:

Is there a way to query ccls about what it thinks the value of clang -print-resource-directory is? If so, we can validate this path every time Homebrew llvm is rebuilt to make sure it doesn't refer to a non-existent path.

What's the canonical way of dumping the ccls embedded Clang resource path?

carlocab commented 2 years ago

@madscientist sure, that's why neither of my suggestions involved checking the resource directory at runtime.

gromgit commented 2 years ago

Homebrew ccls now uses a more stable resource dir path that should only "break" when Homebrew llvm itself gets version-bumped (which I gather requires a ccls rebuild anyway). Revisions at the formula level (like changes to the build process) should no longer break things.

madscientist commented 2 years ago

I don't know much about brew (or MacOS in general) so I can't say what's right or wrong: if it works great. Hopefully there are appropriate dependency relationships documented so that it's not possible to break things by uninstalling clang while leaving ccls installed or updating clang while leaving ccls un-updated.

Cheers!

gromgit commented 2 years ago

I don't know much about brew (or MacOS in general) so I can't say what's right or wrong: if it works great. Hopefully there are appropriate dependency relationships documented so that it's not possible to break things by uninstalling clang while leaving ccls installed or updating clang while leaving ccls un-updated.

Every package manager I've ever used allows you to force-uninstall or force-upgrade a subset of packages to a point where things break, but they'll warn you when you try, and make you jump through hoops (read: add long options that have to be typed precisely) in the process.

Homebrew is no different in this regard.

MaskRay commented 2 years ago

The Clang resource directory is a mix of:

In the past, there were just the first two. In recent years, many groups added miscellaneous files in the directory. On systems like FreeBSD and Linux musl, if you don't use x86 intrinsics, usually you don't need anything from the resource directory. Reporting a diagnostic when the resource directory does not exist is probably not a bad idea, but is so necessary. Thanks to changes like https://github.com/Homebrew/homebrew-core/pull/91706 which make the directory more stable.

Alternatively maybe the problem is that the ccls build/install should not be attempting to re-use the resource directory from the clang it was built with, and should ALWAYS make a local copy (as part of its install step for example) and be using that instead.

Agree. When LLVM and Clang packages have minor release upgrades, all downstream packages should be recompiled. llvm-project does try hard to not change ABI, though.

AppleClang has some weird configuration of default search paths. https://github.com/MaskRay/ccls/wiki/Build#macos should be up to update (I used Apple m1 few months ago).

carlocab commented 2 years ago

https://github.com/MaskRay/ccls/wiki/Build#macos should be up to update (I used Apple m1 few months ago).

Yes, that looks right. I've made a small improvement, though.

carlocab commented 2 years ago

FYI: we're bumping LLVM to 13.0.1 in Homebrew/homebrew-core#94339.

CI caught the stale reference to the old resource-dir, which contained a reference to 13.0.0 when it should now point to 13.0.1.

The PR now includes a rebuild for ccls to make sure installations of ccls are not broken by upgrading LLVM. (Only for Homebrew-installed ccls, of course. You'll need to rebuild ccls yourself after upgrading LLVM if you built it yourself against Homebrew llvm.)