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

[Nathan Lanza via Phabricator via llvm-commits]

lanza added inline comments.


================
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:
> > 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?
The actual implementation that causes this problem is. 


```
enum TargetType
{
  EXECUTABLE,
  STATIC_LIBRARY,
  SHARED_LIBRARY,
  MODULE_LIBRARY,
  OBJECT_LIBRARY,
  UTILITY,
  GLOBAL_TARGET,
  INTERFACE_LIBRARY,
  UNKNOWN_LIBRARY
};
...
    if (target->GetType() >= cmStateEnums::OBJECT_LIBRARY &&
        target->GetType() != cmStateEnums::UNKNOWN_LIBRARY) {
      ::reportError(context, content->GetOriginalExpression(),
                    "Target \"" + name +
                      "\" is not an executable or library.");
      return nullptr;
    }
...
```

So I guess I'll just replicate this check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77106