Details
- Reviewers
sammccall kbobyrev - Commits
- rGf726101b6240: [clangd] Fix shared-lib builds
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thank you for taking care of thiis!
The complete fix, however, should include two more things:
- Moving this
if (CLANGD_ENABLE_REMOTE) include(FindGRPC) endif()
before that
if(CLANG_INCLUDE_TESTS) add_subdirectory(test) add_subdirectory(unittests) endif()
Otherwise by the time unittests are compiled the include paths are not set appropriately and we will see the errors with Protobuf version mismatch through transitive includes etc.
- ClangdTests should be linked against RemoteIndexProto, I didn't notice this problem before since linking clangdRemoteMarshalling already did the trick for static index builds but now we need to do that explicitly in clangd/unittests/CMakeLists.txt.
Actually, it would be safer if FindGRPC was also higher than add_subdirectory(tool): right now it only includes remote/index/Client.h and Client.h does not depend on any gRPC/Protobuf headers for now but that might change at some point and break builds unexpectedly. I would suggest just moving it as high as possible, there is no harm in that apart from decoupling FindGRPC from add_subdirectory(index/remote) and related code but we kind of have to do that anyway.
I'm not totally following the discussion on the more complete fix (LMK if you want me to dig into that), but if the build is broken in some configurations we should likely prioritize fixing that over ensuring the fix is conceptually complete.
Otherwise by the time unittests are compiled the include paths are not set appropriately and we will see the errors with Protobuf version mismatch through transitive includes etc.
I don't think that's how CMake works, the whole CMakeLists tree is parsed before anything is compiled, so it shouldn't race like that.
| clang-tools-extra/clangd/index/remote/CMakeLists.txt | ||
|---|---|---|
| 6 | oh, this is sad, because we specify the proto dependencies to generate_protos but it doesn't actually set up all the dep relationships we need (and can't because we specify the filename, not the rule). Can you add a "FIXME: move this into generate_protos somehow"? | |
Yes, this is basically an expanded context from a discussion me and Kadir had over the last couple of days. Sorry for confusion, the "complete" means "it's still not working for me without these changes". And the configuration we're looking at is simply shared libs build.
Otherwise by the time unittests are compiled the include paths are not set appropriately and we will see the errors with Protobuf version mismatch through transitive includes etc.
I don't think that's how CMake works, the whole CMakeLists tree is parsed before anything is compiled, so it shouldn't race like that.
This may look counter-intuitive but this is how it works on my machine. MarshallingTests.cpp entry in compile_commands.json does not have gRPC include path. (rg "MarshallingTests.cpp" compile_commands.json | rg grpc is empty). When I put FindGRPC before add_directory(unittests) the compile commands have the right include.
(also please add "Fixes https://github.com/clangd/clangd/issues/598" to the patch description to close the related issue on GH)
I don't think that's how CMake works, the whole CMakeLists tree is parsed before anything is compiled, so it shouldn't race like that.
That was also my mental model, but it looks like include_directories statement within the FindGRPC submodule only affects targets created afterwards (lexicographically). Even though cmake docs say that it should affect everything within that cmakelists file (https://cmake.org/cmake/help/latest/command/include_directories.html).
I've tried to migrate into target_include_directories instead(i.e. only introduce the include header to targets generated via generate_protos and propagate into their dependents), and it works great with targets introduced via add_clang_executable or add_unittest, but for some reason I could not figure out; propagation doesn't work for targets introduced via add_clang_library :(.
And indeed my compile commands also didn't contain the necessary -isystem, but I had an acceptable version of protobuf installed in /usr/include even though i tried really hard to delete all the remaining packages, so it was working on my machine :/
Hopefully it should be good now. It is quite nice that @nridge has also noticed this while we are working on a fix, so I hope he can try this patch on his setup as well :)
I applied the patch locally and it fixes most of the linker errors but I'm still seeing one:
[6/393] Linking CXX executable bin/clangd-index-server FAILED: bin/clangd-index-server : && /usr/bin/clang++-10 -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -fuse-ld=gold -Wl,-O3 -Wl,--gc-sections tools/clang/tools/extra/clangd/index/remote/server/CMakeFiles/clangd-index-server.dir/Server.cpp.o -o bin/clangd-index-server -Wl,-rpath,"\$ORIGIN/../lib" -lpthread lib/libRemoteIndexServiceProto.so.12git lib/libclangdRemoteMarshalling.so.12git -lgrpc++ lib/libclangDaemon.so.12git lib/libclangdSupport.so.12git lib/libRemoteIndexProto.so.12git lib/libLLVMSupport.so.12git -Wl,-rpath-link,llvm/prod-build/lib && : tools/clang/tools/extra/clangd/index/remote/server/CMakeFiles/clangd-index-server.dir/Server.cpp.o:Server.cpp:function llvm::detail::stream_operator_format_adapter<clang::clangd::remote::(anonymous namespace)::RemoteIndexServer::TextProto>::format(llvm::raw_ostream&, llvm::StringRef): error: undefined reference to 'google::protobuf::Message::DebugString[abi:cxx11]() const' clang: error: linker command failed with exit code 1 (use -v to see invocation)
Ah, right. I think clangd-remote-index needs protobuf dependency. @nridge, I believe this should fix the problem you're seeing. I'm somewhat confused as to why I'm not seeing the same problem though but it makes total sense to me.
Otherwise LGTM + patch description :)
I am not able to reproduce the failure. Maybe we should just make the dependencies introduced in generate_protos for grpc++ and protobuf PUBLIC, to ensure they propagate into users (i believe that's the default for my configuration for whatever reason). Can you share your cmake configuration ?
I agree that would be a good thing but since Server.cpp explicitly uses DebugString I think explicitly specifying protobuf dependency there is still a way to go.
The cmake command I'm using for this build building is something like:
cmake -G Ninja <sourcedir> -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DLLVM_USE_LINKER=gold -DBUILD_SHARED_LIBS=ON \
-DLLVM_OPTIMIZED_TABLEGEN=ON -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_CCACHE_BUILD=ON -DCMAKE_BUILD_TYPE=Release \
-DCLANGD_ENABLE_REMOTE=ON \
-DLLVM_APPEND_VC_REV=OFFNot sure if this is what you meant, but I fixed it by adding a protobuf dependency to clangd-index-server in clang-tools-extra/clangd/index/remote/server/CMakeLists.txt.
After fixing that (and one more shared-libs dependency issue unrelated to clangd), my build completed successfully.
Uh, yeah, sorry, I meant clangd-index-server, just used the wrong name for some reason. Thanks for checking!
oh, this is sad, because we specify the proto dependencies to generate_protos but it doesn't actually set up all the dep relationships we need (and can't because we specify the filename, not the rule).
Can you add a "FIXME: move this into generate_protos somehow"?