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

[Chris Bieneman via cfe-commits]

beanz added inline comments.

================
Comment at: runtime/CMakeLists.txt:41
@@ +40,3 @@
+
+  add_custom_target(compiler-rt-clear
+    DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-cleared
----------------
samsonov wrote:
> So, that's the target that you can invoke manually to clean compiler-rt build?
Yes.

================
Comment at: runtime/CMakeLists.txt:46
@@ +45,3 @@
+    OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/compiler-rt-cleared
+    DEPENDS clang llvm-config
+    COMMAND ${CMAKE_COMMAND} -E remove_directory ${BINARY_DIR}
----------------
samsonov wrote:
> Hm? But "compiler-rt" target also depends on clang and llvm-config.
It should. We need it to rebuild whenever clang and llvm-config change.

Although it just occurred to me that I also need to make sure that the clearing happens before compiler-rt gets built, so there will need to be a dependency on that too... I'll figure that out.

================
Comment at: runtime/CMakeLists.txt:69
@@ -47,3 +68,3 @@
                -DCOMPILER_RT_INCLUDE_TESTS=${LLVM_INCLUDE_TESTS}
-               -DCOMPILER_RT_ENABLE_WERROR=ON
+               -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}
     INSTALL_COMMAND ""
----------------
samsonov wrote:
> I thought we only refer to COMPILER_RT_INSTALL_PATH in compiler-rt's CMake. Do we actually need CMAKE_INSTALL_PREFIX there as well?
I don't think we're actually using it in compiler-rt today, but I think that passing it through is a good idea so that if in the future compiler-rt ever uses CMAKE_INSTALL_PREFIX it will be properly populated.

================
Comment at: runtime/CMakeLists.txt:70
@@ -49,2 +69,3 @@
+               -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}
     INSTALL_COMMAND ""
     STEP_TARGETS configure build
----------------
samsonov wrote:
> Disagree - we can't set COMPILER_RT_ENABLE_WERROR in compiler-rt's CMake, because standalone compiler-rt can be built with any compiler in the world, which we don't control. OTOH, if we *know* we're building it with trunk Clang, having it -Werror-clean is smth. we can enforce. It's also consistent with what autoconf did previously.
At the very least if we do that it should be tied to LLVM_ENABLE_WERROR, not just defaulted to On. None of our projects default WERROR to On anywhere. While I agree building with Werror is desirable, we shouldn't be defaulting it on, and we certainly shouldn't be doing it in a way that makes it so the user has to edit the build files in order to turn it off.

================
Comment at: runtime/CMakeLists.txt:78
@@ -62,11 +77,3 @@
 
-  ExternalProject_Add_Step(compiler-rt clobber
-    COMMAND ${CMAKE_COMMAND} -E remove_directory <BINARY_DIR>
-    COMMAND ${CMAKE_COMMAND} -E make_directory <BINARY_DIR>
-    COMMENT "Clobberring compiler-rt build directory..."
-    DEPENDERS configure
-    DEPENDS ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang
-  )
-
-  ExternalProject_Get_Property(compiler-rt BINARY_DIR)
-  set(COMPILER_RT_BINARY_DIR ${BINARY_DIR})
+  add_custom_target(install-compiler-rt
+                    DEPENDS compiler-rt
----------------
samsonov wrote:
> This is also convenience target, right?
Yes.


http://reviews.llvm.org/D13399