[PATCH] D77106: [cmake] Only set deps for an ExternalProject if the type is executable or library

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added reviewers: phosek, compnerd.
smeenai requested changes to this revision.
smeenai added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/cmake/modules/LLVMExternalProjectUtils.cmake:68
+      get_target_property(target_type "${tool}" TYPE)
+      if (target_type STREQUAL "EXECUTABLE" OR target_type SREQUAL "LIBRARY")
+        list(APPEND TOOLCHAIN_BINS $<TARGET_FILE:${tool}>)
----------------
Should be STREQUAL instead of SREQUAL on the second one.


================
Comment at: llvm/cmake/modules/LLVMExternalProjectUtils.cmake:68
+      get_target_property(target_type "${tool}" TYPE)
+      if (target_type STREQUAL "EXECUTABLE" OR target_type SREQUAL "LIBRARY")
+        list(APPEND TOOLCHAIN_BINS $<TARGET_FILE:${tool}>)
----------------
smeenai wrote:
> Should be STREQUAL instead of SREQUAL on the second one.
Nit: it's more common in LLVM to have no space after the if, i.e. `if(` instead of `if (`


================
Comment at: llvm/cmake/modules/LLVMExternalProjectUtils.cmake:68
+      get_target_property(target_type "${tool}" TYPE)
+      if (target_type STREQUAL "EXECUTABLE" OR target_type SREQUAL "LIBRARY")
+        list(APPEND TOOLCHAIN_BINS $<TARGET_FILE:${tool}>)
----------------
smeenai wrote:
> smeenai wrote:
> > Should be STREQUAL instead of SREQUAL on the second one.
> Nit: it's more common in LLVM to have no space after the if, i.e. `if(` instead of `if (`
https://cmake.org/cmake/help/latest/prop_tgt/TYPE.html says the library types are "SHARED_LIBRARY", "STATIC_LIBRARY", etc. Does just "LIBRARY" work?

To be fair, I don't know if we even want libraries in this list. But if you wanna fix the CMake issue while having the least chance of changing existing behavior, a blacklist might be easier than a whitelist?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77106