[libcxx-commits] [PATCH] D97913: [runtimes] Use add_lit_testsuite to register lit testsuites

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Actually, I had another look at this, and I'm not sure it's right. It used to be `add_lit_testsuite`, but I changed it to `add_lit_target` in

  commit 6f69318c7248275b509ecf0f88eb2ba725aaeb82
  Author: Louis Dionne <ldionne at apple.com>
  Date:   Thu Jul 9 11:54:09 2020 -0400
  
      [runtimes] Allow passing Lit parameters through CMake
      
      This allows passing parameters to the test suites without using
      LLVM_LIT_ARGS. The problem is that we sometimes want to set some
      Lit arguments on the CMake command line, but the Lit parameters in
      a CMake cache file. If the only knob to do that is LLVM_LIT_ARGS,
      the command-line entry overrides the cache one, and the parameters
      set by the cache are ignored.
      
      This fixes a current issue with the build bots that they completely
      ignore the 'std' param set by Lit, because other Lit arguments are
      provided via LLVM_LIT_ARGS on the CMake command-line.

IIRC, the issue was that `add_lit_testsuite` sets several parameters globally, i.e. for all the test suites:

  if(NOT ARG_EXCLUDE_FROM_CHECK_ALL)
      # Register the testsuites, params and depends for the global check rule.
      set_property(GLOBAL APPEND PROPERTY LLVM_LIT_TESTSUITES ${ARG_UNPARSED_ARGUMENTS})
      set_property(GLOBAL APPEND PROPERTY LLVM_LIT_PARAMS ${ARG_PARAMS})
      set_property(GLOBAL APPEND PROPERTY LLVM_LIT_DEPENDS ${ARG_DEPENDS})
      set_property(GLOBAL APPEND PROPERTY LLVM_LIT_EXTRA_ARGS ${ARG_ARGS})
    endif()

This is wrong because we don't want e.g. the libc++ parameters to be used when running the Clang test suite. I agree it's probably better to fix the root cause instead of just *not* using `add_lit_testsuite` though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97913