[PATCH] D97451: [PR48898][CMake] Support MinGW Toolchain tools in llvm_ExternalProject_Add

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/cmake/modules/LLVMExternalProjectUtils.cmake:56
       string(REGEX REPLACE "^-DCMAKE_SYSTEM_NAME=(.*)$" "\\1" _cmake_system_name "${arg}")
+    elseif (arg MATCHES "^-DCMAKE_(C|ASM|CXX)_COMPILER_TARGET=(.*)-windows-(gnu|msvc).*$")
+      if (${CMAKE_MATCH_3} STREQUAL "msvc")
----------------
zero9178 wrote:
> mstorsjo wrote:
> > Technically, you can have triples in other forms too, like `x86_64-win32`; `win32` is an alias for `windows`, and if the `-msvc` suffix is omitted it defaults to the msvc variant.
> > 
> > If this only deals with canonicalized triples it should be fine. And for other cases, I guess it might be fine as well (as you'd get proper results if you pass triples in the expected form at least and we can't expect to mimic all the quirks handled by the llvm internal triple parser).
> I was checking around if there were any functions in other cmake modules or so that could parse or canonicalize triples. I was thinking that for now only supporting canonicalized triples should be fine. Other patterns could technically be added in the future. As far as I am aware, clang would also accept eg. GCC style triple for mingw as well I believe. 
This string matching of the cmake argument list makes me feel like we should be passing the target as a proper function argument, and then have llvm_ExternalProject_Add be responsible for appending all three DCMAKE_(C|CXX|ASM)_COMPILER_TARGET variants to the argument list. Do you mind making that happen? It should be a net savings in lines of code. I see only the test-suite caller of llvm_ExternalProject_add doesn't pass these arguments.

I share the concern that cmake doesn't know how to normalize mingw/gnu/msvc triples, but let's factor the decision out into some kind of "is_msvc_triple" utility, and then later we can teach it to match any of the patterns really matter.


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

https://reviews.llvm.org/D97451