[PATCH] D107898: [CMake] Fix recompile all .inc files with LLVM_OPTIMIZED_TABLEGEN in Visual Studio.

[Daniil Fukalov via Phabricator via llvm-commits]

dfukalov marked an inline comment as done.
dfukalov added inline comments.


================
Comment at: llvm/cmake/modules/CrossCompile.cmake:107
 
+  if (CMAKE_GENERATOR MATCHES "Visual Studio")
+    set(output_path ${output_path}.exe)
----------------
smeenai wrote:
> beanz wrote:
> > smeenai wrote:
> > > Shouldn't this be something like `if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")`? The `.exe` suffix will be present whenever your native system is Windows.
> > Better yet, use `CMAKE_EXECUTABLE_SUFFIX`:
> > https://cmake.org/cmake/help/latest/variable/CMAKE_EXECUTABLE_SUFFIX.html
> Will that be set for the host or target platform though?
> Shouldn't this be something like `if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")`? The `.exe` suffix will be present whenever your native system is Windows.
AFAIK "Visual Studio" generated projects are working on Windows only, so I just used the same approach as in [[ https://reviews.llvm.org/source/llvm-github/browse/main/llvm/cmake/modules/TableGen.cmake$74 | TableGen.cmake ]].

On the other hand, I've just re-checked ninja and make builds on Windows/Cygwin and they doesn't have the issue with re-generating all .inc files for any build although they have dependencies pointed to `llvm-tblgen` binary without the extension.

> Better yet, use CMAKE_EXECUTABLE_SUFFIX:
> https://cmake.org/cmake/help/latest/variable/CMAKE_EXECUTABLE_SUFFIX.html
Thanks for suggestion! It seems the better way is to use `set(output_path ${output_path}${CMAKE_EXECUTABLE_SUFFIX})` unconditionally.

> Will that be set for the host or target platform though?
I guess we're here about host (in this particular case NATIVE) and the path is returned in `output_path_var` variable from this `build_native_tool()`. E.g. in TableGen.cmake it is called as `build_native_tool(${target} ${project}_TABLEGEN_EXE DEPENDS ${target})`.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107898