[libcxx-commits] [PATCH] D60276: [libc++] Localize CMake code only related to the shared library

Fri Apr 5 10:23:39 PDT 2019

ldionne added inline comments.

================
Comment at: libcxx/lib/CMakeLists.txt:183
+    # to build the static libc++abi before libc++, so we must tell it explicitly.
+    add_dependencies(cxx_shared "${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
+  else()
----------------
smeenai wrote:
> smeenai wrote:
> > Sorry, I just realized a potential issue here. At least with the Ninja generator, this adds an order-only dependency from cxx_shared to LIBCXX_CXX_STATIC_ABI_LIBRARY, not a full dependency. In other words, if you change a source file for LIBCXX_CXX_STATIC_ABI_LIBRARY and then rebuild cxx_shared, LIBCXX_CXX_STATIC_ABI_LIBRARY will get rebuilt, but cxx_shared won't be relinked. I'm not sure if there's any way to get the full dependency other than just using target_link_libraries directly :(
> Well, there's `LINK_DEPENDS`, but it's only available for Makefile and Ninja generators. It looks like at least the Xcode generator doesn't have the order-only dependency issue though.
Okay, I'm using the original `target_link_libraries` line where everything is split. We shouldn't have issues with de-duplication of linker flags for `libc++abi.dylib`.

================
Comment at: libcxx/lib/CMakeLists.txt:321
 # after cxx builds.
 if (LIBCXX_ENABLE_SHARED AND LIBCXX_ENABLE_ABI_LINKER_SCRIPT)
   # Get the name of the ABI library and handle the case where CXXABI_LIBNAME
----------------
phosek wrote:
> Have you considered moving this block into the `if(LIBCXX_ENABLE_SHARED)` above as well? AFAIK this is the only use of `LIBCXX_INTERFACE_LIBRARIES` so we might be able to eliminate that variable altogether.
I moved the block, but I'll see if it's possible to eliminate `LIBCXX_INTERFACE_LIBRARIES` separately.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60276