subject:"\[PATCH\] D91859\: \[clangd\] Fix shared\-lib builds"

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-24 Thread Kadir Cetinkaya via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf726101b6240: [clangd] Fix shared-lib builds (authored by 
kadircet).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/marshalling/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  llvm/cmake/modules/FindGRPC.cmake

Index: llvm/cmake/modules/FindGRPC.cmake
===
--- llvm/cmake/modules/FindGRPC.cmake
+++ llvm/cmake/modules/FindGRPC.cmake
@@ -112,7 +112,7 @@
 
   add_clang_library(${LibraryName} ${GeneratedProtoSource}
 PARTIAL_SOURCES_INTENDED
-LINK_LIBS grpc++ protobuf)
+LINK_LIBS PUBLIC grpc++ protobuf)
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -146,7 +146,8 @@
 if (CLANGD_ENABLE_REMOTE)
   target_link_libraries(ClangdTests
 PRIVATE
-clangdRemoteMarshalling)
+clangdRemoteMarshalling
+RemoteIndexProto)
 endif()
 
 if (CLANGD_BUILD_XPC)
Index: clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
@@ -17,6 +17,4 @@
   RemoteIndexProto
   RemoteIndexServiceProto
   clangdRemoteMarshalling
-
-  grpc++
   )
Index: clang-tools-extra/clangd/index/remote/marshalling/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/marshalling/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/marshalling/CMakeLists.txt
@@ -4,7 +4,6 @@
   LINK_LIBS
   RemoteIndexProto
 
-  protobuf
   clangDaemon
   clangdSupport
 
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -3,6 +3,12 @@
   generate_protos(RemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
+  # FIXME: Move this into generate_protos. Currently we only mention proto
+  # filename as a dependency, but linking requires target name.
+  target_link_libraries(RemoteIndexServiceProto
+PRIVATE
+RemoteIndexProto
+)
   include_directories(${CMAKE_CURRENT_BINARY_DIR})
   include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../)
 
@@ -18,8 +24,8 @@
 RemoteIndexProto
 RemoteIndexServiceProto
 clangdRemoteMarshalling
-protobuf
-grpc++
+clangBasic
+clangDaemon
 clangdSupport
 
 DEPENDS
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -168,17 +168,18 @@
   add_subdirectory(xpc)
 endif ()
 
+if (CLANGD_ENABLE_REMOTE)
+  include(FindGRPC)
+endif()
+
 if(CLANG_INCLUDE_TESTS)
-add_subdirectory(test)
-add_subdirectory(unittests)
+  add_subdirectory(test)
+  add_subdirectory(unittests)
 endif()
 
 # FIXME(kirillbobyrev): Document this in the LLVM docs once remote index is stable.
 option(CLANGD_ENABLE_REMOTE "Use gRPC library to enable remote index support for Clangd" OFF)
 set(GRPC_INSTALL_PATH "" CACHE PATH "Path to gRPC library manual installation.")
 
-if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
-endif()
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-24 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 307281.
kadircet added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

- Link protobuf and grpc++ publicly to generated targets


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/marshalling/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  llvm/cmake/modules/FindGRPC.cmake

Index: llvm/cmake/modules/FindGRPC.cmake
===
--- llvm/cmake/modules/FindGRPC.cmake
+++ llvm/cmake/modules/FindGRPC.cmake
@@ -112,7 +112,7 @@
 
   add_clang_library(${LibraryName} ${GeneratedProtoSource}
 PARTIAL_SOURCES_INTENDED
-LINK_LIBS grpc++ protobuf)
+LINK_LIBS PUBLIC grpc++ protobuf)
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -146,7 +146,8 @@
 if (CLANGD_ENABLE_REMOTE)
   target_link_libraries(ClangdTests
 PRIVATE
-clangdRemoteMarshalling)
+clangdRemoteMarshalling
+RemoteIndexProto)
 endif()
 
 if (CLANGD_BUILD_XPC)
Index: clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/server/CMakeLists.txt
@@ -17,6 +17,4 @@
   RemoteIndexProto
   RemoteIndexServiceProto
   clangdRemoteMarshalling
-
-  grpc++
   )
Index: clang-tools-extra/clangd/index/remote/marshalling/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/marshalling/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/marshalling/CMakeLists.txt
@@ -4,7 +4,6 @@
   LINK_LIBS
   RemoteIndexProto
 
