[lit] Move the recursiveExpansionLimit setting to TestingConfig

[ldionne]

Summary

The LitConfig is shared across the whole test suite. However, since
enabling recursive expansion can be a breaking change for some test
suites, it's important to confine the setting to test suites that
enable it explicitly.

Note that other issues were raised with the way recursiveExpansionLimit
operates. However, this commit simply moves the setting to the right
place -- the mechanism by which it works can be improved independently.

[ldionne]

Like I mentioned in the commit message, this simply fixes the place where the recursiveExpansionLimit is set. Regardless of that, I still plan to explore addressing concerns raised in https://reviews.llvm.org/D76178, which is somewhat orthogonal.

Accepting for libc++ and libc++abi.

[yln]

llvm/docs/CommandGuide/lit.rst

457–460

Can we stash these changes until we decided how to deal with prefix matching?

460–463

Typo: "on top of"

[delcypher]

LGTM

libcxx/test/lit.cfg

 config.name = 'libc++'
 
 # suffixes: A list of file extensions to treat as test files.
 config.suffixes = ['.pass.cpp', '.fail.cpp', '.sh.cpp', '.pass.mm']
 
 # test_source_root: The root path where tests are located.
 config.test_source_root = os.path.dirname(__file__)
 
+# Allow expanding substitutions that are based on other substitutions
+config.recursiveExpansionLimit = 10
+
 loaded_site_cfg = getattr(config, 'loaded_site_config', False)
 if not loaded_site_cfg:
     import libcxx.test.config
     libcxx.test.config.loadSiteConfig(lit_config, config, 'libcxx_site_config',
                                       'LIBCXX_SITE_CONFIG')
 
 # Infer the test_exec_root from the libcxx_object root.
 obj_root = getattr(config, 'libcxx_obj_root', None)

libcxx/utils/libcxx/test/format.py

def _execute(self, test, lit_config):
             lit_config.fatal('Unsupported RUN line found in test %s' % name)
 
         tmpDir, tmpBase = lit.TestRunner.getTempPaths(test)
         substitutions = lit.TestRunner.getDefaultSubstitutions(
             test, tmpDir, tmpBase, normalize_slashes=self.execute_external)
 
         # Apply substitutions in FILE_DEPENDENCIES markup
         data_files = lit.TestRunner.applySubstitutions(test.file_dependencies, substitutions,
-                                                       recursion_limit=10)
+                                                       recursion_limit=test.config.recursiveExpansionLimit)
         local_cwd = os.path.dirname(test.getSourcePath())
         data_files = [f if os.path.isabs(f) else os.path.join(local_cwd, f) for f in data_files]
         substitutions.append(('%{file_dependencies}', ' '.join(data_files)))
 
         script = lit.TestRunner.applySubstitutions(script, substitutions,
-                                                   recursion_limit=10)
+                                                   recursion_limit=test.config.recursiveExpansionLimit)
 
         test_cxx = copy.deepcopy(self.cxx)
         if is_fail_test:
             test_cxx.useCCache(False)
             test_cxx.useWarnings(False)
         extra_modules_defines = self._get_parser('MODULES_DEFINES:',
                                                  parsers).getValue()
         if '-fmodules' in test.config.available_features:

libcxx/utils/libcxx/test/newformat.py

class CxxStandardLibraryTest(lit.formats.TestFormat):
 
     # Utility function to add compile flags in lit.local.cfg files.
     def addCompileFlags(self, config, *flags):
         string = ' '.join(flags)
         config.substitutions = [(s, x + ' ' + string) if s == '%{compile_flags}' else (s, x) for (s, x) in config.substitutions]
 
     # Modified version of lit.TestRunner.executeShTest to handle custom parsers correctly.
     def _executeShTest(self, test, litConfig, steps, fileDependencies=None):
-        recursiveExpansionLimit = 10 # TODO: Use the value in litConfig once we set it.
         if test.config.unsupported:
             return lit.Test.Result(lit.Test.UNSUPPORTED, 'Test is unsupported')
 
         additionalCompileFlags = []
         fileDependencies = fileDependencies or []
         parsers = [
             lit.TestRunner.IntegratedTestKeywordParser('FILE_DEPENDENCIES:',
                                                        lit.TestRunner.ParserKind.LIST,
def _executeShTest(self, test, litConfig, steps, fileDependencies=None):
         substitutions = lit.TestRunner.getDefaultSubstitutions(test, tmpDir, tmpBase,
                                                                normalize_slashes=useExternalSh)
 
         # Perform substitutions inside FILE_DEPENDENCIES lines (or injected dependencies).
         # This allows using variables like %t in file dependencies. Also note that we really
         # need to resolve %{file_dependencies} now, because otherwise we won't be able to
         # make all paths absolute below.
         fileDependencies = lit.TestRunner.applySubstitutions(fileDependencies, substitutions,
-                                                             recursion_limit=recursiveExpansionLimit)
+                                                             recursion_limit=test.config.recursiveExpansionLimit)
 
         # Add the %{file_dependencies} substitution before we perform substitutions
         # inside the script.
         testDir = os.path.dirname(test.getSourcePath())
         fileDependencies = [f if os.path.isabs(f) else os.path.join(testDir, f) for f in fileDependencies]
         substitutions.append(('%{file_dependencies}', ' '.join(map(pipes.quote, fileDependencies))))
 
         # Perform substitution in the script itself.
         script = lit.TestRunner.applySubstitutions(script, substitutions,
-                                                   recursion_limit=recursiveExpansionLimit)
+                                                   recursion_limit=test.config.recursiveExpansionLimit)
 
         return lit.TestRunner._runShTest(test, litConfig, useExternalSh, script, tmpBase)

