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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

It sounds like the change in `flang/test/lit.cfg.py` is not needed. Otherwise LGTM!



================
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)
 
----------------
DavidTruby wrote:
> 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.
Sounds like this is an unrelated change - could you remove it? Otherwise tracking the history becomes a bit trickier.

Perhaps we should get rid of `flang_llvm_tools_dir` in a separate patch?


================
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()
----------------
DavidTruby wrote:
> 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!
This makes a lot of sense - thank you for explaining! Perhaps it would be worth expanding the comment above or the commit msg? Your call, I'm fine with the way it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84022