Page MenuHomePhabricator

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

Authored by thakis on Feb 12 2019, 4:29 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

thakis created this revision.Feb 12 2019, 4:29 PM

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.

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

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

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.

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.

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)

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 accepted this revision.Feb 12 2019, 6:01 PM

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.

This revision is now accepted and ready to land.Feb 12 2019, 6:01 PM
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).

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

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

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 added a comment.Feb 13 2019, 11:45 AM
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.

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.

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 added a comment.Feb 13 2019, 3:20 PM

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.

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.

In D58157#1397302, @rnk 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!

Thanks for starting the thread, Reid.

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 12:26 PM