[PATCH] D94387: Add new LLVMComponents CMake module.

[serge via Phabricator via llvm-commits]

serge-sans-paille added inline comments.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:726
 function(add_llvm_component_group name)
   cmake_parse_arguments(ARG "HAS_JIT" "" "LINK_COMPONENTS" ${ARGN})
+
----------------
If I understand correctly, you're now defering this to `llvm_component_add_library` ? In that case this should be reflected somewhere, otherwise you're loosing some component information here. 


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:742
+    DEPENDS ${LLVM_COMMON_DEPENDS})
+
+  # TODO: LLVM_LIBS here is actually components.
----------------
I like this change. You'd need to update the documentation to reflect that this is now a normal component


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:1510
   list(APPEND LLVM_LINK_COMPONENTS Support) # gtest needs it for raw_ostream
-  add_llvm_executable(${test_name} IGNORE_EXTERNALIZE_DEBUGINFO NO_INSTALL_RPATH ${ARGN})
+  add_llvm_executable(${test_name}
+    IGNORE_EXTERNALIZE_DEBUGINFO
----------------
I agree with this diff, but given the size of this patch, it's probably better *not* to fix cosmetic stuff ;-) 


================
Comment at: llvm/lib/CMakeLists.txt:59
 
 # Component post-processing
+#LLVMBuildGenerateCFragment(OUTPUT ${LLVMCONFIGLIBRARYDEPENDENCIESINC})
----------------
It's super nice that you manage to remove this post-processing step!
But removing the `LLVMBuildGenerateCFragment` puzzles me... I don't see it being generated elsewhere :-)


================
Comment at: llvm/lib/ExecutionEngine/Orc/CMakeLists.txt:35
   LINK_COMPONENTS
+  Analysis
+  BitReader
----------------
I <3 this kind of change.


================
Comment at: llvm/lib/Support/CMakeLists.txt:259
 
-set_property(TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS "${llvm_system_libs}")
+set_property(TARGET ${_prop_target} PROPERTY LLVM_SYSTEM_LIBS "${llvm_system_libs}")
 
----------------
I don't see where `_prop_target` is set o_O


================
Comment at: llvm/tools/llvm-config/CMakeLists.txt:12
 # Compute the substitution values for various items.
-get_property(SUPPORT_SYSTEM_LIBS TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS)
-get_property(WINDOWSMANIFEST_SYSTEM_LIBS TARGET LLVMWindowsManifest PROPERTY LLVM_SYSTEM_LIBS)
+llvm_component_get_target_property(SUPPORT_SYSTEM_LIBS LLVMSupport LLVM_SYSTEM_LIBS)
+llvm_component_get_target_property(WINDOWSMANIFEST_SYSTEM_LIBS LLVMWindowsManifest LLVM_SYSTEM_LIBS)
----------------
This is much cleaner, thanks!


================
Comment at: llvm/unittests/Target/X86/CMakeLists.txt:14
   Target
-  X86CodeGen
-  X86Desc
-  X86Info
+  TargetX86
   )
----------------
Cool!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94387