[PATCH] D64383: build: use multiple `install` rather than building up a list

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd closed this revision.
compnerd added a comment.

SVN r365562


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D64383



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64383: build: use multiple `install` rather than building up a list

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.



Comment at: src/CMakeLists.txt:440
+  endif()
+  if (LIBCXX_INSTALL_STATIC_LIBRARY)
+install(TARGETS cxx_static

Super nit: you don't have a newline here but do have a newline before the 
LIBCXX_INSTALL_EXPERIMENTAL_LIBRARY conditional. (I don't have a preference for 
newline vs. no newline; the inconsistency is just mildly noticeable).


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D64383



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64383: build: use multiple `install` rather than building up a list

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai accepted this revision.
smeenai added a comment.

Nice!


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D64383



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64383: build: use multiple `install` rather than building up a list

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd marked an inline comment as done.
compnerd added a comment.

@ldionne - that was exactly the motivation for this change - it always takes me 
a couple of reads to figure out what we are trying to do here.




Comment at: src/CMakeLists.txt:441
+  if (LIBCXX_INSTALL_STATIC_LIBRARY)
+install(TARGETS cxx_static
+  ARCHIVE DESTINATION 
${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx

ldionne wrote:
> Just to double-check, it's redundant to say `ARCHIVE` here because CMake 
> knows it's a static archive -- correct?
No, the `ARCHIVE DESTINATION` here indicates to CMake where to install the 
target *if* it is an archive.  This is the location used if the target is a 
static library or if the target is Windows and the target is a shared library 
in which case this is the path where the import library is installed.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D64383



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64383: build: use multiple `install` rather than building up a list

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I like that. Almost anything that helps us abolish global variables and lists 
(here `LIBCXX_INSTALL_TARGETS`) in the CMake scripts is a good thing.




Comment at: src/CMakeLists.txt:441
+  if (LIBCXX_INSTALL_STATIC_LIBRARY)
+install(TARGETS cxx_static
+  ARCHIVE DESTINATION 
${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx

Just to double-check, it's redundant to say `ARCHIVE` here because CMake knows 
it's a static archive -- correct?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D64383



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64383: build: use multiple `install` rather than building up a list

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added reviewers: smeenai, beanz, EricWF.
Herald added subscribers: libcxx-commits, ldionne, mgorny.

Rather than building up a list to iterate over later, just create multiple 
`install` commands based on the configuration.  This makes it easier to see 
what is getting installed and allows for the install handling to be 
centralised.  NFC


Repository:
  rCXX libc++

https://reviews.llvm.org/D64383

Files:
  src/CMakeLists.txt


Index: src/CMakeLists.txt
===
--- src/CMakeLists.txt
+++ src/CMakeLists.txt
@@ -321,9 +321,6 @@
   endif()
 
   list(APPEND LIBCXX_BUILD_TARGETS "cxx_shared")
-  if (LIBCXX_INSTALL_SHARED_LIBRARY)
-list(APPEND LIBCXX_INSTALL_TARGETS "cxx_shared")
-  endif()
   if(WIN32 AND NOT MINGW AND NOT "${CMAKE_HOST_SYSTEM_NAME}" STREQUAL 
"Windows")
 # Since we most likely do not have a mt.exe replacement, disable the
 # manifest bundling.  This allows a normal cmake invocation to pass which
@@ -360,9 +357,6 @@
   endif()
 
   list(APPEND LIBCXX_BUILD_TARGETS "cxx_static")
-  if (LIBCXX_INSTALL_STATIC_LIBRARY)
-list(APPEND LIBCXX_INSTALL_TARGETS "cxx_static")
-  endif()
   # Attempt to merge the libc++.a archive and the ABI library archive into one.
   if (LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY)
 set(MERGE_ARCHIVES_SEARCH_PATHS "")
@@ -437,13 +431,25 @@
 endif()
 
 if (LIBCXX_INSTALL_LIBRARY)
+  if (LIBCXX_INSTALL_SHARED_LIBRARY)
+install(TARGETS cxx_shared
+  ARCHIVE DESTINATION 
${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  LIBRARY DESTINATION 
${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  RUNTIME DESTINATION ${LIBCXX_INSTALL_PREFIX}bin COMPONENT cxx)
+  endif()
+  if (LIBCXX_INSTALL_STATIC_LIBRARY)
+install(TARGETS cxx_static
+  ARCHIVE DESTINATION 
${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  LIBRARY DESTINATION 
${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  RUNTIME DESTINATION ${LIBCXX_INSTALL_PREFIX}bin COMPONENT cxx)
+  endif()
+
   if (LIBCXX_INSTALL_EXPERIMENTAL_LIBRARY)
-set(experimental_lib cxx_experimental)
+install(TARGETS cxx_experimental
+  LIBRARY DESTINATION 
${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  ARCHIVE DESTINATION 
${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  RUNTIME DESTINATION ${LIBCXX_INSTALL_PREFIX}bin COMPONENT cxx)
   endif()
-  install(TARGETS ${LIBCXX_INSTALL_TARGETS} ${experimental_lib}
-LIBRARY DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} 
COMPONENT cxx
-ARCHIVE DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} 
COMPONENT cxx
-)
   # NOTE: This install command must go after the cxx install command otherwise
   # it will not be executed after the library symlinks are installed.
   if (LIBCXX_ENABLE_SHARED AND LIBCXX_ENABLE_ABI_LINKER_SCRIPT)


Index: src/CMakeLists.txt
===
--- src/CMakeLists.txt
+++ src/CMakeLists.txt
@@ -321,9 +321,6 @@
   endif()
 
   list(APPEND LIBCXX_BUILD_TARGETS "cxx_shared")
-  if (LIBCXX_INSTALL_SHARED_LIBRARY)
-list(APPEND LIBCXX_INSTALL_TARGETS "cxx_shared")
-  endif()
   if(WIN32 AND NOT MINGW AND NOT "${CMAKE_HOST_SYSTEM_NAME}" STREQUAL "Windows")
 # Since we most likely do not have a mt.exe replacement, disable the
 # manifest bundling.  This allows a normal cmake invocation to pass which
@@ -360,9 +357,6 @@
   endif()
 
   list(APPEND LIBCXX_BUILD_TARGETS "cxx_static")
-  if (LIBCXX_INSTALL_STATIC_LIBRARY)
-list(APPEND LIBCXX_INSTALL_TARGETS "cxx_static")
-  endif()
   # Attempt to merge the libc++.a archive and the ABI library archive into one.
   if (LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY)
 set(MERGE_ARCHIVES_SEARCH_PATHS "")
@@ -437,13 +431,25 @@
 endif()
 
 if (LIBCXX_INSTALL_LIBRARY)
+  if (LIBCXX_INSTALL_SHARED_LIBRARY)
+install(TARGETS cxx_shared
+  ARCHIVE DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  LIBRARY DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  RUNTIME DESTINATION ${LIBCXX_INSTALL_PREFIX}bin COMPONENT cxx)
+  endif()
+  if (LIBCXX_INSTALL_STATIC_LIBRARY)
+install(TARGETS cxx_static
+  ARCHIVE DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  LIBRARY DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  RUNTIME DESTINATION ${LIBCXX_INSTALL_PREFIX}bin COMPONENT cxx)
+  endif()
+
   if (LIBCXX_INSTALL_EXPERIMENTAL_LIBRARY)
-set(experimental_lib cxx_experimental)
+install(TARGETS cxx_experimental
+  LIBRARY DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} COMPONENT cxx
+  ARCHIVE DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR}