[PATCH] D117852: [CMake][WinMsvc] Replace MSVC_BASE/WINSDK_BASE with LLVM_WINSYSROOT

Sat Jan 22 08:33:49 PST 2022

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

Lg for now; once lld-link supports the flag we can make it nicer :)

================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:212
+if (NOT WINSDK_VER)
+  get_highest_version("${LLVM_WINSYSROOT}/Windows Kits/10/Include" WINSDK_VER)
 endif()
----------------
ychen wrote:
> thakis wrote:
> > clang does this internally if they're not defined. Maybe add -vctoolsversion only if MSVC_VER is defined, and -winsdkversion only if WINSDK_VER is defined?
> yeah, considered that. Later I found out I still need MSVC_VER and WINSDK_VER to specify libpath for the linker.  Guess we need to wait until lld-link accepts `/winsysroot`...
Someone over here is working on that :)

================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:288
     # Prevent CMake from attempting to invoke mt.exe. It only recognizes the slashed form and not the dashed form.
     /manifest:no

----------------
ychen wrote:
> ychen wrote:
> > thakis wrote:
> > > FWIW in Chromium we build our lld-link against libxml2 (with almost everything disabled) these days, and it can do /manifest handling without mt.exe if build that way.
> > Thanks for the info. I'll try removing it locally.
> Oh, okay. It seems LLVM Windows release build does not link libxml2 yet. Is it possible to upstream your code (https://chromium.googlesource.com/chromium/src/+/66ff126e39334b72ec6f23b6d28a60e7efbbc019/tools/clang/scripts/build.py#281) to `llvm/utils/release/build_llvm_package.bat` so the release lld-link works with /manifest?
I wasn't aware of that script. That seems like a good thing to do, yes.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117852