Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[thakis]

Summary

Cited "widely spread opinion" seems less widely spread nowadays, and also it's impossible to build clang without clang-tools-extra in the monorepo while that's there.

If you want to build clang-tools-extra with monorepo, just add it to LLVM_ENABLE_PROJECTS like with other projects.

[smeenai]

Can you also add clang-tools-extra to LLVM_ALL_PROJECTS?

I'm personally in favor of this (explicit is better than implicit), but there are some bots that assume clang implies clang-tools-extra. @phosek ran into this with some Fuchsia bots, for example.

[thakis]

comments

[thakis]

In D58157#1395593, @smeenai wrote:

Can you also add clang-tools-extra to LLVM_ALL_PROJECTS?

Good point, done. Thanks!

I'm personally in favor of this (explicit is better than implicit), but there are some bots that assume clang implies clang-tools-extra. @phosek ran into this with some Fuchsia bots, for example.

That's fine, we can update the bots :)

(I'm also open to other approaches that allow us to build clang without clang-tools-extra in the monorepo, but since not everybody agrees with that comment there, this seemed like the simplest and most explicit approach.)

[rnk]

I didn't see any discussion about the idea that clang-tools-extra should be part of clang back in the RFC from Nov 2016, but I guess Mehdi added it here:
https://reviews.llvm.org/D26365#588659

I don't think anyone has pushed this idea since then, so I'm in favor of this change to make it independently toggleable again.

+some clang-tools people @sammccall @ilya-biryukov @ioeric

[mehdi_amini]

What is the motivation to change this now?

also it's impossible to build clang without clang-tools-extra in the monorepo while that's there.

That was the intent.

This was added with the expectation (and somehow consensus in the discussions I had at the time about the move to git) that the distinction with clang-tools-extra was mostly artificial and that we should have them "part of clang".
In the case of the monorepo, the plan was that we would move the clang-tools-extra folder from the top-level to be nested under clang after we move to GitHub. In the same idea, at the time the plan was that if we had gone with multiple repos instead of a monorepo, there wouldn't have been a separate clang-tools-extra repo.
Not building them would then be an opt-in, the same way as LLVM_BUILD_TOOLS is controlled for example.

[thakis]

In D58157#1395666, @mehdi_amini wrote:

What is the motivation to change this now?

also it's impossible to build clang without clang-tools-extra in the monorepo while that's there.

That was the intent.

This was added with the expectation (and somehow consensus in the discussions I had at the time about the move to git) that the distinction with clang-tools-extra was mostly artificial and that we should have them "part of clang".

I don't think there's agreement on this. Several clang folks I talked to on IRC are against this (eg AaronBallman ErichKeane myself), as were several people at the last dev meeting (forgot the names though) – with the argument that clang/ should be the compiler proper, and extra tools should be in extra tools.

I don't remember this being discussed on cfe-dev – did I miss it? Do you have a link?

In the case of the monorepo, the plan was that we would move the clang-tools-extra folder from the top-level to be nested under clang after we move to GitHub. In the same idea, at the time the plan was that if we had gone with multiple repos instead of a monorepo, there wouldn't have been a separate clang-tools-extra repo.
Not building them would then be an opt-in, the same way as LLVM_BUILD_TOOLS is controlled for example.

[sammccall]

I think I'm +1 for this change, and to preserving and strengthening the distinction between CTE and Clang.

I find the layering between clang and tools to be useful, as someone who works mostly on tools. When I have to make changes to clang-format or libclang, their entanglement with clang proper seems odd and bloat-y.

(I'm not actually sure about the bundling of clang-tidy, clangd etc together into clang-tools-extra, ISTM the bigger tools are really separate projects. But I don't know about the costs vs benefits of actually splitting them out)

[sammccall]

In the same idea, at the time the plan was that if we had gone with multiple repos instead of a monorepo, there wouldn't have been a separate clang-tools-extra repo.

Speaking only for clangd: if the project as a whole wasn't so strongly on the monorepo train, we would have moved to a separate clangd repository by now. But I don't know all the historical context, who was involved in the git discussions etc.

[rnk]

I think we have consensus, so I'll go ahead and stamp it. I don't see any zorg builders that this will affect, I think it will only affect developers... I guess just send a PSA to llvm-dev after you land it.

[mehdi_amini]

In D58157#1395716, @rnk wrote:

I think we have consensus,

Based on three comments in a revision? Seems strange to me.
I don't really care about this, so do whatever you want, but I would expect that "consensus" means an actual wider discussion (i.e. llvm-dev + cfe-dev).

[smeenai]

In D58157#1395716, @rnk wrote:

I think we have consensus, so I'll go ahead and stamp it. I don't see any zorg builders that this will affect, I think it will only affect developers... I guess just send a PSA to llvm-dev after you land it.

It'll affect FuchsiaBuilder, which will need to add clang-tools-extra to its LLVM_ENABLE_PROJECTS (cc @phosek).

[lebedev.ri]

cfe-commits (if not cfe-dev proper) should probably also be part of this disscussion.

[thakis]

In D58157#1395762, @mehdi_amini wrote:

In D58157#1395716, @rnk wrote:

I think we have consensus,

Based on three comments in a revision? Seems strange to me.
I don't really care about this, so do whatever you want, but I would expect that "consensus" means an actual wider discussion (i.e. llvm-dev + cfe-dev).

Please cite said discussion for when you added this, as requested above. Else, I think this has seen more discussion than the change that is undoing. It also has the support of several folks very actively working on clang and clang-tools-extra.

[rnk]

In D58157#1395762, @mehdi_amini wrote:

In D58157#1395716, @rnk wrote:

I think we have consensus,

Based on three comments in a revision? Seems strange to me.
I don't really care about this, so do whatever you want, but I would expect that "consensus" means an actual wider discussion (i.e. llvm-dev + cfe-dev).

Well, Sam's comment matters, since he works in the codebase in question. I also think I misread your first comment as being more positive on this. My (incorrect) interpretation was, "we were going to push clang-tools-extra under clang as part of the monorepo reorg, which is why we added this special case in cmake". And, given that we didn't do that reorganization and nobody intends to do it, it seems like the cmake should mirror the current code structure.

I'm not trying to approve things under the radar, I just want to expedite things without creating unneeded extra process. And, this change really just keeps us at parity with what we had with svn. We can always revisit the decision to merge the clang tools into clang. This particular change just gives us more options, today.

[mehdi_amini]

In D58157#1396072, @thakis wrote:

In D58157#1395762, @mehdi_amini wrote:

In D58157#1395716, @rnk wrote:

I think we have consensus,

Based on three comments in a revision? Seems strange to me.
I don't really care about this, so do whatever you want, but I would expect that "consensus" means an actual wider discussion (i.e. llvm-dev + cfe-dev).

Please cite said discussion for when you added this, as requested above.

Sorry, I don't have time to do archeology for you right now. But this is beside the point: your patch is changing a 2 years status quo, so my take on it is that it is *on you* to build the consensus to change this (maybe the consensus exists, I don't know, but this Phabricator diff alone seems quite light to demonstrate evidence of it).

