[CMake] Use llvm-ar etal for external project build on Darwin

[phosek]

Summary

This is needed for the runtimes build since libc++ and libc++abi
archive merging relies on ar to extract files out of the archive,
even on Darwin.

[smeenai]

I've never really understood this block, since the APPLE and WIN32/MSVC conditionals here will reflect the host you're building on and not your actual runtimes target, but this change itself looks fine.

LGTM barring the question about the WIN32 -> MSVC change.

llvm/cmake/modules/LLVMExternalProjectUtils.cmake

50

What's the reason for this part of the change?

[phosek]

In D74107#1862126, @smeenai wrote:

I've never really understood this block, since the APPLE and WIN32/MSVC conditionals here will reflect the host you're building on and not your actual runtimes target, but this change itself looks fine.

They're affected by CMAKE_SYSTEM_NAME which is usually set for the runtimes build so it's not necessarily the host. I'm still not sure whether this condition is desirable though since most of these tools either support APPLE or WIN32 platforms, or CMake will just ignore them and use the appropriate tool for the given platform. This code predates runtimes build and my changes so I don't know what the motivation behind that condition was.

LGTM barring the question about the WIN32 -> MSVC change.

llvm/cmake/modules/LLVMExternalProjectUtils.cmake

50

AFAIK these tools should be still applicable to mingw so this is more precise, but I'm fine keeping the condition as is.

[smeenai]

In D74107#1862274, @phosek wrote:

In D74107#1862126, @smeenai wrote:

I've never really understood this block, since the APPLE and WIN32/MSVC conditionals here will reflect the host you're building on and not your actual runtimes target, but this change itself looks fine.

They're affected by CMAKE_SYSTEM_NAME which is usually set for the runtimes build so it's not necessarily the host. I'm still not sure whether this condition is desirable though since most of these tools either support APPLE or WIN32 platforms, or CMake will just ignore them and use the appropriate tool for the given platform. This code predates runtimes build and my changes so I don't know what the motivation behind that condition was.

Right, but the relevant calls to llvm_ExternalProject_Add from inside llvm/runtimes/CMakeLists.txt all happen when it's being included from LLVM's CMake (i.e. it's not the configure for the actual runtimes build), so at that point CMAKE_SYSTEM_NAME will just reflect the host, right?

llvm/cmake/modules/LLVMExternalProjectUtils.cmake

50

llvm-ar, llvm-nm, and llvm-objdump are potentially applicable to MSVC too :) In fact MSVC's llvm-shlib build actually uses llvm-nm: https://github.com/llvm/llvm-project/blob/03a2d0045d2c11f4601bb2cbf5cdd5389ed2d72a/llvm/tools/llvm-shlib/CMakeLists.txt#L162

[phosek]

In D74107#1862361, @smeenai wrote:

In D74107#1862274, @phosek wrote:

In D74107#1862126, @smeenai wrote:

I've never really understood this block, since the APPLE and WIN32/MSVC conditionals here will reflect the host you're building on and not your actual runtimes target, but this change itself looks fine.

They're affected by CMAKE_SYSTEM_NAME which is usually set for the runtimes build so it's not necessarily the host. I'm still not sure whether this condition is desirable though since most of these tools either support APPLE or WIN32 platforms, or CMake will just ignore them and use the appropriate tool for the given platform. This code predates runtimes build and my changes so I don't know what the motivation behind that condition was.

Right, but the relevant calls to llvm_ExternalProject_Add from inside llvm/runtimes/CMakeLists.txt all happen when it's being included from LLVM's CMake (i.e. it's not the configure for the actual runtimes build), so at that point CMAKE_SYSTEM_NAME will just reflect the host, right?

Good point, I've changed the code to instead grep for CMAKE_SYSTEM_NAME in the arguments passed to the external project.

[smeenai]

LGTM

llvm/cmake/modules/LLVMExternalProjectUtils.cmake

function(llvm_ExternalProject_Add name source_dir)
   cmake_parse_arguments(ARG
     "USE_TOOLCHAIN;EXCLUDE_FROM_ALL;NO_INSTALL;ALWAYS_CLEAN"
     "SOURCE_DIR"
     "CMAKE_ARGS;TOOLCHAIN_TOOLS;RUNTIME_LIBRARIES;DEPENDS;EXTRA_TARGETS;PASSTHROUGH_PREFIXES;STRIP_TOOL"
     ${ARGN})
   canonicalize_tool_name(${name} nameCanon)
   if(NOT ARG_TOOLCHAIN_TOOLS)
     set(ARG_TOOLCHAIN_TOOLS clang lld)
-    if(NOT APPLE AND NOT WIN32)
+    if(NOT APPLE AND NOT MSVC)
       list(APPEND ARG_TOOLCHAIN_TOOLS llvm-ar llvm-lipo llvm-ranlib llvm-nm llvm-objcopy llvm-objdump llvm-strip)
+    elseif(APPLE)
+      list(APPEND ARG_TOOLCHAIN_TOOLS llvm-ar llvm-lipo llvm-ranlib)
     endif()
   endif()
   foreach(tool ${ARG_TOOLCHAIN_TOOLS})
     if(TARGET ${tool})
       list(APPEND TOOLCHAIN_TOOLS ${tool})
       list(APPEND TOOLCHAIN_BINS $<TARGET_FILE:${tool}>)
     endif()
   endforeach()