[Lldb-commits] [PATCH] D132608: [CMake] Clean up CMake binary dir handling

[Louis Dionne via Phabricator via lldb-commits]

ldionne added a comment.

I think I like the spirit of this change, which seems to be to move us closer to `CMAKE_foo` variables and further from `LLVM_foo` variables for equivalent functionality. I have a comment, but this essentially LGTM. The libc++ CI failures (in particular the bootstrapping build failure) seems to be real, though, so it should be understood before landing the patch.



================
Comment at: libcxx/CMakeLists.txt:421
+if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
+  set(default_install_path "${CMAKE_INSTALL_INCLUDEDIR}/${LLVM_DEFAULT_TARGET_TRIPLE}/c++/v1")
+else()
----------------
Instead, I'd do this:

```
if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
  set(_include_target_dir "${CMAKE_INSTALL_INCLUDEDIR}/${LLVM_DEFAULT_TARGET_TRIPLE}/c++/v1")
  set(_install_library_dir "lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE}")
else()
  set(_include_target_dir "${LIBCXX_INSTALL_INCLUDE_DIR}")
  set(_install_library_dir "lib${LLVM_LIBDIR_SUFFIX}")
endif()

set(LIBCXX_INSTALL_INCLUDE_TARGET_DIR "${_include_target_dir}" CACHE PATH
    "Path where target-specific libc++ headers should be installed.")
set(LIBCXX_INSTALL_LIBRARY_DIR "${_install_library_dir}" CACHE PATH
    "Path where built libc++ libraries should be installed.")
```

IMO that's easier on the eye.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132608