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

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added a comment.

Thanks for the review!



================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:212
+if (NOT WINSDK_VER)
+  get_highest_version("${LLVM_WINSYSROOT}/Windows Kits/10/Include" WINSDK_VER)
 endif()
----------------
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`...


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:217
+  message(SEND_ERROR
+          "Must specify CMake variable LLVM_WINSYSROOT, MSVC_VER and WINSDK_VER")
 endif()
----------------
thakis wrote:
> MSVC_VER and WINSDK_VER are optional, so you probably should only check for the sysroot
same. Optional for the user, still need it to specify linker libpath.


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:266
     --target=${TRIPLE_ARCH}-windows-msvc
     -fms-compatibility-version=19.14
+    -vctoolsversion ${MSVC_VER}
----------------
thakis wrote:
> Do you want to add `/winsysroot` down here too? Or how is that flag getting added?
https://github.com/llvm/llvm-project/blob/4cc514579f467db51907567b305f21b0607b27d1/llvm/cmake/modules/HandleLLVMOptions.cmake#L463


================
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
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117852