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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/lib/CMakeLists.txt:177
+    if (APPLE)
+      target_link_options(cxx_shared PRIVATE "-Wl,-force_load" "$<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>")
+    else()
----------------
smeenai wrote:
> `target_link_options` was added in CMake 3.13, so you're gonna have to use `target_link_libraries` to specify linker options (which is unfortunate, but our minimum CMake version is 3.4.3).
> 
> How come you're using a generator expression here whereas the previous code was just using LIBCXX_CXX_STATIC_ABI_LIBRARY directly? The need for that might go away with the switch to `target_link_libraries`.
> `target_link_options` was added in CMake 3.13, so you're gonna have to use `target_link_libraries` to specify linker options (which is unfortunate, but our minimum CMake version is 3.4.3).

Will do. Out of curiosity, what/who decides what's the minimal version of CMake we have to support? CMake being one of the easiest tools to update on any system, I'd argue we should have a more aggressive stance towards requesting a recent version.


> How come you're using a generator expression here whereas the previous code was just using `LIBCXX_CXX_STATIC_ABI_LIBRARY` directly? The need for that might go away with the switch to `target_link_libraries`.

It didn't work anymore with `target_link_options`, probably because it doesn't expand target names into their `TARGET_LINKER_FILE`. Changing to `target_link_libraries` removes the need for it, but I'd like to keep it because it's more precise (it doesn't rely on CMake magic under the hood).

Changing to `target_link_libraries` also removes the need for the `add_dependencies` call below, so I'll remove that one.


================
Comment at: libcxx/lib/CMakeLists.txt:185
+  else()
+    target_link_libraries(cxx_shared PRIVATE "${LIBCXX_CXX_SHARED_ABI_LIBRARY}")
+  endif()
----------------
smeenai wrote:
> We were previously using `add_interface_library` for this, which would populate the list of libraries used to form the libc++ linker script. I don't see a replacement for that logic here?
When I wrote my patch I originally added it explicitly to `LIBCXX_INTERFACE_LIBRARY_NAMES` as:

```
list(APPEND LIBCXX_INTERFACE_LIBRARIES "${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
```

but eventually I decided it wasn't necessary. I can't remember why now -- maybe I was wrong. I'll put it back.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60276