libcxxabi/test/lit.cfg

if 'PYLINT_IMPORT' in os.environ:
     lit_config = object()
 
 # name: The name of this test suite.
 config.name = 'libc++abi'
 
 # suffixes: A list of file extensions to treat as test files.
 config.suffixes = ['.cpp', '.s']
 
+# Allow expanding substitutions that are based on other substitutions
+config.recursiveExpansionLimit = 10
+
 # test_source_root: The root path where tests are located.
 config.test_source_root = os.path.dirname(__file__)
 
+# Allow expanding substitutions that are based on other substitutions
+config.recursiveExpansionLimit = 10
+
 # Infer the libcxx_test_source_root for configuration import.
 # If libcxx_source_root isn't specified in the config, assume that the libcxx
 # and libcxxabi source directories are sibling directories.
 libcxx_src_root = getattr(config, 'libcxx_src_root', None)
 if not libcxx_src_root:
     libcxx_src_root = os.path.join(config.test_source_root, '../../libcxx')
 libcxx_test_src_root = os.path.join(libcxx_src_root, 'utils')
 if os.path.isfile(os.path.join(libcxx_test_src_root, 'libcxx', '__init__.py')):

llvm/docs/CommandGuide/lit.rst

  %:T                     On Windows, %/T but a ``:`` is removed if its the second character.
                          Otherwise, %T but with a single leading ``/`` removed.
  ======================= ==============
 
 Other substitutions are provided that are variations on this base set and
 further substitution patterns can be defined by each test module. See the
 modules :ref:`local-configuration-files`.
 
 By default, substitutions are expanded exactly once, so that if e.g. a
 substitution ``%build`` is defined in top of another substitution ``%cxx``,
 ``%build`` will expand to ``%cxx`` textually, not to what ``%cxx`` expands to.
-However, if the ``recursiveExpansionLimit`` property of the ``LitConfig`` is
-set to a non-negative integer, substitutions will be expanded recursively until
-that limit is reached. It is an error if the limit is reached and expanding
-substitutions again would yield a different result.
+However, if the ``recursiveExpansionLimit`` property of the ``TestingConfig``
+is set to a non-negative integer, substitutions will be expanded recursively
+until that limit is reached. It is an error if the limit is reached and
+expanding substitutions again would yield a different result.
 
 More detailed information on substitutions can be found in the
 :doc:`../TestingGuide`.
 
 TEST RUN OUTPUT FORMAT
 ~~~~~~~~~~~~~~~~~~~~~~
 
 The :program:`lit` output for a test run conforms to the following schema, in

llvm/utils/lit/lit/LitConfig.py

