[flang] Fix multi-config generator builds.

[DavidTruby]

Summary

Currently the binaries are output directly into the bin subdirectory of the
build directory. This doesn't work correctly with multi-config generators which
should output the binaries into <CONFIG_NAME>/bin instead.

[sscalpone]

Hi David, what is a multi-config generator build? And how do I try it?

[DavidTruby]

In D84022#2162603, @sscalpone wrote:

Hi David, what is a multi-config generator build? And how do I try it?

Some CMake generators allow you to have multiple build configurations in the same build directory: e.g. you can have a single build directory and CMake line but a separate Debug and Release build in that directory. This is the only supported configuration for msbuild on Windows.
If you have a very recent cmake (3.17+) you can try this on Linux with Ninja with the -G"Ninja Mutli-Config" configuration option (instead of e.g. -GNinja). You can then build with: cmake --build . --config Debug, and cmake --build . --config Release, from the same cmake build directory.
This is of course not necessary on Linux, but I've been trying it out and am pre-emptively trying to fix this for when we move to support platforms where multi-config generators are the only option.

[awarzynski]

Thank you for working on this @DavidTruby!

flang/test/lit.cfg.py

64

Shouldn't config.flang_llvm_tools_dir be fixed instead (or _as well_)?

flang/tools/f18/CMakeLists.txt

63–69

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?

[DavidTruby]

flang/test/lit.cfg.py

64

They point to the same directory. We should probably just remove one of them.

flang/tools/f18/CMakeLists.txt

63–69

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!

[awarzynski]

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

flang/test/lit.cfg.py

64

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?

flang/tools/f18/CMakeLists.txt

63–69

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.

[sscalpone]

Michael Kruse reports that he has successfully built flang on Windows. Perhaps he could take a look?

[DavidTruby]

Removed unnecessary change to flang/test/lit.cfg.py

[awarzynski]

To me this looks fairly straightforward. @DavidTruby , could you wait one more day before merging? Perhaps somebody else would like to review (e.g. @Meinersbur) ?

[DavidTruby]

The previous change broke builds again, so I think we should take the other option and just remove the unnecessary variable here.

[PeteSteinfeld]

I would be happy to test out-of-tree builds to determine if this change will work for them.

flang/tools/f18/CMakeLists.txt

67

This command fails in an out-of-tree build because I don't have `LLVM_RUNTIME_OUTPUT_INTDIR`` defined. This results in the build trying to copy flang to the directory /flang`, which doesn't exist.

[PeteSteinfeld]

Note that, for people eager to do out-of-tree builds, just replacing flang/tools/f18/CMakeLists.txt with the previous version allows out-of-tree builds of flang to succeed.

[PeteSteinfeld]

Actually, to get both the build and tests to work correctly I had to restore all of the files in this commit to their previous versions.

[DavidTruby]

Hi Pete, I don't do out of tree builds myself and they aren't defended by buildbots which would be why I didn't spot that this broke that configuration.
You could possibly add if statements around the changes to have the old code in an out-of-tree configuration, using the FLANG_STANDALONE_BUILD cmake variable. Obviously then multi-config builds won't work out-of-tree still, but it's possible nobody cares about that configuration.

[PeteSteinfeld]

Out-of-tree builds are four times faster than in-tree builds. Since I build flang every time I review a change, I do several builds a day (in addition to the builds I do for my own work). Thus, this change significantly degrades my productivity.

You say "You could possibly add if statements ...". Are you implying that you do not plan to amend your change to allow out-of-tree builds in addition to supporting multi-config generator builds?

[DavidTruby]

Out-of-tree builds are four times faster than in-tree builds. Since I build flang every time I review a change, I do several builds a day (in addition to the builds I do for my own work). Thus, this change significantly degrades my productivity.

If the in-tree incremental builds are slow, that seems like a cmake bug; there shouldn't be any difference in performance between the two. Unless you're not rebuilding the LLVM changes each time, which in the general case won't work especially as we get closer integration between flang and LLVM.

You say "You could possibly add if statements ...". Are you implying that you do not plan to amend your change to allow out-of-tree builds in addition to supporting multi-config generator builds?

My understanding is that in general in LLVM out-of-tree builds are considered a "best effort" feature that isn't guaranteed to work. In the flang case they aren't defended by buildbots and so people won't get notified when their patches break them (as I was not notified here).
I personally don't do out of tree builds and so it would be difficult for me to fix this breakage without being able to reproduce it. I can provide some more concrete pointers on here about what a potential fix might look like though:

Around the changes in CMakeLists.txt files, add:

if(FLANG_STANDALONE_BUILD) 
   // old code
else()
  // new code
endif()

