[lit] Use Visual Studio build config when testing

[modocache]

Summary

Visual Studio allows the build configuration to be set at *build* time,
and it does not make use of CMAKE_BUILD_TYPE. As a result, the location
of the FileCheck/not executables, which lit's test suite depends upon,
is not known until build time. To inform lit's test suite of where to
find these executables, pass --build_configuration as a --param.

This fixes a test failure when running the lit test suite via Visual
Studio.

Test plan:
On a Windows development environment with Visual Studio installed, run
the following and verify that it succeeds:

cmake \
    C:\Users\modocache\Source\llvm\llvm \
    -G "Visual Studio 14 2015" \
    -DCMAKE_INSTALL_PREFIX=C:\Users\modocache\Source\llvm\install
cmake \
    --build C:\Users\modocache\Source\llvm\build \
    --target check-lit \
    --config Debug

[chapuni]

Would it be generalized with CMAKE_CFG_INTDIR?

[gbedwell]

utils/lit/tests/CMakeLists.txt

26

add_lit_target in AddLLVM.cmake already appends "--param build_mode=${CMAKE_CFG_INTDIR}" to the lit args so you should be able to just make use of that param rather than adding a new one.

utils/lit/tests/lit.cfg

72

This isn't a Windows/Visual Studio specific issue, but a multi-config CMake generator issue. The ones I'm aware of are Visual Studio and Xcode, but there may be others (see https://cmake.org/cmake/help/v3.9/variable/CMAKE_CFG_INTDIR.html ) so I don't think this should be limited on is_windows.

[gbedwell]

Apologies for jumping in on your patch here. I looked into this a bit more because I was convinced that this should be handled at CMake level elsewhere and I think along the way I possibly came up with a different way to solve this. I've posted my findings here:

Comparing "llvm/utils/lit/tests/lit.site.cfg.in" with another similar file "llvm/test/Unit/lit.site.cfg.in" there are a few interesting differences.

Firstly, the latter uses

config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"

rather than:

config.llvm_tools_dir = "@LLVM_TOOLS_BINARY_DIR@"

This LLVM_TOOLS_DIR variable is created by the function configure_lit_site_cfg in AddLLVM.cmake and for all of the other test target CMakeLists.txt files the configure_file step is done via that function rather than calling configure_file directly. LLVM_TOOLS_DIR is based on LLVM_TOOLS_BINARY_DIR but with the build configuration replaced with the string "%(build_config)s".

The next interesting thing in "llvm/test/Unit/lit.site.cfg.in" is this block of code:

# Support substitution of the tools_dir and build_mode 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.llvm_build_mode = config.llvm_build_mode % lit_config.params
except KeyError:
    e = sys.exc_info()[1]
    key, = e.args
    lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key,key))

This appears to be solving the exact problem we're running into by dynamically substituting the value specified with "--param build_mode=<value>" in place of the %(build_config)s string at runtime rather than configure time where it's still unknown for multi-config generators.

With that in mind, borrowing the relevant parts of that file, I tried the following patch which seems to do the trick. What do you think?

diff --git a/utils/lit/tests/CMakeLists.txt b/utils/lit/tests/CMakeLists.txt
index f63fd90..0cb5c2b 100644
--- a/utils/lit/tests/CMakeLists.txt
+++ b/utils/lit/tests/CMakeLists.txt
@@ -2,5 +2,5 @@
 # until the tests are run as we need to copy it into
 # a copy of the tests folder
-configure_file("lit.site.cfg.in" "lit.site.cfg" @ONLY)
+configure_lit_site_cfg("lit.site.cfg.in" "lit.site.cfg")

 # For every lit.cfg in the Inputs tree, create a lit.site.cfg that points at
@@ -12,5 +12,5 @@ foreach(lit_cfg ${inputs_suites})
   file(RELATIVE_PATH LIT_TEST_BIN_DIR "${CMAKE_CURRENT_SOURCE_DIR}" "${LIT_TEST_SRC_DIR}")
   set(LIT_TEST_BIN_DIR "${CMAKE_CURRENT_BINARY_DIR}/${LIT_TEST_BIN_DIR}")
-  configure_file("Inputs/lit.site.cfg.in" "${LIT_TEST_BIN_DIR}/lit.site.cfg" @ONLY)
+  configure_lit_site_cfg("Inputs/lit.site.cfg.in" "${LIT_TEST_BIN_DIR}/lit.site.cfg")
 endforeach()

diff --git a/utils/lit/tests/lit.site.cfg.in b/utils/lit/tests/lit.site.cfg.in
index 745c528..4b66748 100644
--- a/utils/lit/tests/lit.site.cfg.in
+++ b/utils/lit/tests/lit.site.cfg.in
@@ -1,7 +1,18 @@
-## Autogenerated by LLVM/Clang configuration.
-# Do not edit!
+@LIT_SITE_CFG_IN_HEADER@
+
+import sys
+
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
-config.llvm_tools_dir = "@LLVM_TOOLS_BINARY_DIR@"
+config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
+
+# 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
+except KeyError:
+    e = sys.exc_info()[1]
+    key, = e.args
+    lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key,key))

 # Let the main config do the real work.

