[PATCH] D36212: [lit] Use Visual Studio build config when testing

[Greg Bedwell via Phabricator via llvm-commits]

gbedwell added a comment.

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!
  + at 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.


https://reviews.llvm.org/D36212