[PATCH] D26334: Add some facilities to work with a git monorepo (experimental setup)

Mon Nov 7 11:37:59 PST 2016

Hi Justin,

> On Nov 7, 2016, at 11:03 AM, Justin Bogner <mail at justinbogner.com> wrote:
> 
> Mehdi AMINI via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> writes:
>> mehdi_amini created this revision.
>> mehdi_amini added a reviewer: jlebar.
>> mehdi_amini added a subscriber: llvm-commits.
>> Herald added subscribers: modocache, mgorny.
>> 
>> Some changes are made to cmake, especially the addition of a new
>> LLVM_ENABLE_PROJECTS option that makes the build system aware of
>> the monorepo directory structure.
>> 
>> Also a new script is added in llvm/utils/git-svn/. When present in
>> the $PATH, it enables a `git llvm` command. It is providing at this
>> point only the ability to push from the git monorepo: `git llvm push`.
>> It is intended to evolves with more features, for instance I plan on
>> features like `git llvm show r284955` to help working with sequential
>> revision numbers.
> 
> These seem like two unrelated changes to me. I don't see what the
> git-llvm tool has to do with the top-level-in-a-monorepo cmake stuff.
> Maybe this should be split in two?

I can commit separately. It makes the doc update weird though, but I can put it in a third commit for example.

> 
> I'll only comment on the cmake parts here.
> 
>> mehdi_amini added a comment.
>> 
>> Added @beanz and @bogner to make sure I didn't mess up the cmake part.
>> 
>> mehdi_amini updated this revision to Diff 77054.
>> mehdi_amini added a comment.
>> 
>> Address Justin's comments.
>> 
>> 
>> https://reviews.llvm.org/D26334 <https://reviews.llvm.org/D26334>
>> 
>> Files:
>>  libcxxabi/CMakeLists.txt
>>  llvm/CMakeLists.txt
>>  llvm/docs/GettingStarted.rst
>>  llvm/utils/git-svn/git-llvm
>> 
> ...
>> Index: llvm/CMakeLists.txt
>> ===================================================================
>> --- llvm/CMakeLists.txt
>> +++ llvm/CMakeLists.txt
>> @@ -93,6 +93,25 @@
>>   endif()
>> endif()
>> 
>> +# Git Mono-Repo handling: automatically set the LLVM_EXTERNAL_${project}_SOURCE_DIR
> 
> If we go to a real mono-repo, I think this would make way more sense as
> a top-level cmake rather than inside LLVM. That would also let us
> provide a helpful error message if nothing is set at all.

Sure, but right now we’re *not* going with a real mono-repo.

> 
> As such, I'm not 100% convinced this should go in here and now, since
> people may start relying on it and it would become harder to change. Is
> there a way to put things in the top level of the mono-repo prototype
> instead?

I believe doing it this way is providing a smoother path of upgrade for the future. The staging I foresee right now is, *if* we ever move to a monorepo, the following:

- improve the current cmake infrastructure (like this patch is starting to do).
- sort out everything needed to make to move
- decide on the move
- make the move happen
- work on upgrading the build system (like a top-level CMakeList.txt like you mentioned).

>> +set(LLVM_ALL_PROJECTS "clang;libcxx;libcxxabi;lldb;compiler-rt;lld;polly")
>> +set(LLVM_ENABLE_PROJECTS "" CACHE STRING
>> +	"Semicolon-separated list of projects to build (${LLVM_ALL_PROJECTS}), or \"all\".")
>> +if( LLVM_ENABLE_PROJECTS STREQUAL "all" )
>> +  set( LLVM_ENABLE_PROJECTS ${LLVM_ALL_PROJECTS})
>> +endif()
>> +foreach(proj ${LLVM_ENABLE_PROJECTS})
>> +  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()
>> +  string(TOUPPER "${proj}" upper_proj)
>> +  set(LLVM_EXTERNAL_${upper_proj}_SOURCE_DIR   "${CMAKE_CURRENT_SOURCE_DIR}/../${proj}")
>> +  if (proj STREQUAL "clang")
> 
> Shouldn't this be clang-tools-extra? This looks like it enables
> clang-tools-extra whenever clang is enabled, which is surprising.

My impression is that majority of people involved with clang/clang-tools/extra would like it to be nested in the monorepo.
That’s mimicking this behavior by making clang-tools-extra tied to enabling clang.

>> +    set(LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../clang-tools-extra")
>> +  endif()
>> +endforeach()
>> +
>> # The following only works with the Ninja generator in CMake >= 3.0.
>> set(LLVM_PARALLEL_COMPILE_JOBS "" CACHE STRING
>>   "Define the maximum number of concurrent compilation jobs.")
>> Index: libcxxabi/CMakeLists.txt
>> ===================================================================
>> --- libcxxabi/CMakeLists.txt
>> +++ libcxxabi/CMakeLists.txt
>> @@ -144,6 +144,7 @@
>>         ${LIBCXXABI_LIBCXX_PATH}/include
>>         ${CMAKE_BINARY_DIR}/${LIBCXXABI_LIBCXX_INCLUDES}
>>         ${LLVM_MAIN_SRC_DIR}/projects/libcxx/include
>> +        ${LLVM_MAIN_SRC_DIR}/../libcxx/include
> 
> Is it possible to only add the "correct" set of headers here? If headers
> exist in both places for some reason I think weird things could happen.

What is the “correct” set? Any suggestion on what to write here?
It seems that the “weird things” you refer to could happen already if the headers exists in the already listed multiple paths.

> 
>>         ${LLVM_INCLUDE_DIR}/c++/v1
>>   )
>> 
>> @@ -156,6 +157,7 @@
>>   PATHS ${LIBCXXABI_LIBCXX_PATH}
>>         ${LIBCXXABI_LIBCXX_INCLUDES}/../
>>         ${LLVM_MAIN_SRC_DIR}/projects/libcxx/
>> +        ${LLVM_MAIN_SRC_DIR}/../libcxx/
>>   NO_DEFAULT_PATH
>>   )

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161107/4348add3/attachment.html>