def __init__(self, progname, path, quiet,
             else:
                 # The default is 'summary'.
                 self.valgrindArgs.append('--leak-check=no')
             self.valgrindArgs.extend(self.valgrindUserArgs)
 
         self.maxIndividualTestTime = maxIndividualTestTime
         self.parallelism_groups = parallelism_groups
         self.echo_all_commands = echo_all_commands
-        self._recursiveExpansionLimit = None
-
-    @property
-    def recursiveExpansionLimit(self):
-        return self._recursiveExpansionLimit
-
-    @recursiveExpansionLimit.setter
-    def recursiveExpansionLimit(self, value):
-        if value is not None and not isinstance(value, int):
-            self.fatal('recursiveExpansionLimit must be either None or an integer (got <{}>)'.format(value))
-        if isinstance(value, int) and value < 0:
-            self.fatal('recursiveExpansionLimit must be a non-negative integer (got <{}>)'.format(value))
-        self._recursiveExpansionLimit = value
 
     @property
     def maxIndividualTestTime(self):
         """
             Interface for getting maximum time to spend executing
             a single test
         """
         return self._maxIndividualTestTime

llvm/utils/lit/lit/TestRunner.py

def executeShTest(test, litConfig, useExternalSh,
     if litConfig.noExecute:
         return lit.Test.Result(Test.PASS)
 
     tmpDir, tmpBase = getTempPaths(test)
     substitutions = list(extra_substitutions)
     substitutions += getDefaultSubstitutions(test, tmpDir, tmpBase,
                                              normalize_slashes=useExternalSh)
     script = applySubstitutions(script, substitutions,
-                                recursion_limit=litConfig.recursiveExpansionLimit)
+                                recursion_limit=test.config.recursiveExpansionLimit)
 
     return _runShTest(test, litConfig, useExternalSh, script, tmpBase)

llvm/utils/lit/lit/TestingConfig.py

 import os
 import sys
 
 
-class TestingConfig:
+class TestingConfig(object):
     """"
     TestingConfig - Information on the tests inside a suite.
     """
 
     @staticmethod
     def fromdefaults(litConfig):
         """
         fromdefaults(litConfig) -> TestingConfig
def __init__(self, parent, name, suffixes, test_format,
         self.pipefail = pipefail
         # This list is used by TestRunner.py to restrict running only tests that
         # require one of the features in this list if this list is non-empty.
         # Configurations can set this list to restrict the set of tests to run.
         self.limit_to_features = set(limit_to_features)
         # Whether the suite should be tested early in a given run.
         self.is_early = bool(is_early)
         self.parallelism_group = parallelism_group
+        self._recursiveExpansionLimit = None
+
+    @property
+    def recursiveExpansionLimit(self):
+        return self._recursiveExpansionLimit
+
+    @recursiveExpansionLimit.setter
+    def recursiveExpansionLimit(self, value):
+        if value is not None and not isinstance(value, int):
+            raise ValueError('recursiveExpansionLimit must be either None or an integer (got <{}>)'.format(value))
+        if isinstance(value, int) and value < 0:
+            raise ValueError('recursiveExpansionLimit must be a non-negative integer (got <{}>)'.format(value))
+        self._recursiveExpansionLimit = value
 
     def finish(self, litConfig):
         """finish() - Finish this config object, after loading is complete."""
 
         self.name = str(self.name)
         self.suffixes = set(self.suffixes)
         self.environment = dict(self.environment)
         self.substitutions = list(self.substitutions)

llvm/utils/lit/tests/Inputs/shtest-recursive-substitution/does-not-substitute-within-limit/lit.cfg

 import lit.formats
 config.name = 'does-not-substitute-within-limit'
 config.suffixes = ['.py']
 config.test_format = lit.formats.ShTest()
 config.test_source_root = None
 config.test_exec_root = None
 
 config.substitutions = [("%rec1", "STOP"), ("%rec2", "%rec1"),
                         ("%rec3", "%rec2"), ("%rec4", "%rec3"),
                         ("%rec5", "%rec4")]
 
-lit_config.recursiveExpansionLimit = 2
+config.recursiveExpansionLimit = 2

llvm/utils/lit/tests/Inputs/shtest-recursive-substitution/negative-integer/lit.cfg

 import lit.formats
 config.name = 'negative-integer'
 config.suffixes = ['.py']
 config.test_format = lit.formats.ShTest()
 config.test_source_root = None
 config.test_exec_root = None
 
-lit_config.recursiveExpansionLimit = -4
+config.recursiveExpansionLimit = -4

llvm/utils/lit/tests/Inputs/shtest-recursive-substitution/not-an-integer/lit.cfg

 import lit.formats
 config.name = 'not-an-integer'
 config.suffixes = ['.py']
 config.test_format = lit.formats.ShTest()
 config.test_source_root = None
 config.test_exec_root = None
 
-lit_config.recursiveExpansionLimit = "not-an-integer"
+config.recursiveExpansionLimit = "not-an-integer"

llvm/utils/lit/tests/Inputs/shtest-recursive-substitution/set-to-none/lit.cfg

 import lit.formats
 config.name = 'set-to-none'
 config.suffixes = ['.py']
 config.test_format = lit.formats.ShTest()
 config.test_source_root = None
 config.test_exec_root = None
 
-lit_config.recursiveExpansionLimit = None
+config.recursiveExpansionLimit = None

llvm/utils/lit/tests/Inputs/shtest-recursive-substitution/substitutes-within-limit/lit.cfg

 import lit.formats
 config.name = 'substitutes-within-limit'
 config.suffixes = ['.py']
 config.test_format = lit.formats.ShTest()
 config.test_source_root = None
 config.test_exec_root = None
 
 config.substitutions = [("%rec1", "STOP"), ("%rec2", "%rec1"),
                         ("%rec3", "%rec2"), ("%rec4", "%rec3"),
                         ("%rec5", "%rec4")]
 
-lit_config.recursiveExpansionLimit = 5
+config.recursiveExpansionLimit = 5