[PATCH] D145228: [clangd] Add clangd headers to install targets

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko abandoned this revision.
ivanmurashko added a comment.

The diff is not required after https://reviews.llvm.org/D145302 deployed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

> If we consider the bare minimum with the only goal to build outside LLVM 
> source tree then we don’t need to copy all internal headers.  At the case the 
> D145302  can be modified and we can end up 
> with the only one header that is required to copy:  
> clang-tools-extra/clangd/tool/ClangdMain.h.

I just updated D145302  with the changes. The 
D145228  can be abandoned if the changes are 
OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D145228#4174829 , @sammccall wrote:

> I think this is a bit abstract though. Concretely, what API do you need here? 
> e.g. which headers do you want to include, to what end?

If we consider the bare minimum with the only goal to build outside LLVM source 
tree then we don’t need to copy all internal headers.  At the case the D145302 
 can be modified and we can end up with the 
only one header that is required to copy:  
clang-tools-extra/clangd/tool/ClangdMain.h.

For further customization one might require additional headers to be installed. 
That can be done once it’s required or via one diff that install all possible 
headers.

What do you think about it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D145228#4174829 , @sammccall wrote:

> @aaron.ballman
>
> If someone wants to work on producing a more modular, component-based design, 
> that's probably a good idea (but tricky to get right). Probably a good place 
> to start is working out what pieces can be moved into separate libraries with 
> e.g. a coherent layering story, thinking about how they might be reused in 
> other tools etc.
>
> Exposing all clangd's headers as-is is not that. I don't think the idea that 
> llvm-project is modular is at odds with the idea that leaf subprojects exist, 
> though clangd is big enough that it may make sense to split up. And private 
> headers (those that live next to source files in the tree) are not installed 
> (with the recent exception of `clang-tidy`, which IMO should have been 
> restructured to publish in this way).

Ah, okay, thank you for the clarification! I agree we have to go about 
splitting the project up into libraries very carefully and this approach isn't 
appropriate. But I was getting the impression there was push-back against the 
idea of making clangd into a series of libraries in general and that worried me.

> FWIW, I do think clang is a fair example of a project where the default of 
> "everything is a public library" has made the library design somewhat 
> incoherent and hard to evolve. OTOH it's enabled tons of useful stuff, so...

IMO, we've not really had too many issues evolving Clang's libraries, but I do 
think there's room for improvement (it's hard to figure out how to split up 
Sema, for example). As you say, it's not an easy problem to solve but it 
enables a lot of really useful stuff we wouldn't otherwise be able to achieve 
as easily.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

@aaron.ballman

If someone wants to work on producing a more modular, component-based design, 
that's probably a good idea (but tricky to get right). Probably a good place to 
start is working out what pieces can be moved into separate libraries with e.g. 
a coherent layering story, thinking about how they might be reused in other 
tools etc.

Exposing all clangd's headers as-is is not that. I don't think the idea that 
llvm-project is modular is at odds with the idea that leaf subprojects exist, 
though clangd is big enough that it may make sense to split up. And private 
headers (those that live next to source files in the tree) are not installed 
(with the recent exception of `clang-tidy`, which IMO should have been 
restructured to publish in this way).

FWIW, I do think clang is a fair example of a project where the default of 
"everything is a public library" has made the library design somewhat 
incoherent and hard to evolve. OTOH it's enabled tons of useful stuff, so...

@ivanmurashko

Yes, my experience is that in practice people object to e.g. removing things 
without replacement, even in an "unstable" API.
Heck, *I* object to that, because it's often disruptive, and in practice people 
do put effort into making transitions easier.

I think this is a bit abstract though. Concretely, what API do you need here? 
e.g. which headers do you want to include, to what end?

My impression so far is that you actually don't need an API, but rather to link 
some extra libraries into an otherwise-unmodified clangd binary, and that being 
able to call `clangdMain()` from an external source file would make this easier 
in your build system. That seems like a problem worth solving, but installing 
all clangd's private headers into /usr/lib doesn't seem like a particularly 
direct solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D145228#4170864 , @sammccall wrote:

> Clangd isn't designed as a collection of libraries.

clangd is not my area of expertise, so I don't intend to step into the middle 
of this with my comment, but: one of the tenants of the LLVM project is that 
it's a modular set of reusable components. clangd being designed such that it 
cannot be used as a library goes against the grain in that regard, so I'm 
surprised to see push-back against attempts to make a more modular, 
component-based design. What am I missing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D145228#4174543 , @kadircet wrote:

> 

I understand concerns about maintenance cost for the change. But I dare to ask 
why you think it is so high? Perhaps there are different expectations from the 
feature. I’m not asking to support any API stability or anything like this. 
IMHO, the maintenance cost for the feature should be on people who would like 
to use it. Clangd can only export headers and libraries as they are with one 
addition to allow main function overriding. Users of the feature should be 
prepared that it might change significantly at any point even within release. 
There is smaller but similar problem with other many other LLVM APIs, that are 
significantly changed especially between releases. I hope this expectation 
should make the maintenance cost for the change smaller and we are happy to 
support them if it is needed.