-  protobuf
   clangDaemon
   clangdSupport
 
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -3,6 +3,12 @@
   generate_protos(RemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
+  # FIXME: Move this into generate_protos. Currently we only mention proto
+  # filename as a dependency, but linking requires target name.
+  target_link_libraries(RemoteIndexServiceProto
+PRIVATE
+RemoteIndexProto
+)
   include_directories(${CMAKE_CURRENT_BINARY_DIR})
   include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../)
 
@@ -18,8 +24,8 @@
 RemoteIndexProto
 RemoteIndexServiceProto
 clangdRemoteMarshalling
-protobuf
-grpc++
+clangBasic
+clangDaemon
 clangdSupport
 
 DEPENDS
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -168,17 +168,18 @@
   add_subdirectory(xpc)
 endif ()
 
+if (CLANGD_ENABLE_REMOTE)
+  include(FindGRPC)
+endif()
+
 if(CLANG_INCLUDE_TESTS)
-add_subdirectory(test)
-add_subdirectory(unittests)
+  add_subdirectory(test)
+  add_subdirectory(unittests)
 endif()
 
 # FIXME(kirillbobyrev): Document this in the LLVM docs once remote index is stable.
 option(CLANGD_ENABLE_REMOTE "Use gRPC library to enable remote index support for Clangd" OFF)
 set(GRPC_INSTALL_PATH "" CACHE PATH "Path to gRPC library manual installation.")
 
-if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
-endif()
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-22 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev added a comment.

In D91859#2410252 , @nridge wrote:

> In D91859#2410002 , @kbobyrev wrote:
>
>> Ah, right. I think `clangd-remote-index` needs `protobuf` dependency. 
>> @nridge, I believe this should fix the problem you're seeing.
>
> Not 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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-22 Thread Nathan Ridge via Phabricator via cfe-commits

nridge added a comment.

In D91859#2410002 , @kbobyrev wrote:

> Ah, right. I think `clangd-remote-index` needs `protobuf` dependency. 
> @nridge, I believe this should fix the problem you're seeing.

Not 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-22 Thread Nathan Ridge via Phabricator via cfe-commits

nridge added a comment.

In D91859#2410166 , @kadircet wrote:

> In D91859#2409743 , @nridge wrote:
>
>> I applied the patch locally and it fixes most of the linker errors but I'm 
>> still seeing one:
>>
>> [...]
>
> 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 ?

The cmake command I'm using for this build building is something like:

  cmake -G Ninja  -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=OFF


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-22 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev added a comment.

In D91859#2410166 , @kadircet wrote:

> In D91859#2409743 , @nridge wrote:
>
>> 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>  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)
>
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-22 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added a comment.

In D91859#2409743 , @nridge wrote:

> 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  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)

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 ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-22 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev accepted this revision.
kbobyrev added a comment.
This revision is now accepted and ready to land.

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.

In D91859#2409419 , @kbobyrev wrote:

> (also please add "Fixes https://github.com/clangd/clangd/issues/598; to the 
> patch description to close the related issue on GH)

Otherwise LGTM + patch description :)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-21 Thread Nathan Ridge via Phabricator via cfe-commits

nridge added a comment.

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::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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-21 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 306830.
kadircet marked an inline comment as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CMakeLists.txt


Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -143,7 +143,8 @@
 if (CLANGD_ENABLE_REMOTE)
   target_link_libraries(ClangdTests
 PRIVATE
-clangdRemoteMarshalling)
+clangdRemoteMarshalling
+RemoteIndexProto)
 endif()
 
 if (CLANGD_BUILD_XPC)
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -3,6 +3,12 @@
   generate_protos(RemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
+  # FIXME: Move this into generate_protos. Currently we only mention proto
+  # filename as a dependency, but linking requires target name.
+  target_link_libraries(RemoteIndexServiceProto
+PRIVATE
+RemoteIndexProto
+)
   include_directories(${CMAKE_CURRENT_BINARY_DIR})
   include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../)
 
@@ -20,6 +26,8 @@
 clangdRemoteMarshalling
 protobuf
 grpc++
+clangBasic
+clangDaemon
 clangdSupport
 
 DEPENDS
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -167,17 +167,18 @@
   add_subdirectory(xpc)
 endif ()
 
+if (CLANGD_ENABLE_REMOTE)
+  include(FindGRPC)
+endif()
+
 if(CLANG_INCLUDE_TESTS)
