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

[Shoaib Meenai via Phabricator via libcxx-commits]

smeenai added a subscriber: beanz.
smeenai 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()
----------------
ldionne wrote:
> 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.
I'd agree on being more aggressive with CMake upgrades. CC @beanz

One problem with the generator expression is that it assumes `${LIBCXX_CXX_STATIC_ABI_LIBRARY}` is a target, whereas it could in theory be the libc++abi system library. I don't know if anyone actually cares about the use case of statically linking the system libc++abi (assuming your system even has a static libc++abi) into libc++ though.

Since you're using the generator expression, could you switch to

```
      target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load,$<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>")
```

That'll prevent CMake's library reordering from misplacing the argument to the force_load.

(Alternatively, if you find combining `-Wl` arguments too ugly, the following should be fine too:

```
      target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load $<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>")
```


================
Comment at: libcxx/lib/CMakeLists.txt:179
+    else()
+      target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive" "-Wl,-Bstatic" "$<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>" "-Wl,-Bdynamic" "-Wl,--no-whole-archive")
+    endif()
----------------
Same comment here:

```
      target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive,-Bstatic,$<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}>,-Bdynamic,--no-whole-archive")
```

(or separated out)

```
      target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive -Wl,-Bstatic $<TARGET_LINKER_FILE:${LIBCXX_CXX_STATIC_ABI_LIBRARY}> -Wl,-Bdynamic -Wl,--no-whole-archive")
```


================
Comment at: libcxx/lib/CMakeLists.txt:184
+  endif()
+  list(APPEND LIBCXX_INTERFACE_LIBRARIES "${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
+
----------------
I believe this should be part of the `else()` above.


================
Comment at: libcxx/lib/CMakeLists.txt:200
+    target_link_libraries(cxx_shared PRIVATE
+      "-compatibility_version" "1"
+      "-install_name" "/usr/lib/libc++.1.dylib"
----------------
I'd combine these two into a single argument


================
Comment at: libcxx/lib/CMakeLists.txt:201
+      "-compatibility_version" "1"
+      "-install_name" "/usr/lib/libc++.1.dylib"
+      "-Wl,-unexported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/libc++unexp.exp"
----------------
Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60276