Else, I think this has seen more discussion than the change that is undoing. It also has the support of several folks very actively working on clang and clang-tools-extra.

Again: I have no incentive to weigh one way or another with respect to what is the right way forward for clang-tools-extra, so I don't care what happens here.

[mehdi_amini]

In D58157#1396824, @rnk wrote:

And, this change really just keeps us at parity with what we had with svn. We can always revisit the decision to merge the clang tools into clang. This particular change just gives us more options, today.

Sure, looks fine. The cost of reversing it isn't that high either (updating a bunch of bot configurations).
It'd still be nice to identify the end goal with most cfe people (i.e. even if you land this right now, discussing the desired layout in the monorepo)

[rnk]

In D58157#1397274, @mehdi_amini wrote:

It'd still be nice to identify the end goal with most cfe people (i.e. even if you land this right now, discussing the desired layout in the monorepo)

I sent off http://lists.llvm.org/pipermail/cfe-dev/2019-February/061290.html, so the larger community is aware of this change. I don't have much to add to a larger discussion about possible code reorganizations, but I suppose folks may come forward to express their opinions.

[phosek]

We need to update https://github.com/llvm/llvm-zorg/blob/master/zorg/buildbot/builders/FuchsiaBuilder.py#L102 as well as our downstream bots, but other than that I'm fine with this change.

[mehdi_amini]

In D58157#1397302, @rnk wrote:

In D58157#1397274, @mehdi_amini wrote:

It'd still be nice to identify the end goal with most cfe people (i.e. even if you land this right now, discussing the desired layout in the monorepo)

I sent off http://lists.llvm.org/pipermail/cfe-dev/2019-February/061290.html, so the larger community is aware of this change. I don't have much to add to a larger discussion about possible code reorganizations, but I suppose folks may come forward to express their opinions.

Thanks!

[thakis]

Thanks for starting the thread, Reid.

In D58157#1397260, @mehdi_amini wrote:

Sorry, I don't have time to do archeology for you right now. But this is beside the point: your patch is changing a 2 years status quo, so my take on it is that it is *on you* to build the consensus to change this (maybe the consensus exists, I don't know, but this Phabricator diff alone seems quite light to demonstrate evidence of it).

The "2 years" isn't very relevant since the monorepo was some unofficial optional thing for most of those 2 years imho.

Else, I think this has seen more discussion than the change that is undoing. It also has the support of several folks very actively working on clang and clang-tools-extra.

Again: I have no incentive to weigh one way or another with respect to what is the right way forward for clang-tools-extra, so I don't care what happens here.

Cool.

CMakeLists.txt

foreach(proj ${LLVM_ALL_PROJECTS})
     if ("${proj}" IN_LIST LLVM_ENABLE_PROJECTS)
       message(STATUS "${proj} project is enabled")
       set(SHOULD_ENABLE_PROJECT TRUE)
       set(PROJ_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../${proj}")
       if(NOT EXISTS "${PROJ_DIR}" OR NOT IS_DIRECTORY "${PROJ_DIR}")
         message(FATAL_ERROR "LLVM_ENABLE_PROJECTS requests ${proj} but directory not found: ${PROJ_DIR}")
       endif()
       set(LLVM_EXTERNAL_${upper_proj}_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../${proj}")
-      # There is a widely spread opinion that clang-tools-extra should be merged
-      # into clang. The following simulates it by always enabling clang-tools-extra
-      # when enabling clang.
-      if (proj STREQUAL "clang")
-        set(LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../clang-tools-extra")
-      endif()
     else()
       message(STATUS "${proj} project is disabled")
       set(SHOULD_ENABLE_PROJECT FALSE)
     endif()
     # Force `LLVM_TOOL_${upper_proj}_BUILD` variables to have values that
     # corresponds with `LLVM_ENABLE_PROJECTS`. This prevents the user setting
     # `LLVM_TOOL_${upper_proj}_BUILD` variables externally. At some point
     # we should deprecate allowing users to set these variables by turning them