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

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

`/winsysroot` is somewhat new, but I suppose that's fine (?)

Looks fine to me (haven't tried it though). The only real question below is where and how `winsysroot` gets added as flag :)



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


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:217
+  message(SEND_ERROR
+          "Must specify CMake variable LLVM_WINSYSROOT, MSVC_VER and WINSDK_VER")
 endif()
----------------
MSVC_VER and WINSDK_VER are optional, so you probably should only check for the sysroot


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:221
+set(ATLMFC_LIB     "${LLVM_WINSYSROOT}/VC/Tools/MSVC/${MSVC_VER}/atlmfc/lib")
+set(MSVC_INCLUDE   "${LLVM_WINSYSROOT}/VC/Tools/MSVC/${MSVC_VER}/include")
+set(MSVC_LIB       "${LLVM_WINSYSROOT}/VC/Tools/MSVC/${MSVC_VER}/lib")
----------------
given that this is only set to check for existence, maybe this variable (and the check against it) isn't needed?


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:223
+set(MSVC_LIB       "${LLVM_WINSYSROOT}/VC/Tools/MSVC/${MSVC_VER}/lib")
+set(WINSDK_INCLUDE "${LLVM_WINSYSROOT}/Windows Kits/10/Include/${WINSDK_VER}")
+set(WINSDK_LIB     "${LLVM_WINSYSROOT}/Windows Kits/10/Lib/${WINSDK_VER}")
----------------
same here


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:266
     --target=${TRIPLE_ARCH}-windows-msvc
     -fms-compatibility-version=19.14
+    -vctoolsversion ${MSVC_VER}
----------------
Do you want to add `/winsysroot` down here too? Or how is that flag getting added?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117852