In lit.site.cfg.py add:

if @FLANG_STANDALONE_BUILD@:
  config.flang_llvm_tools_dir = "@CMAKE_BINARY_DIR@/bin"
else:
  config.flang_llvm_tools_dir = config.llvm_tools_dir

And in lit.cfg.py change config.llvm_tools_dir to config.flang_llvm_tools_dir

I believe this should change the behaviour back to how it was before for out-of-tree but keep the fixes for in-tree builds.

[tskeith]

I've reverted this in 765b81f6b9 so that out-of-tree builds work while we figure this out.

[Meinersbur]

In D84022#2175761, @awarzynski wrote:

To me this looks fairly straightforward. @DavidTruby , could you wait one more day before merging? Perhaps somebody else would like to review (e.g. @Meinersbur) ?

Multi-config builds usually involve CMAKE_CFG_INTDIR, which I do not being used here. I only built a single configuration in the MSVC IDE yet, so I wouldn't notice name clashes. I was concentrating on compiler errors anyway.

There's an flang.sh file involved, but one cannot assume the presence of a sh-compatible shell on Windows. Why not using a executable-relative path to @FLANG_INTRINSIC_MODULES_DIR@ like clang does? Wouldn't it change between install-path and build-path anyway?

[DavidTruby]

@Meinersbur CMAKE_CFG_INTDIR is being used here indirectly by LLVM_RUNTIME_OUTPUT_INTDIR, which references it. I've also added a comment about the flang.sh script (that previously was getting installed on both platforms) to say that we will now only install it on Linux. I did it this way as I couldn't find a neater way to make it executable on POSIX platforms in a custom command than using chmod.

@tskeith could you explain your rationale for reverting this please? I don't think it's reasonable to expect all patches to be tested with out-of-tree configurations by people that don't perform them, and reverting here seems to raise that expectation. As such I would much rather have seen a patch from someone that needs out-of-tree builds along the lines that I suggested, rather than a revert. I'm not the person in the best position to fix out-of-tree support for this patch as I do not perform out of tree builds...

Furthermore, this patch was up for quite a while before I pushed it and I requested people to test it. It also passed both the pre and post commit testing in LLVM. Generally I would say patches should only be reverted if they break buildbots. For example I do not believe this patch meets any of the criteria for a patch being revertable as per the developer policy: https://llvm.org/docs/DeveloperPolicy.html

[tskeith]

In D84022#2188213, @DavidTruby wrote:

@tskeith could you explain your rationale for reverting this please? I don't think it's reasonable to expect all patches to be tested with out-of-tree configurations by people that don't perform them, and reverting here seems to raise that expectation.

Out-of-tree builds is a supported configuration, documented in the README. I don't think developers should be expected to test every possible build configuration, but if a change breaks a build and can't be fixed right away it should be reverted.

As such I would much rather have seen a patch from someone that needs out-of-tree builds along the lines that I suggested, rather than a revert. I'm not the person in the best position to fix out-of-tree support for this patch as I do not perform out of tree builds...

I agree, a patch would be great, but no one has come up with one yet.

Furthermore, this patch was up for quite a while before I pushed it and I requested people to test it. It also passed both the pre and post commit testing in LLVM. Generally I would say patches should only be reverted if they break buildbots.

Clearly I disagree. We should discuss this at the next Flang Community Technical Call.

[DavidTruby]

In D84022#2188303, @tskeith wrote:

In D84022#2188213, @DavidTruby wrote:

@tskeith could you explain your rationale for reverting this please? I don't think it's reasonable to expect all patches to be tested with out-of-tree configurations by people that don't perform them, and reverting here seems to raise that expectation.

Out-of-tree builds is a supported configuration, documented in the README. I don't think developers should be expected to test every possible build configuration, but if a change breaks a build and can't be fixed right away it should be reverted.

This is effectively the same as asking everyone to test every build configuration; for example, what should I do now that this revert has broken my builds?
I can either revert the revert, or I am forced to test and fix a build configuration I don't use, which leads to me having to test every possible build configuration.
The only solution to not requiring people to test every build configuration is that the burden of fixing build configurations that aren't defended by buildbots should be the on the person requiring those configurations. Otherwise people de-facto have to build with and fix every configuration someone might care about or have their patch reverted like this one.

[DavidTruby]

Fixed by D85078

flang/CMakeLists.txt

 
 else()
   set(FLANG_STANDALONE_BUILD OFF)
   option(FLANG_INCLUDE_TESTS
          "Generate build targets for the Flang unit tests."
          ${LLVM_INCLUDE_TESTS})
   set(FLANG_GTEST_AVAIL 1)
 
-  set(FLANG_BINARY_DIR ${CMAKE_BINARY_DIR}/tools/flang)
+  set(FLANG_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
   set(BACKEND_PACKAGE_STRING "${PACKAGE_STRING}")
   if (LINK_WITH_FIR)
     set(MLIR_MAIN_SRC_DIR ${LLVM_MAIN_SRC_DIR}/../mlir/include ) # --src-root
     set(MLIR_INCLUDE_DIR ${LLVM_MAIN_SRC_DIR}/../mlir/include ) # --includedir
     set(MLIR_TABLEGEN_OUTPUT_DIR ${CMAKE_BINARY_DIR}/tools/mlir/include)
     set(MLIR_TABLEGEN_EXE $<TARGET_FILE:mlir-tblgen>)
     include_directories(SYSTEM ${MLIR_INCLUDE_DIR})
     include_directories(SYSTEM ${MLIR_TABLEGEN_OUTPUT_DIR})
   endif()
 endif()
 
 if(LINK_WITH_FIR)
   # tco tool and FIR lib output directories
-  set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/bin)
-  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/lib)
   # Always build tco tool
   set(LLVM_BUILD_TOOLS ON)
   message(STATUS "Linking driver with FIR and LLVM")
   llvm_map_components_to_libnames(LLVM_COMMON_LIBS support)
   message(STATUS "LLVM libraries: ${LLVM_COMMON_LIBS}")
 endif()
 
 # Add Flang-centric modules to cmake path.

flang/test/lit.cfg.py

 
 # For each occurrence of a flang tool name, replace it with the full path to
 # the build directory holding that tool.
 tools = [
   ToolSubst('%f18', command=FindTool('f18'),
     extra_args=["-intrinsic-module-directory "+config.flang_intrinsic_modules_dir],
     unresolved='fatal')
 ]
-llvm_config.add_tool_substitutions(tools, [config.flang_llvm_tools_dir])
+llvm_config.add_tool_substitutions(tools, config.llvm_tools_dir)
 
 # Enable libpgmath testing
 result = lit_config.params.get("LIBPGMATH")
 if result:
     config.environment["LIBPGMATH"] = True

flang/test/lit.site.cfg.py.in

 @LIT_SITE_CFG_IN_HEADER@
 
 import sys
 
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
 config.flang_obj_root = "@FLANG_BINARY_DIR@"
 config.flang_src_dir = "@FLANG_SOURCE_DIR@"
 config.flang_tools_dir = "@FLANG_TOOLS_DIR@"
 config.flang_intrinsic_modules_dir = "@FLANG_INTRINSIC_MODULES_DIR@"
-config.flang_llvm_tools_dir = "@CMAKE_BINARY_DIR@/bin"
+config.flang_llvm_tools_dir = "@LLVM_TOOLS_DIR@"
 config.python_executable = "@PYTHON_EXECUTABLE@"
 
 # Support substitution of the tools_dir with user parameters. This is
 # used when we can't determine the tool dir at configuration time.
 try:
     config.llvm_tools_dir = config.llvm_tools_dir % lit_config.params
     config.flang_tools_dir = config.flang_tools_dir % lit_config.params
 except KeyError:

flang/tools/f18/CMakeLists.txt

foreach(filename ${MODULES})
   install(FILES ${include}/${filename}.f18.mod DESTINATION include/flang)
 endforeach()
 
 add_custom_target(module_files ALL DEPENDS ${MODULE_FILES})
 
 install(TARGETS f18 DESTINATION bin)
 
 set(FLANG_INTRINSIC_MODULES_DIR ${FLANG_BINARY_DIR}/include/flang)
-configure_file(${CMAKE_CURRENT_SOURCE_DIR}/flang.sh.in ${CMAKE_BINARY_DIR}/tools/flang/bin/flang @ONLY)
-file(COPY ${CMAKE_BINARY_DIR}/tools/flang/bin/flang DESTINATION ${CMAKE_BINARY_DIR}/bin FILE_PERMISSIONS OWNER_EXECUTE OWNER_READ OWNER_WRITE)
+# This flang shell script will only work in a POSIX shell.
+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()
 # The flang script to be installed needs a different path to the headers.
 set(FLANG_INTRINSIC_MODULES_DIR ${CMAKE_INSTALL_PREFIX}/include/flang)
 configure_file(${CMAKE_CURRENT_SOURCE_DIR}/flang.sh.in ${FLANG_BINARY_DIR}/bin/flang-install.sh @ONLY)
 
 install(PROGRAMS ${FLANG_BINARY_DIR}/bin/flang-install.sh DESTINATION bin RENAME flang PERMISSIONS OWNER_EXECUTE OWNER_READ OWNER_WRITE)