[libcxx-commits] [PATCH] D98367: [libcxxabi] Use cxx-headers target to consume libcxx headers

[Petr Hosek via Phabricator via libcxx-commits]

phosek added inline comments.


================
Comment at: libcxxabi/CMakeLists.txt:163
     "Specify path to libc++ includes.")
-message(STATUS "Libc++abi will be using libc++ includes from ${LIBCXXABI_LIBCXX_INCLUDES}")
+if (LIBCXXABI_STANDALONE_BUILD)
+  if (NOT IS_DIRECTORY ${LIBCXXABI_LIBCXX_INCLUDES})
----------------
compnerd wrote:
> Why not make this `if (NOT TARGET cxx-headers)`?  You don't really care so much about the standalone target as much as you do about the `cxx-headers` target existing.
The issue is that depending on the order in which users put projects in `LLVM_ENABLE_PROJECTS` or `LLVM_ENABLE_RUNTIMES`, `cxx-headers` target may not yet exist, see also D97314 for a related discussion.


================
Comment at: libcxxabi/CMakeLists.txt:170-174
+  if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC")
+    target_compile_options(cxx-headers INTERFACE /I "${LIBCXXABI_LIBCXX_INCLUDES}")
+  else()
+    target_compile_options(cxx-headers INTERFACE -I "${LIBCXXABI_LIBCXX_INCLUDES}")
+  endif()
----------------
compnerd wrote:
> There is no need for this complicated special handling, why not just let CMake handle it?
We tried to use that approach in the past but it broke the runtimes build with what appeared to be a CMake bug where it would simply drop the include on the floor on certain platforms like macOS, see D82702 for some of the background discussion.

We could try and revisit this but I'd prefer to do it in a follow up change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98367