There are some additional context about the change. We have a separate build 
system (not CMake) for in-house apps. The LVVM is considered as a third-party 
with corresponding libs and headers. The approach allows us to create in-house 
clang tools such as lint and refactoring tools compiling from the same sources 
against multiple clang versions and variants. The approach is supported by LLVM 
modular structure that gives an ability to combine different pieces to create 
powerful customized in-house apps. Use forks of LLVM is one of the obvious way 
to use the feature, but it has high maintenance cost and it makes harder to 
contribute back bug fixes and features. Because we have to pay this cost anyway 
we can support this feature in upstream for everybody and make LLVM even more 
re-usable. If we need to support it in another build system, we can do it as 
well (up to the degree we can in LLVM).

What do you think about it? Have I missed something important?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

i agree with Sam's concerns here. clangd isn't designed to be consumed as a 
library, but rather as a binary through LSP. increasing surface are here and 
letting people build applications on top of clangd internals would create extra 
maintenance burden that we're not equipped to support.

do you have any specific use case that's pending on this change, or is this a 
change that might allow things in the future? if you have a specific need it 
would be better if you can share it directly and we can look for other 
solutions instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D145228#4170826 , @DmitryPolukhin 
wrote:

> In D145228#4170792 , @sammccall 
> wrote:
>
>>> The install target for clang distributes the clangd static libs
>>
>> I don't think this was ever intended, looks like an accidental side-effect 
>> of using LLVM's many cmake macros.
>> Can we fix this instead?
>
> Why not allow people building custom clangd outside of LLVM repo?

Clangd isn't designed as a collection of libraries.
In a separate build system where it was (accidentally) easy to consume these, 
we've had issues with people using things like ParsedAST as a general interface 
to clang.
There *was* a goal to have it possible to essentially embed the whole thing 
into a different system. That's pretty involved and AFAIK has one user today, 
as such integrating with the build system seems like a better tradeoff than 
relying on all clang users installing this library + headers, dealing with 
version skew between the headers and the consuming application, etc.

> There was exactly the same issue with clang-tidy (see D73236 
> )

Sure, if clang-tidy maintainers are happy to support clang-tidy as engine + 
library checks for running against ASTs, in addition to the clang-tidy tool per 
se, I guess that makes sense. I don't think it follows that it makes sense for 
clangd.

> In comparison with headers, libraries takes much more space

As I said, including the libraries looks like an oversight, the intent was to 
distribute clangd as a binary only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a comment.

In D145228#4170792 , @sammccall wrote:

>> The install target for clang distributes the clangd static libs
>
> I don't think this was ever intended, looks like an accidental side-effect of 
> using LLVM's many cmake macros.
> Can we fix this instead?

Why not allow people building custom clangd outside of LLVM repo? There was 
exactly the same issue with clang-tidy (see D73236 
) and it was fixed with adding header to the 
distribution. In comparison with headers, libraries takes much more space so it 
is just very tiny increase in size and significant convenience for people who 
need this feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

> The install target for clang distributes the clangd static libs

I don't think this was ever intended, looks like an accidental side-effect of 
using LLVM's many cmake macros.
Can we fix this instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Shoaib Meenai via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: sammccall, alexfh, smeenai, aaron.ballman.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
ivanmurashko requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

The install target for clang distributes the clangd static libs but missing 
corresponding headers. The diff adds necessary headers. That opens a 
possibility to create custom clangd builds outside LLVM repo.

Test Plan:

  ninja install-clangd-headers

see the headers installed at the install folder


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145228

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


Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -218,3 +218,26 @@
 
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)
+
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  install(DIRECTORY .
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/clangd"
+COMPONENT clangd-headers
+FILES_MATCHING
+PATTERN "*.h"
+)
+  install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
+COMPONENT clangd-headers
+FILES_MATCHING
+PATTERN "CMakeFiles" EXCLUDE
+PATTERN "*.inc"
+)
+  add_custom_target(clangd-headers)
+  set_target_properties(clangd-headers PROPERTIES FOLDER "Misc")
+  if(NOT LLVM_ENABLE_IDE)
+add_llvm_install_targets(install-clangd-headers
+ DEPENDS clangd-headers
+ COMPONENT clangd-headers)
+  endif()
+endif()


Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -218,3 +218,26 @@
 
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)
+
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  install(DIRECTORY .
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/clangd"
+COMPONENT clangd-headers
+FILES_MATCHING
+PATTERN "*.h"
+)
+  install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
+COMPONENT clangd-headers
+FILES_MATCHING
+PATTERN "CMakeFiles" EXCLUDE
+PATTERN "*.inc"
+)
+  add_custom_target(clangd-headers)
+  set_target_properties(clangd-headers PROPERTIES FOLDER "Misc")
+  if(NOT LLVM_ENABLE_IDE)
+add_llvm_install_targets(install-clangd-headers
+ DEPENDS clangd-headers
+ COMPONENT clangd-headers)
+  endif()
+endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits