[libcxx] Refactor lit.cfg.

[danalbert]

Summary

The purely imperative format of the configuration performed in lit.cfg was making merge conflicts with changes I have for Android an unbelievable pain in the ass. I've moved all of the configuration into a Configuration class, with each piece of configuration happening in a different method. This way I can avoid merge conflicts, and any new features that get added (as have been done with the sanitizers, the -std flag, etc.) can be easily applied to Android as well.

[danalbert]

Once again, Phabricator ruined a perfectly good diff. Git shows a nice clean diff, so it's probably easier to apply this patch to your local tree (against r215882, there were a few things submitted while I was working on this that I need to rebase on to) and read it there.

[jroelofs]

test/lit.cfg

237

This pattern:

self.src_root = self.lit_config.params.get('libcxx_src_root', None)
if self.src_root is None:
    self.src_root = getattr(self.config, 'libcxx_src_root', None)
    if self.src_root is None:
        self.src_root = ...

happens a lot. Maybe it's worth abstracting it out?

[danalbert]

• Factor out some of the configuration logic.

[EricWF]

It makes me very happy to approve this! Now commit it before we have merge conflicts!

A few nits you can choose to ignore or implement:

• I had trouble finding the start to the configuration class. Maybe add better visual separation.
• Perhaps we can add better visual separation between methods as well.
• We lose a lot of space to indentation. I would prefer 2 space tabs. (even though all my editors are set to 4).

Either way LGTM.

[danalbert]

Rebased.

[danalbert]

r216196.

test/lit.cfg

def _evaluate_test(self, test, lit_config):
                     return lit.Test.FAIL, report
             finally:
                 try:
                     os.remove(exec_path)
                 except:
                     pass
         return lit.Test.PASS, ""
 
-# name: The name of this test suite.
-config.name = 'libc++'
 
-# suffixes: A list of file extensions to treat as test files.
-config.suffixes = ['.cpp']
+class Configuration(object):
+    def __init__(self, lit_config, config):
+        self.lit_config = lit_config
+        self.config = config
+        self.cxx = None
+        self.src_root = None
+        self.obj_root = None
+        self.env = {}
+        self.compile_flags = []
+        self.link_flags = []
+        self.use_system_lib = False
+
+        if platform.system() not in ('Darwin', 'FreeBSD', 'Linux'):
+            self.lit_config.fatal("unrecognized system")
+
+    def get_lit_conf(self, name, default=None):
+        val = self.lit_config.params.get(name, None)
+        if val is None:
+            val = getattr(self.config, name, None)
+            if val is None:
+                val = default
+        return val
+
+    def configure(self):
+        self.configure_cxx()
+        self.configure_triple()
+        self.configure_src_root()
+        self.configure_obj_root()
+        self.configure_use_system_lib()
+        self.configure_env()
+        self.configure_std_flag()
+        self.configure_compile_flags()
+        self.configure_link_flags()
+        self.configure_sanitizer()
+        self.configure_features()
+
+    def get_test_format(self):
+        return LibcxxTestFormat(
+            self.cxx,
+            cpp_flags=['-nostdinc++'] + self.compile_flags,
+            ld_flags=['-nodefaultlibs'] + self.link_flags,
+            exec_env=self.env)
 
-# test_source_root: The root path where tests are located.
-config.test_source_root = os.path.dirname(__file__)
+    def configure_cxx(self):
+        # Gather various compiler parameters.
+        self.cxx = self.get_lit_conf('cxx_under_test')
+
+        # If no specific cxx_under_test was given, attempt to infer it as
+        # clang++.
+        if self.cxx is None:
+            clangxx = lit.util.which('clang++',
+                                     self.config.environment['PATH'])
+            if clangxx:
+                self.cxx = clangxx
+                self.lit_config.note(
+                    "inferred cxx_under_test as: %r" % self.cxx)
+        if not self.cxx:
+            self.lit_config.fatal('must specify user parameter cxx_under_test '
+                                  '(e.g., --param=cxx_under_test=clang++)')
 
+    def configure_src_root(self):
+        self.src_root = self.get_lit_conf(
+            'libcxx_src_root', os.path.dirname(self.config.test_source_root))
+
+    def configure_obj_root(self):
+        self.obj_root = self.get_lit_conf('libcxx_obj_root', self.src_root)
+
+    def configure_use_system_lib(self):
+        # This test suite supports testing against either the system library or
+        # the locally built one; the former mode is useful for testing ABI
+        # compatibility between the current headers and a shipping dynamic
+        # library.
+        use_system_lib_str = self.get_lit_conf('use_system_lib')
+        if use_system_lib_str:
+            if use_system_lib_str.lower() in ('1', 'true'):
+                self.use_system_lib = True
+            elif use_system_lib_str.lower() in ('', '0', 'false'):
+                self.use_system_lib = False
+            else:
+                self.lit_config.fatal(
+                    'user parameter use_system_lib should be 0 or 1')
+        else:
+            # Default to testing against the locally built libc++ library.
+            self.use_system_lib = False
+            self.lit_config.note(
+                "inferred use_system_lib as: %r" % self.use_system_lib)
+
+    def configure_features(self):
         # Figure out which of the required locales we support
         locales = {
             'Darwin': {
                 'en_US.UTF-8': 'en_US.UTF-8',
                 'cs_CZ.ISO8859-2': 'cs_CZ.ISO8859-2',
                 'fr_FR.UTF-8': 'fr_FR.UTF-8',
                 'fr_CA.ISO8859-1': 'cs_CZ.ISO8859-1',
                 'ru_RU.UTF-8': 'ru_RU.UTF-8',
                 'zh_CN.UTF-8': 'zh_CN.UTF-8',
             },
-    'FreeBSD' : {
+            'FreeBSD': {
                 'en_US.UTF-8': 'en_US.UTF-8',
                 'cs_CZ.ISO8859-2': 'cs_CZ.ISO8859-2',
                 'fr_FR.UTF-8': 'fr_FR.UTF-8',
                 'fr_CA.ISO8859-1': 'fr_CA.ISO8859-1',
                 'ru_RU.UTF-8': 'ru_RU.UTF-8',
                 'zh_CN.UTF-8': 'zh_CN.UTF-8',
             },
             'Linux': {
                 'en_US.UTF-8': 'en_US.UTF-8',
                 'cs_CZ.ISO8859-2': 'cs_CZ.ISO-8859-2',
                 'fr_FR.UTF-8': 'fr_FR.UTF-8',
                 'fr_CA.ISO8859-1': 'fr_CA.ISO-8859-1',
                 'ru_RU.UTF-8': 'ru_RU.UTF-8',
                 'zh_CN.UTF-8': 'zh_CN.UTF-8',
             },
             'Windows': {
                 'en_US.UTF-8': 'English_United States.1252',
                 'cs_CZ.ISO8859-2': 'Czech_Czech Republic.1250',
                 'fr_FR.UTF-8': 'French_France.1252',
                 'fr_CA.ISO8859-1': 'French_Canada.1252',
                 'ru_RU.UTF-8': 'Russian_Russia.1251',
                 'zh_CN.UTF-8': 'Chinese_China.936',
             },
         }
 
+        default_locale = locale.setlocale(locale.LC_ALL)
         for feature, loc in locales[platform.system()].items():
             try:
                 locale.setlocale(locale.LC_ALL, loc)
-        config.available_features.add('locale.{}'.format(feature))
+                self.config.available_features.add('locale.{}'.format(feature))
             except:
-        lit_config.warning('The locale {} is not supported by your platform. '
-                           'Some tests will be unsupported.'.format(loc))
-
-# Gather various compiler parameters.
-cxx_under_test = lit_config.params.get('cxx_under_test', None)
-if cxx_under_test is None:
-    cxx_under_test = getattr(config, 'cxx_under_test', None)
-
-    # If no specific cxx_under_test was given, attempt to infer it as clang++.
-    if cxx_under_test is None:
-        clangxx = lit.util.which('clang++', config.environment['PATH'])
-        if clangxx is not None:
-            cxx_under_test = clangxx
-    lit_config.note("inferred cxx_under_test as: %r" % (cxx_under_test,))
-if cxx_under_test is None:
-    lit_config.fatal('must specify user parameter cxx_under_test '
-                     '(e.g., --param=cxx_under_test=clang++)')
+                self.lit_config.warning('The locale {} is not supported by '
+                                        'your platform. Some tests will be '
+                                        'unsupported.'.format(loc))
+        locale.setlocale(locale.LC_ALL, default_locale)
+
+        # Write an "available feature" that combines the triple when
+        # use_system_lib is enabled. This is so that we can easily write XFAIL
+        # markers for tests that are known to fail with versions of libc++ as
+        # were shipped with a particular triple.
+        if self.use_system_lib:
+            # Drop sub-major version components from the triple, because the
+            # current XFAIL handling expects exact matches for feature checks.
+            sanitized_triple = re.sub(
+                r"([^-]+)-([^-]+)-([^-.]+).*", r"\1-\2-\3",
+                self.config.target_triple)
+            self.config.available_features.add(
+                'with_system_lib=%s' % sanitized_triple)
 
-libcxx_src_root = lit_config.params.get('libcxx_src_root', None)
-if libcxx_src_root is None:
-    libcxx_src_root = getattr(config, 'libcxx_src_root', None)
-    if libcxx_src_root is None:
-        libcxx_src_root = os.path.dirname(config.test_source_root)
-
-libcxx_obj_root = lit_config.params.get('libcxx_obj_root', None)
-if libcxx_obj_root is None:
-    libcxx_obj_root = getattr(config, 'libcxx_obj_root', None)
-    if libcxx_obj_root is None:
-        libcxx_obj_root = libcxx_src_root
-
-# This test suite supports testing against either the system library or the
-# locally built one; the former mode is useful for testing ABI compatibility
-# between the current headers and a shipping dynamic library.
-use_system_lib_str = lit_config.params.get('use_system_lib', None)
-if use_system_lib_str is not None:
-    if use_system_lib_str.lower() in ('1', 'true'):
-        use_system_lib = True
-    elif use_system_lib_str.lower() in ('', '0', 'false'):
-        use_system_lib = False
-    else:
-        lit_config.fatal('user parameter use_system_lib should be 0 or 1')
-else:
-    # Default to testing against the locally built libc++ library.
-    use_system_lib = False
-    lit_config.note("inferred use_system_lib as: %r" % (use_system_lib,))
+    def configure_compile_flags(self):
+        # Configure extra compiler flags.
+        self.compile_flags += ['-I' + self.src_root + '/include',
+                               '-I' + self.src_root + '/test/support']
 
-link_flags = []
-link_flags_str = lit_config.params.get('link_flags', None)
-if link_flags_str is None:
-    link_flags_str = getattr(config, 'link_flags', None)
+    def configure_link_flags(self):
+        self.link_flags += ['-L' + self.obj_root + '/lib', '-lc++']
+        link_flags_str = self.get_lit_conf('link_flags')
         if link_flags_str is None:
-      cxx_abi = getattr(config, 'cxx_abi', 'libcxxabi')
+            cxx_abi = self.get_lit_conf('cxx_abi', 'libcxxabi')
             if cxx_abi == 'libstdc++':
-        link_flags += ['-lstdc++']
+                self.link_flags += ['-lstdc++']
             elif cxx_abi == 'libsupc++':
-        link_flags += ['-lsupc++']
+                self.link_flags += ['-lsupc++']
             elif cxx_abi == 'libcxxabi':
-        link_flags += ['-lc++abi']
+                self.link_flags += ['-lc++abi']
             elif cxx_abi == 'libcxxrt':
-        link_flags += ['-lcxxrt']
+                self.link_flags += ['-lcxxrt']
             elif cxx_abi == 'none':
                 pass
             else:
-        lit_config.fatal('C++ ABI setting %s unsupported for tests' % cxx_abi)
+                self.lit_config.fatal(
+                    'C++ ABI setting %s unsupported for tests' % cxx_abi)
 
             if sys.platform == 'darwin':
-        link_flags += ['-lSystem']
+                self.link_flags += ['-lSystem']
             elif sys.platform == 'linux2':
-        link_flags += [ '-lgcc_eh', '-lc', '-lm', '-lpthread',
+                self.link_flags += ['-lgcc_eh', '-lc', '-lm', '-lpthread',
                                     '-lrt', '-lgcc_s']
             elif sys.platform.startswith('freebsd'):
-        link_flags += ['-lc', '-lm', '-pthread', '-lgcc_s']
+                self.link_flags += ['-lc', '-lm', '-pthread', '-lgcc_s']
             else:
-        lit_config.fatal("unrecognized system")
+                self.lit_config.fatal("unrecognized system")
 
-      lit_config.note("inferred link_flags as: %r" % (link_flags,))
-if not link_flags_str is None:
-    link_flags += shlex.split(link_flags_str)
+            self.lit_config.note(
+                "inferred link_flags as: %r" % self.link_flags)
+        if link_flags_str:
+            self.link_flags += shlex.split(link_flags_str)
+
+        if sys.platform == 'linux2':
+            if not self.use_system_lib:
+                self.link_flags += ['-Wl,-R', self.obj_root + '/lib']
+            self.compile_flags += ['-D__STDC_FORMAT_MACROS',
+                                   '-D__STDC_LIMIT_MACROS',
+                                   '-D__STDC_CONSTANT_MACROS']
+        elif sys.platform.startswith('freebsd'):
+            if not self.use_system_lib:
+                self.link_flags += ['-Wl,-R', self.obj_root + '/lib']
 
-# Configure extra compiler flags.
-include_paths = ['-I' + libcxx_src_root + '/include',
-    '-I' + libcxx_src_root + '/test/support']
-library_paths = ['-L' + libcxx_obj_root + '/lib']
-compile_flags = []
-
-# Try and get the std version from the command line. Fall back to default given
-# in lit.site.cfg is not present. If default is not present then force c++11.
-std = lit_config.params.get('std', None)
-if std is None:
-    std = getattr(config, 'std', None)
+    def configure_std_flag(self):
+        # Try and get the std version from the command line. Fall back to
+        # default given in lit.site.cfg is not present. If default is not
+        # present then force c++11.
+        std = self.get_lit_conf('std')
         if std is None:
             std = 'c++11'
-        lit_config.note('using default std: \'-std=c++11\'')
+            self.lit_config.note('using default std: \'-std=c++11\'')
+        self.compile_flags += ['-std={}'.format(std)]
+        self.config.available_features.add(std)
+
+    def configure_sanitizer(self):
+        san = self.get_lit_conf('llvm_use_sanitizer', '').strip()
+        if san:
+            self.compile_flags += ['-fno-omit-frame-pointer']
+            if san == 'Address':
+                self.compile_flags += ['-fsanitize=address']
+                self.config.available_features.add('asan')
+            elif san == 'Memory' or san == 'MemoryWithOrigins':
+                self.compile_flags += ['-fsanitize=memory']
+                if san == 'MemoryWithOrigins':
+                    self.compile_flags += ['-fsanitize-memory-track-origins']
+                self.config.available_features.add('msan')
             else:
-    lit_config.note('using user specified std: \'-std={}\''.format(std))
-compile_flags += ['-std={}'.format(std)]
-config.available_features.add(std)
-
-built_w_san = getattr(config, 'llvm_use_sanitizer')
-if built_w_san and built_w_san.strip():
-    built_w_san = built_w_san.strip()
-    compile_flags += ['-fno-omit-frame-pointer']
-    if built_w_san == 'Address':
-        compile_flags += ['-fsanitize=address']
-        config.available_features.add('asan')
-    elif built_w_san == 'Memory' or built_w_san == 'MemoryWithOrigins':
-        compile_flags += ['-fsanitize=memory']
-        if built_w_san == 'MemoryWithOrigins':
-            compile_flags += ['-fsanitize-memory-track-origins']
-        config.available_features.add('msan')
-    else:
-        lit_config.fatal(
-            'unsupported value for libcxx_use_sanitizer: {}'.format(built_w_san))
+                self.lit_config.fatal('unsupported value for '
+                                      'libcxx_use_san: {}'.format(san))
+
+    def configure_triple(self):
+        # Get or infer the target triple.
+        self.config.target_triple = self.get_lit_conf('target_triple')
+        # If no target triple was given, try to infer it from the compiler
+        # under test.
+        if not self.config.target_triple:
+            self.config.target_triple = lit.util.capture(
+                [self.cxx, '-dumpmachine']).strip()
+            self.lit_config.note(
+                "inferred target_triple as: %r" % self.config.target_triple)
 
+    def configure_env(self):
         # Configure extra linker parameters.
-exec_env = {}
         if sys.platform == 'darwin':
-    if not use_system_lib:
-        exec_env['DYLD_LIBRARY_PATH'] = os.path.join(libcxx_obj_root, 'lib')
-elif sys.platform == 'linux2':
-    if not use_system_lib:
-        link_flags += ['-Wl,-R', libcxx_obj_root + '/lib']
-    compile_flags += ['-D__STDC_FORMAT_MACROS', '-D__STDC_LIMIT_MACROS',
-        '-D__STDC_CONSTANT_MACROS']
-elif sys.platform.startswith('freebsd'):
-    if not use_system_lib:
-        link_flags += ['-Wl,-R', libcxx_obj_root + '/lib']
-else:
-    lit_config.fatal("unrecognized system")
+            if not self.use_system_lib:
+                self.env['DYLD_LIBRARY_PATH'] = os.path.join(self.obj_root,
+                                                             'lib')
 
-config.test_format = LibcxxTestFormat(
-    cxx_under_test,
-    cpp_flags = ['-nostdinc++'] + compile_flags + include_paths,
-    ld_flags = ['-nodefaultlibs'] + library_paths + ['-lc++'] + link_flags,
-    exec_env = exec_env)
 
-# Get or infer the target triple.
-config.target_triple = lit_config.params.get('target_triple', None)
-# If no target triple was given, try to infer it from the compiler under test.
-if config.target_triple is None:
-    config.target_triple = lit.util.capture(
-        [cxx_under_test, '-dumpmachine']).strip()
-    lit_config.note("inferred target_triple as: %r" % (config.target_triple,))
-
-# Write an "available feature" that combines the triple when use_system_lib is
-# enabled. This is so that we can easily write XFAIL markers for tests that are
-# known to fail with versions of libc++ as were shipped with a particular
-# triple.
-if use_system_lib:
-    # Drop sub-major version components from the triple, because the current
-    # XFAIL handling expects exact matches for feature checks.
-    sanitized_triple = re.sub(r"([^-]+)-([^-]+)-([^-.]+).*", r"\1-\2-\3",
-                              config.target_triple)
-    config.available_features.add('with_system_lib=%s' % (sanitized_triple,))
+# name: The name of this test suite.
+config.name = 'libc++'
+
+# suffixes: A list of file extensions to treat as test files.
+config.suffixes = ['.cpp']
+
+# test_source_root: The root path where tests are located.
+config.test_source_root = os.path.dirname(__file__)
+
+configuration = Configuration(lit_config, config)
+configuration.configure()
+config.test_format = configuration.get_test_format()