[modocache]

Apologies for jumping in on your patch here.

Not at all, I learned a lot! I was just trying to fix a buildbot that used Visual Studio, I had no idea the build systems in Xcode and VS both worked this way.

I tried the following patch which seems to do the trick. What do you think?

Looks great! It's funny, I also looked at llvm/test/lit.* in order to spot why this wasn't working, and noticed the fact that LLVM_BINARY_TOOLS_DIR was being used here instead of LLVM_TOOLS_DIR, but where I fell short was noticing LIT_SITE_CFG_IN_HEADER. Thanks to you and @chapuni for pointing that out!

@gbedwell It seems like your diff is more in line with how this fix should be written. Would you care to commit it or submit it for review? Or if you don't have the time I would be happy to.

[gbedwell]

In D36212#829005, @modocache wrote:

@gbedwell It seems like your diff is more in line with how this fix should be written. Would you care to commit it or submit it for review? Or if you don't have the time I would be happy to.

I'm happy either way! I was going to put the patch up for review but I noticed that after running some lit tests there appear to be some artifacts being created and left over outside of the build directory. I'm just rebuilding without the patch now to see whether it's that which is causing them to end up there or if it was happening anyway. For reference, these are:

Untracked files:
  (use "git add <file>..." to include in what will be committed)

        utils/lit/tests/Inputs/max-failures/Output/
        utils/lit/tests/Inputs/shtest-format/external_shell/Output/
        utils/lit/tests/Inputs/shtest-output-printing/Output/
        utils/lit/tests/Inputs/shtest-shell/Output/
        utils/lit/tests/Inputs/shtest-timeout/Output/

Unfortunately, I'm leaving the office early today so have run out of time. If you want to carry on with it please do, otherwise I'll aim to put something up tomorrow morning (UK time).

[modocache]

I'm happy either way! I was going to put the patch up for review but I noticed that after running some lit tests there appear to be some artifacts being created and left over outside of the build directory.

Yeah, I noticed that as well. I'm a little suspicious of D36026.

[modocache]

Yup, I've tried reverting D36026 locally, and after doing so check-lit no longer dirties my source tree. I'll comment there with next steps. IMHO reverting and then applying the changes you suggested might be a good way to go!

[vleschuk]

In D36212#829222, @modocache wrote:

Yup, I've tried reverting D36026 locally, and after doing so check-lit no longer dirties my source tree. I'll comment there with next steps. IMHO reverting and then applying the changes you suggested might be a good way to go!

Please do that and create a new review (referencing this one) with new patch (I don't think updating this one with new patch is a good idea).

[gbedwell]

D36263 created.

[modocache]

Abandoning in favor of D36212. Thanks @gbedwell!

utils/lit/tests/CMakeLists.txt

foreach(lit_cfg ${inputs_suites})
   set(LIT_TEST_BIN_DIR "${CMAKE_CURRENT_BINARY_DIR}/${LIT_TEST_BIN_DIR}")
   configure_file("Inputs/lit.site.cfg.in" "${LIT_TEST_BIN_DIR}/lit.site.cfg" @ONLY)
 endforeach()
 
 # Add rules for lit's own test suite
 add_lit_testsuite(check-lit "Running lit's tests"
   ${CMAKE_CURRENT_BINARY_DIR}
   DEPENDS "FileCheck" "not"
+  # Visual Studio allows the build configuration to be set at *build* time,
+  # and it does not make use of CMAKE_BUILD_TYPE. As a result, the location
+  # of the FileCheck/not executables, which lit's test suite depends upon,
+  # is not known until build time. To inform lit's test suite of where to
+  # find these executables, pass --build_configuration as a --param.
+  PARAMS build_configuration=$<CONFIG>
 )
 
 # For IDEs
 set_target_properties(check-lit PROPERTIES FOLDER "Tests")

utils/lit/tests/lit.cfg

 try:
     import psutil
     lit_config.note('Found python psutil module')
     config.available_features.add("python-psutil")
 except ImportError:
     lit_config.warning('Could not import psutil. Some tests will be skipped and'
                        ' the --timeout command line argument will not work.')
 
-if sys.platform.startswith('win') or sys.platform.startswith('cygwin'):
+is_windows = sys.platform.startswith('win') or sys.platform.startswith('cygwin')
+if is_windows:
     config.available_features.add('windows')
 
 # Add llvm tools directory if this config is being loaded indirectly
 llvm_tools_dir = getattr(config, 'llvm_tools_dir', None)
 if llvm_tools_dir != None:
+    build_configuration = lit_config.params.get('build_configuration', None)
+    if is_windows and build_configuration is not None:
+        # When using a Visual Studio CMake generator, at the time that lit.site.cfg.in
+        # is configured, CMake variables such as LLVM_TOOLS_BINARY_DIR are set using
+        # a path that contains a placeholder value: "$(Configuration)". Here at build
+        # time, replace the placeholder with the actual configuration value.
+        llvm_tools_dir = llvm_tools_dir.replace('$(Configuration)', build_configuration)
     path = os.path.pathsep.join((llvm_tools_dir, config.environment['PATH']))
     config.environment['PATH'] = path