[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354057: Stop enabling clang-tools-extra automatically when 
clang is in… (authored by nico, committed by ).
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D58157?vs=186559=186895#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58157

Files:
  llvm/trunk/CMakeLists.txt
  llvm/trunk/docs/GettingStarted.rst


Index: llvm/trunk/CMakeLists.txt
===
--- llvm/trunk/CMakeLists.txt
+++ llvm/trunk/CMakeLists.txt
@@ -104,7 +104,7 @@
 # LLVM_EXTERNAL_${project}_SOURCE_DIR using LLVM_ALL_PROJECTS
 # This allows an easy way of setting up a build directory for llvm and another
 # one for llvm+clang+... using the same sources.
-set(LLVM_ALL_PROJECTS 
"clang;compiler-rt;debuginfo-tests;libclc;libcxx;libcxxabi;libunwind;lld;lldb;llgo;llvm;openmp;parallel-libs;polly;pstl")
+set(LLVM_ALL_PROJECTS 
"clang;clang-tools-extra;compiler-rt;debuginfo-tests;libclc;libcxx;libcxxabi;libunwind;lld;lldb;llgo;llvm;openmp;parallel-libs;polly;pstl")
 set(LLVM_ENABLE_PROJECTS "" CACHE STRING
"Semicolon-separated list of projects to build (${LLVM_ALL_PROJECTS}), 
or \"all\".")
 if( LLVM_ENABLE_PROJECTS STREQUAL "all" )
@@ -141,12 +141,6 @@
 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)
Index: llvm/trunk/docs/GettingStarted.rst
===
--- llvm/trunk/docs/GettingStarted.rst
+++ llvm/trunk/docs/GettingStarted.rst
@@ -64,8 +64,8 @@
 
  * ``-DLLVM_ENABLE_PROJECTS='...'`` --- semicolon-separated list of the 
LLVM
subprojects you'd like to additionally build. Can include any of: clang,
-   libcxx, libcxxabi, libunwind, lldb, compiler-rt, lld, polly, or
-   debuginfo-tests.
+   clang-tools-extra, libcxx, libcxxabi, libunwind, lldb, compiler-rt, lld,
+   polly, or debuginfo-tests.
 
For example, to build LLVM, Clang, libcxx, and libcxxabi, use
``-DLLVM_ENABLE_PROJECTS="clang;libcxx;libcxxabi"``.


Index: llvm/trunk/CMakeLists.txt
===
--- llvm/trunk/CMakeLists.txt
+++ llvm/trunk/CMakeLists.txt
@@ -104,7 +104,7 @@
 # LLVM_EXTERNAL_${project}_SOURCE_DIR using LLVM_ALL_PROJECTS
 # This allows an easy way of setting up a build directory for llvm and another
 # one for llvm+clang+... using the same sources.
-set(LLVM_ALL_PROJECTS "clang;compiler-rt;debuginfo-tests;libclc;libcxx;libcxxabi;libunwind;lld;lldb;llgo;llvm;openmp;parallel-libs;polly;pstl")
+set(LLVM_ALL_PROJECTS "clang;clang-tools-extra;compiler-rt;debuginfo-tests;libclc;libcxx;libcxxabi;libunwind;lld;lldb;llgo;llvm;openmp;parallel-libs;polly;pstl")
 set(LLVM_ENABLE_PROJECTS "" CACHE STRING
 	"Semicolon-separated list of projects to build (${LLVM_ALL_PROJECTS}), or \"all\".")
 if( LLVM_ENABLE_PROJECTS STREQUAL "all" )
@@ -141,12 +141,6 @@
 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)
Index: llvm/trunk/docs/GettingStarted.rst
===
--- llvm/trunk/docs/GettingStarted.rst
+++ llvm/trunk/docs/GettingStarted.rst
@@ -64,8 +64,8 @@
 
  * ``-DLLVM_ENABLE_PROJECTS='...'`` --- semicolon-separated list of the LLVM
subprojects you'd like to additionally build. Can include any of: clang,
-   libcxx, libcxxabi, libunwind, lldb, compiler-rt, lld, polly, or
-   debuginfo-tests.
+   clang-tools-extra, libcxx, libcxxabi, libunwind, lldb, compiler-rt, lld,
+   polly, or debuginfo-tests.
 
For example, to build LLVM, Clang, libcxx, and libcxxabi, use

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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.


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

https://reviews.llvm.org/D58157



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

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

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!


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

https://reviews.llvm.org/D58157



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

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

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.


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

https://reviews.llvm.org/D58157



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

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


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

https://reviews.llvm.org/D58157



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

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

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)


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

https://reviews.llvm.org/D58157



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

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

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.


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

https://reviews.llvm.org/D58157



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

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


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

https://reviews.llvm.org/D58157



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

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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.


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

https://reviews.llvm.org/D58157



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

[PATCH] D58157: Stop enabling clang-tools-extra automatically when clang is in LLVM_ENABLE_PROJECTS

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added subscribers: cfe-commits, lebedev.ri.
lebedev.ri added a comment.

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


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

https://reviews.llvm.org/D58157



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