-add_subdirectory(test)
-add_subdirectory(unittests)
+  add_subdirectory(test)
+  add_subdirectory(unittests)
 endif()
 
 # FIXME(kirillbobyrev): Document this in the LLVM docs once remote index is 
stable.
 option(CLANGD_ENABLE_REMOTE "Use gRPC library to enable remote index support 
for Clangd" OFF)
 set(GRPC_INSTALL_PATH "" CACHE PATH "Path to gRPC library manual 
installation.")
 
-if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
-endif()
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)


Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -143,7 +143,8 @@
 if (CLANGD_ENABLE_REMOTE)
   target_link_libraries(ClangdTests
 PRIVATE
-clangdRemoteMarshalling)
+clangdRemoteMarshalling
+RemoteIndexProto)
 endif()
 
 if (CLANGD_BUILD_XPC)
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -3,6 +3,12 @@
   generate_protos(RemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
+  # FIXME: Move this into generate_protos. Currently we only mention proto
+  # filename as a dependency, but linking requires target name.
+  target_link_libraries(RemoteIndexServiceProto
+PRIVATE
+RemoteIndexProto
+)
   include_directories(${CMAKE_CURRENT_BINARY_DIR})
   include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../)
 
@@ -20,6 +26,8 @@
 clangdRemoteMarshalling
 protobuf
 grpc++
+clangBasic
+clangDaemon
 clangdSupport
 
 DEPENDS
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -167,17 +167,18 @@
   add_subdirectory(xpc)
 endif ()
 
+if (CLANGD_ENABLE_REMOTE)
+  include(FindGRPC)
+endif()
+
 if(CLANG_INCLUDE_TESTS)
-add_subdirectory(test)
-add_subdirectory(unittests)
+  add_subdirectory(test)
+  add_subdirectory(unittests)
 endif()
 
 # FIXME(kirillbobyrev): Document this in the LLVM docs once remote index is stable.
 option(CLANGD_ENABLE_REMOTE "Use gRPC library to enable remote index support for Clangd" OFF)
 set(GRPC_INSTALL_PATH "" CACHE PATH "Path to gRPC library manual installation.")
 
-if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
-endif()
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-21 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added a subscriber: nridge.
kadircet added a comment.

> 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 :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-21 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev added a comment.

(also please add "Fixes https://github.com/clangd/clangd/issues/598; to the 
patch description to close the related issue on GH)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-20 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev added a comment.

In D91859#2408064 , @sammccall wrote:

> 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.

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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-20 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

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.




Comment at: clang-tools-extra/clangd/index/remote/CMakeLists.txt:6
 GRPC)
+  target_link_libraries(RemoteIndexServiceProto
+PRIVATE

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"?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-20 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-20 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev requested changes to this revision.
kbobyrev added a comment.
This revision now requires changes to proceed.

Thank you for taking care of thiis!

The complete fix, however, should include two more things:

1. 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.

2. `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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91859/new/

https://reviews.llvm.org/D91859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

2020-11-20 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet created this revision.
kadircet added reviewers: sammccall, kbobyrev.
Herald added subscribers: cfe-commits, usaxena95, arphaman, mgorny.
Herald added a project: clang.
kadircet requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91859

Files:
  clang-tools-extra/clangd/index/remote/CMakeLists.txt


Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -3,6 +3,10 @@
   generate_protos(RemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
+  target_link_libraries(RemoteIndexServiceProto
+PRIVATE
+RemoteIndexProto
+)
   include_directories(${CMAKE_CURRENT_BINARY_DIR})
   include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../)
 
@@ -20,6 +24,8 @@
 clangdRemoteMarshalling
 protobuf
 grpc++
+clangBasic
+clangDaemon
 clangdSupport
 
 DEPENDS


Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -3,6 +3,10 @@
   generate_protos(RemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
+  target_link_libraries(RemoteIndexServiceProto
+PRIVATE
+RemoteIndexProto
+)
   include_directories(${CMAKE_CURRENT_BINARY_DIR})
   include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../)
 
@@ -20,6 +24,8 @@
 clangdRemoteMarshalling
 protobuf
 grpc++
+clangBasic
+clangDaemon
 clangdSupport
 
 DEPENDS
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

[PATCH] D91859: [clangd] Fix shared-lib builds

17 matches

Site Navigation

Mail list logo

Footer information