[PATCH] D13399: [CMake] Bug 14109 - CMake build for compiler-rt should use just-built clang

[Chris Bieneman via cfe-commits]

beanz added a comment.

I will send updated patches shortly.

Comments inline below.


================
Comment at: runtime/CMakeLists.txt:33
@@ -32,1 +32,3 @@
+      )
+    set(cmake_3_4_USES_TERMINAL USES_TERMINAL 1)
   endif()
----------------
samsonov wrote:
> Should this also be named cmake_3_4_USES_TERMINAL_OPTIONS?
This was used in an earlier iteration to pass USES_TERMINAL into a call to `ExternalProject_Add_Step` I will remove it.

================
Comment at: runtime/CMakeLists.txt:40
@@ +39,3 @@
+  set(STAMP_DIR ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-stamps/)
+  set(BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-bins/)
+
----------------
samsonov wrote:
> This one is later re-defined from ExternalProject_Get_Property. Does it provide the same value there?
Yep, it can be removed.

================
Comment at: runtime/CMakeLists.txt:42
@@ +41,3 @@
+
+  add_custom_target(compiler-rt-clear
+    DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-cleared
----------------
samsonov wrote:
> Where do you use this target?
That is a convenience target for clearing out the compiler-rt directory. It isn't strictly needed, but I have found it (and the bootstrap-clear) to be useful.

Particularly as I've been changing which files are generated by the compiler-rt build and where they end up it is nice to be able to nuke the build directory.

================
Comment at: runtime/CMakeLists.txt:47
@@ +46,3 @@
+    OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-cleared
+    DEPENDS clang llvm-config
+    COMMAND ${CMAKE_COMMAND} -E remove_directory ${BINARY_DIR}
----------------
samsonov wrote:
> Why do you need these deps?
Specifying those deps makes it so that if llvm-config or clang change compiler-rt gets cleaned out and rebuilt.

================
Comment at: runtime/CMakeLists.txt:71
@@ -49,2 +70,3 @@
+               -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}
     INSTALL_COMMAND ""
     STEP_TARGETS configure build
----------------
samsonov wrote:
> Why did you remove -DCOMPILER_RT_ENABLE_WERROR=ON? I think it's fine to keep it enabled, as it's ok to ensure that ToT Clang builds ToT compiler-rt with no warnings.
I'm trying to set the minimal set of options required to make compiler-rt build. Any additional options can be passed in via `CLANG_COMPILER_RT_CMAKE_ARGS`.

I think that if we expect `COMPILER_RT_ENABLE_WERROR` to be the way everyone is building, we should make that the default in compiler-rt, not pass it in here.


http://reviews.llvm.org/D13399