[PATCH] D80873: [clang][cmake] Force CMAKE_LINKER for multistage build in case of BOOTSTRAP_LLVM_ENABLE_LLD and MSVC

Tue Jul 21 10:52:55 PDT 2020

phosek accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.

LGTM

================
Comment at: clang/CMakeLists.txt:751
+  if(BOOTSTRAP_LLVM_ENABLE_LLD)
+    if(MSVC AND NOT BOOTSTRAP_CMAKE_SYSTEM_NAME)
+      set(${CLANG_STAGE}_LINKER -DCMAKE_LINKER=${LLVM_RUNTIME_OUTPUT_INTDIR}/lld-link.exe)
----------------
krisb wrote:
> phosek wrote:
> > I don't understand the second part of this condition, can you elaborate? Why not set `CMAKE_LINKER` to `lld-link.exe` even if `BOOTSTRAP_CMAKE_SYSTEM_NAME STREQUAL "Windows"`?
> The only reason that stopped me from adding `BOOTSTRAP_CMAKE_SYSTEM_NAME STREQUAL "Windows"` case here is that there is no way to keep it working in the future (or, at least, I don't know one). Moreover, the build which sets BOOTSTRAP_CMAKE_SYSTEM_NAME equal to "Windows" is currently broken.
> Since the same issue is exposed if to build LLVM with `clang/cmake/caches/DistributionExample.cmake` on Windows, I included only the changes that contribute to making this configuration buildable. I wanted to avoid making the impression that some other configurations are supported (because they are not) and also avoid adding any dead code that nobody would use and that would easily be broken.
> 
I'd prefer to use `WIN32 OR BOOTSTRAP_CMAKE_SYSTEM_NAME STREQUAL "Windows"` here even if we don't support cross-compilation to Windows now as I hope it's something we're going to support in the future and this will be one less thing we need to address. Alternatively, please at least leave a `TODO` so it's easier to find later. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80873