[PATCH] D84022: [flang] Fix multi-config generator builds.

Tue Jul 21 14:12:40 PDT 2020

DavidTruby marked 2 inline comments as done.
DavidTruby added inline comments.

================
Comment at: flang/test/lit.cfg.py:64
 ]
-llvm_config.add_tool_substitutions(tools, [config.flang_llvm_tools_dir])
+llvm_config.add_tool_substitutions(tools, config.llvm_tools_dir)

----------------
awarzynski wrote:
> Shouldn't `config.flang_llvm_tools_dir` be fixed instead (or _as well_)?
They point to the same directory. We should probably just remove one of them.

================
Comment at: flang/tools/f18/CMakeLists.txt:63-69
+if (NOT WIN32)
+  configure_file(${CMAKE_CURRENT_SOURCE_DIR}/flang.sh.in ${CMAKE_CURRENT_BINARY_DIR}/tools/flang/bin/flang @ONLY)
+add_custom_command(TARGET f18
+  POST_BUILD
+  COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_BINARY_DIR}/tools/flang/bin/flang ${LLVM_RUNTIME_OUTPUT_INTDIR}/flang
+  COMMAND chmod +x ${LLVM_RUNTIME_OUTPUT_INTDIR}/flang)
+endif()
----------------
awarzynski wrote:
> IIUC, these changes are unrelated to fixing CMake's multi-config (i.e. even without these changes the multi-config will work on Linux). Could you either update the commit msg, please? Alternatively, send a separate patch?
This is related; essentially, the file(COPY) command used previously is run at cmake configure time, so it doesn't know what config you're building for; it actually weirdly just expands the ${CONFIGURATION} variable to the variable name as a string. So without this change, you end up with the flang scripts being in either build/bin or ${CONFIGURATION}/bin.
I couldn't work out a reasonable way of changing permissions in a cross platform way so I just did it using chmod. However, on Windows this script won't work anyway as it's a bash script, so I thought we might as well just not copy it there and use this implementation here.
I hope that makes sense!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84022