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

Mon Jul 27 03:32:06 PDT 2020

krisb marked an inline comment as done.
krisb added a comment.

@phosek thank you for reviewing this!

================
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)
----------------
phosek wrote:
> 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. 
Okay, I made the condition to be `(WIN32 AND NOT BOOTSTRAP_CMAKE_SYSTEM_NAME) OR BOOTSTRAP_CMAKE_SYSTEM_NAME STREQUAL "Windows"` to avoid setting `CMAKE_LINKER` in case of cross-compiling with Windows host and non-Windows target.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80873