[CMake] Copy C++ headers before configuring runtimes build

[beanz]

Summary

On some platforms C++ headers are packaged with the compiler not the sysroot. If you don't copy C++ headers into the build include directory during configuraiton of the outer build the C++ check during the runtime configuration may get inaccurate results.

[smeenai]

llvm/runtimes/CMakeLists.txt

9 ↗

(On Diff #200336)

Super nit: I always either leave off the ${} or enclose it in quotes inside if to avoid potentially running afoul of CMake's weird expansion rules for if. It's probably unnecessary most of the time, bit it's easy enough to do.

43 ↗

(On Diff #200336)

Typo: plae

46 ↗

(On Diff #200336)

Any reason to prefer this to either cmake -E copy_directory or file(COPY)?

[phosek]

llvm/runtimes/CMakeLists.txt

44 ↗

(On Diff #200336)

This is true for many other platforms, e.g. in our toolchain build it's the case as well. Can we do this unconditionally?

46 ↗

(On Diff #200336)

Even better, could we factor out the logic for copying headers in https://github.com/llvm/llvm-project/blob/master/libcxx/include/CMakeLists.txt#L218 to a separate .cmake module (except for the generated __config part that can be only done after we've finished configuration) and then reuse it here?

[beanz]

llvm/runtimes/CMakeLists.txt

46 ↗

(On Diff #200336)

I was using ditto because the code was Darwin-only and ditto has been on Darwin for every version of Darwin we support LLVM with, and ditto is way faster than cmake -E ...

If we want this generalized then it should be done with cmake -E ... I'll take a stab at that shortly.

[beanz]

Updates based on review feedback.

[compnerd]

llvm/runtimes/CMakeLists.txt

213 ↗

(On Diff #200345)

Can you add a TODO to migrate to file(COPY when we switch to CMake >= 3.7? That avoids the custom function.

[beanz]

I just came up with a different and slightly crazier approach to this. I'm going to test it out and will upload a new version of this patch shortly.

[beanz]

A new take on how to do this, even more fun this time!

[beanz]

Looping in @EricWF.

[compnerd]

The only thing that Im worried about is cross-compilation accidentally picking up the libc++ headers that are built here (e.g. building on Linux for Linux with the host libc++ headers and building libc++ as a runtime). The rest looks pretty much a substitution and addition of runtime-libcxx-headers target to the runtimes build which is fine.

[EricWF]

I dont see any problems with this change. But I'm not sure I have any interesting insight either.

[smeenai]

libcxx/include/CMakeLists.txt

215 ↗

(On Diff #200372)

Typo: should be configuration

[beanz]

Thank goodness @smeenai can spell because obviously I can't.

[compnerd]

I think that this makes sense - the one case that I had in mind is pretty convoluted and if it breaks, we can fix it then.

[lebedev.ri]

Hello. I'm reasonably sure this broke build for me:
LLVM_ENABLE_PROJECTS=clang;clang-tools-extra;compiler-rt;libcxx;libcxxabi;lld;openmp
BUILD_SHARED_LIBS=ON

CMake Error at /build/libcxx/include/CMakeLists.txt:278 (add_custom_target):
  add_custom_target cannot create target "install-libcxx-headers" because
  another target with the same name already exists.  The existing target is a
  custom target created in source directory "/build/libcxx/include".  See
  documentation for policy CMP0002 for more details.


CMake Error at /build/libcxx/include/CMakeLists.txt:279 (add_custom_target):
  add_custom_target cannot create target "install-libcxx-headers-stripped"
  because another target with the same name already exists.  The existing
  target is a custom target created in source directory
  "/build/libcxx/include".  See documentation for policy CMP0002 for more
  details.

Is this expected? Do i need to purge the build dir? If not, can this please be reverted in the mean time?

libcxx/trunk/include/CMakeLists.txt

if (LIBCXX_NEEDS_SITE_CONFIG)
   set(generated_config_deps cxx-generated-config)
 else()
   set(files
     ${files}
     __config
     )
 endif()
 
+# In some build configuraitons (like boostrapping clang), we need to be able to
+# install the libcxx headers before CMake configuraiton for libcxx runs. Making
+# the name of this target configurable allows LLVM/runtimes/CMakeLists.txt to
+# add this subdirectory to the LLVM build to put libcxx's headers in place
+# before libcxx's build configuration is run.
+if (NOT CXX_HEADER_TARGET)
+  set(CXX_HEADER_TARGET cxx-headers)
+endif()
 if(NOT LIBCXX_USING_INSTALLED_LLVM AND LIBCXX_HEADER_DIR)
   set(output_dir ${LIBCXX_HEADER_DIR}/include/c++/v1)
 
   set(out_files)
   foreach(f ${files})
     set(src ${CMAKE_CURRENT_SOURCE_DIR}/${f})
     set(dst ${output_dir}/${f})
     add_custom_command(OUTPUT ${dst}
if (LIBCXX_NEEDS_SITE_CONFIG)
     set(dst ${output_dir}/__config)
     add_custom_command(OUTPUT ${dst}
         DEPENDS ${src} ${generated_config_deps}
         COMMAND ${CMAKE_COMMAND} -E copy_if_different ${src} ${dst}
         COMMENT "Copying CXX __config")
     list(APPEND out_files ${dst})
   endif()
 
-  add_custom_target(cxx-headers ALL DEPENDS ${out_files} ${LIBCXX_CXX_ABI_HEADER_TARGET})
+  add_custom_target(${CXX_HEADER_TARGET} ALL DEPENDS ${out_files} ${LIBCXX_CXX_ABI_HEADER_TARGET})
 else()
-  add_custom_target(cxx-headers)
+  add_custom_target(${CXX_HEADER_TARGET})
 endif()
-set_target_properties(cxx-headers PROPERTIES FOLDER "Misc")
+set_target_properties(${CXX_HEADER_TARGET} PROPERTIES FOLDER "Misc")
 
 if (LIBCXX_INSTALL_HEADERS)
   foreach(file ${files})
     get_filename_component(dir ${file} DIRECTORY)
     install(FILES ${file}
       DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}include/c++/v1/${dir}
-      COMPONENT cxx-headers
+      COMPONENT ${CXX_HEADER_TARGET}
       PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
     )
   endforeach()
 
   if (LIBCXX_NEEDS_SITE_CONFIG)
     # Install the generated header as __config.
     install(FILES ${LIBCXX_BINARY_DIR}/__generated_config
       DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}include/c++/v1
       PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
       RENAME __config
-      COMPONENT cxx-headers)
+      COMPONENT ${CXX_HEADER_TARGET})
   endif()
 
   if (NOT CMAKE_CONFIGURATION_TYPES)
-    add_custom_target(install-cxx-headers
-                      DEPENDS cxx-headers ${generated_config_deps}
+    add_custom_target(install-${CXX_HEADER_TARGET}
+                      DEPENDS ${CXX_HEADER_TARGET} ${generated_config_deps}
                       COMMAND "${CMAKE_COMMAND}"
-                              -DCMAKE_INSTALL_COMPONENT=cxx-headers
+                              -DCMAKE_INSTALL_COMPONENT=${CXX_HEADER_TARGET}
                               -P "${CMAKE_BINARY_DIR}/cmake_install.cmake")
     # Stripping is a no-op for headers
-    add_custom_target(install-cxx-headers-stripped DEPENDS install-cxx-headers)
+    add_custom_target(install-${CXX_HEADER_TARGET}-stripped DEPENDS install-${CXX_HEADER_TARGET})
 
-    add_custom_target(install-libcxx-headers DEPENDS install-cxx-headers)
-    add_custom_target(install-libcxx-headers-stripped DEPENDS install-cxx-headers-stripped)
+    add_custom_target(install-libcxx-headers DEPENDS install-${CXX_HEADER_TARGET})
+    add_custom_target(install-libcxx-headers-stripped DEPENDS install-${CXX_HEADER_TARGET}-stripped)
   endif()
 endif()

llvm/trunk/runtimes/CMakeLists.txt

 # This file handles building LLVM runtime sub-projects.
 
 # Runtimes are different from tools or other drop-in projects because runtimes
 # should be built with the LLVM toolchain from the build directory. This file is
 # a first step to formalizing runtime build interfaces.
 
-# In the current state this file only works with compiler-rt, other runtimes
-# will work as the runtime build interface standardizes.
+# Setting CMake minimum required version should be at the very top of the file
+# if this is the entry point.
+if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
+  cmake_minimum_required(VERSION 3.4.3)
+  project(Runtimes C CXX ASM)
+endif()
 
 # Find all subdirectories containing CMake projects
 file(GLOB entries *)
 foreach(entry ${entries})
   if(IS_DIRECTORY ${entry} AND EXISTS ${entry}/CMakeLists.txt)
     list(APPEND runtimes ${entry})
   endif()
 endforeach()
else()
       configure_file(
         ${CMAKE_CURRENT_SOURCE_DIR}/Components.cmake.in
         ${LLVM_BINARY_DIR}/runtimes/Components.cmake)
     endif()
   endif()
 
 else() # if this is included from LLVM's CMake
   include(LLVMExternalProjectUtils)
+  if (LLVM_EXTERNAL_LIBCXX_SOURCE_DIR)
+    set(LIBCXX_HEADER_DIR ${LLVM_INCLUDE_DIR}/c++/v1/)
+    set(CXX_HEADER_TARGET runtime-libcxx-headers)
+    add_subdirectory(${LLVM_EXTERNAL_LIBCXX_SOURCE_DIR}/include ${CXX_HEADER_TARGET})
+  endif()
 
   if(NOT LLVM_BUILD_RUNTIMES)
     set(EXTRA_ARGS EXCLUDE_FROM_ALL)
   endif()
 
   function(builtin_default_target compiler_rt_path)
     cmake_parse_arguments(ARG "" "" "DEPENDS" ${ARGN})
 
function(runtime_default_target)
     endforeach()
 
     if(LLVM_INCLUDE_TESTS)
       list(APPEND test_targets runtimes-test-depends check-runtimes)
     endif()
 
     llvm_ExternalProject_Add(runtimes
                              ${CMAKE_CURRENT_SOURCE_DIR}
-                             DEPENDS ${ARG_DEPENDS}
+                             DEPENDS ${ARG_DEPENDS} runtime-libcxx-headers
                              # Builtins were built separately above
                              CMAKE_ARGS -DCOMPILER_RT_BUILD_BUILTINS=Off
                                         -DLLVM_INCLUDE_TESTS=${LLVM_INCLUDE_TESTS}
                                         -DLLVM_BINARY_DIR=${LLVM_BINARY_DIR}
                                         -DLLVM_LIBRARY_DIR=${LLVM_LIBRARY_DIR}
                                         -DLLVM_DEFAULT_TARGET_TRIPLE=${TARGET_TRIPLE}
                                         -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON
                                         -DCMAKE_C_COMPILER_TARGET=${TARGET_TRIPLE}
function(runtime_register_target name target)
     endforeach()
 
     if(target IN_LIST LLVM_RUNTIME_BUILD_ID_LINK_TARGETS)
       list(APPEND EXTRA_ARGS STRIP_TOOL ${CMAKE_CURRENT_BINARY_DIR}/llvm-strip-link)
     endif()
 
     llvm_ExternalProject_Add(runtimes-${name}
                              ${CMAKE_CURRENT_SOURCE_DIR}
-                             DEPENDS ${${name}_deps}
+                             DEPENDS ${${name}_deps} runtime-libcxx-headers
                              # Builtins were built separately above
                              CMAKE_ARGS -DCOMPILER_RT_BUILD_BUILTINS=Off
                                         -DLLVM_INCLUDE_TESTS=${LLVM_INCLUDE_TESTS}
                                         -DLLVM_BINARY_DIR=${LLVM_BINARY_DIR}
                                         -DLLVM_LIBRARY_DIR=${LLVM_LIBRARY_DIR}
                                         -DLLVM_DEFAULT_TARGET_TRIPLE=${target}
                                         -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON
                                         -DCMAKE_C_COMPILER_TARGET=${target}