[lldb] Make the majority of the lit configuration values optional for the API tests

[JDevlieghere]

Summary

LIT uses a model where the test suite is configurable trough a lit.site.cfg file. Most of the time we use the lit.site.cfg which has a bunch of default values, generated by CMake, that match the current build configuration. However, nothing prevents you from running the test suite with a different configuration, either by overriding some of these values from the command line, or by passing a different lit.site.cfg.

Many of those configurationvalues are optional but they still need to be set because lit.cfg.py is accessing them directly. This patch changes the code to use getattr to return the attribute if it exists. This makes it possible to specify a minimal lit.site.cfg with only the mandatory/desired configuration values.

[labath]

lldb/test/API/lit.cfg.py

88–91

I don't think this name and the implementation are a good match. Given that the usages are a matching the name and not the implementation, maybe just change the implementation to hasattr(config, attr)?

That said, I'm not sure that is_configured("foo") is significantly better than hasattr(config, "foo"), so it might be better to just delete the function altogether.

[JDevlieghere]

lldb/test/API/lit.cfg.py

88–91

It does more than just hasattr(config, "foo"), is does hasattr(config, "foo") and config.foo. The option can exist but be set to False or None.

[JDevlieghere]

lldb/test/API/lit.cfg.py

88–91

I couldn't think of a good Python way of turning this into a bool instead of relying not the caller to do so, maybe you do?

[labath]

lldb/test/API/lit.cfg.py

88–91

It does more than just hasattr(config, "foo"), is does hasattr(config, "foo") and config.foo.

Ah, right. I did miss that distinction.

I couldn't think of a good Python way of turning this into a bool instead of relying not the caller to do so, maybe you do?

Well, there's always the option to say if hasattr(config, "foo") and config.foo: return True. However, it's not clear to me why this needs to handle so many cases. I can sort of see how treating a missing attribute and an attribute being set to None in the same way can be handy (for generated config files mainly), and I think it's reasonable for something called is_configured to return True there. But I don't think the same treatment should be extended to False or other False-ish thing -- someone had to explicitly set the attribute to this value, so it definitely /is/ configured.

For instances where this is needed (treating a False-ish value the same way as a non-existent value), I'd just spell this out in code:

if is_configured('llvm_use_sanitizer') and config.llvm_use_sanitizer:
  ...

lldb/test/API/lit.cfg.py

 
 # test_source_root: The root path where tests are located.
 # test_exec_root: The root path where tests should be run.
 config.test_source_root = os.path.dirname(__file__)
 config.test_exec_root = config.test_source_root
 
 
 def mkdir_p(path):
-    import errno
-    try:
-        os.makedirs(path)
-    except OSError as e:
-        if e.errno != errno.EEXIST:
-            raise
-    if not os.path.isdir(path):
-        raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
 
 
 def find_sanitizer_runtime(name):
   resource_dir = subprocess.check_output(
       [config.cmake_cxx_compiler,
        '-print-resource-dir']).decode('utf-8').strip()
   return os.path.join(resource_dir, 'lib', 'darwin', name)
 
except subprocess.CalledProcessError:
     # not copying the executable.
     os.remove(copied_python)
     return real_python
 
   # The copied Python works.
   return copied_python
 
 
+def is_configured(attr):
+  """Return the configuration attribute if it exists and None otherwise.
+
+  This allows us to check if the attribute exists before trying to access it."""
+  return getattr(config, attr, None)
+
+
+def delete_module_cache(path):
+  """Clean the module caches in the test build directory.
+
+  This is necessary in an incremental build whenever clang changes underneath,
+  so doing it once per lit.py invocation is close enough. """
+  if os.path.isdir(path):
+    print("Deleting module cache at %s." % path)
+    shutil.rmtree(path)
+
+if is_configured('llvm_use_sanitizer'):
   if 'Address' in config.llvm_use_sanitizer:
     config.environment['ASAN_OPTIONS'] = 'detect_stack_use_after_return=1'
     if 'Darwin' in config.host_os and 'x86' in config.host_triple:
       config.environment['DYLD_INSERT_LIBRARIES'] = find_sanitizer_runtime(
           'libclang_rt.asan_osx_dynamic.dylib')
 
   if 'Thread' in config.llvm_use_sanitizer:
     if 'Darwin' in config.host_os and 'x86' in config.host_triple:
       config.environment['DYLD_INSERT_LIBRARIES'] = find_sanitizer_runtime(
           'libclang_rt.tsan_osx_dynamic.dylib')
 
 if 'DYLD_INSERT_LIBRARIES' in config.environment and platform.system() == 'Darwin':
   config.python_executable = find_python_interpreter()
 
 # Shared library build of LLVM may require LD_LIBRARY_PATH or equivalent.
-if config.shared_libs:
+if is_configured('shared_libs'):
   for shlibpath_var in find_shlibpath_var():
     # In stand-alone build llvm_shlib_dir specifies LLDB's lib directory while
     # llvm_libs_dir specifies LLVM's lib directory.
     shlibpath = os.path.pathsep.join(
         (config.llvm_shlib_dir, config.llvm_libs_dir,
          config.environment.get(shlibpath_var, '')))
     config.environment[shlibpath_var] = shlibpath
   else:

 if lldb_repro_mode:
   lit_config.note("Running API tests in {} mode.".format(lldb_repro_mode))
   mkdir_p(config.lldb_reproducer_directory)
   if lldb_repro_mode == 'capture':
     config.available_features.add('lldb-repro-capture')
   elif lldb_repro_mode == 'replay':
     config.available_features.add('lldb-repro-replay')
 
-# Clean the module caches in the test build directory. This is necessary in an
-# incremental build whenever clang changes underneath, so doing it once per
-# lit.py invocation is close enough.
-for cachedir in [config.clang_module_cache, config.lldb_module_cache]:
-  if os.path.isdir(cachedir):
-    print("Deleting module cache at %s." % cachedir)
-    shutil.rmtree(cachedir)
-
 # Set a default per-test timeout of 10 minutes. Setting a timeout per test
 # requires that killProcessAndChildren() is supported on the platform and
 # lit complains if the value is set but it is not supported.
 supported, errormsg = lit_config.maxIndividualTestTimeIsSupported
 if supported:
   lit_config.maxIndividualTestTime = 600
 else:
   lit_config.warning("Could not set a default per-test timeout. " + errormsg)
 
 # Build dotest command.
 dotest_cmd = [os.path.join(config.lldb_src_root, 'test', 'API', 'dotest.py')]
-dotest_cmd += ['--arch', config.test_arch]
+
+if is_configured('dotest_args_str'):
   dotest_cmd.extend(config.dotest_args_str.split(';'))
 
 # Library path may be needed to locate just-built clang.
-if config.llvm_libs_dir:
+if is_configured('llvm_libs_dir'):
   dotest_cmd += ['--env', 'LLVM_LIBS_DIR=' + config.llvm_libs_dir]
 
 # Forward ASan-specific environment variables to tests, as a test may load an
 # ASan-ified dylib.
 for env_var in ('ASAN_OPTIONS', 'DYLD_INSERT_LIBRARIES'):
   if env_var in config.environment:
     dotest_cmd += ['--inferior-env', env_var + '=' + config.environment[env_var]]
 
-if config.lldb_build_directory:
+if is_configured('test_arch'):
+  dotest_cmd += ['--arch', config.test_arch]
+
+if is_configured('lldb_build_directory'):
   dotest_cmd += ['--build-dir', config.lldb_build_directory]
 
-if config.lldb_module_cache:
+if is_configured('lldb_module_cache'):
+  delete_module_cache(config.lldb_module_cache)
   dotest_cmd += ['--lldb-module-cache-dir', config.lldb_module_cache]
 
-if config.clang_module_cache:
+if is_configured('clang_module_cache'):
+  delete_module_cache(config.clang_module_cache)
   dotest_cmd += ['--clang-module-cache-dir', config.clang_module_cache]
 
-if config.lldb_executable:
+if is_configured('lldb_executable'):
   dotest_cmd += ['--executable', config.lldb_executable]
 
-if config.test_compiler:
+if is_configured('test_compiler'):
   dotest_cmd += ['--compiler', config.test_compiler]
 
-if config.dsymutil:
+if is_configured('dsymutil'):
   dotest_cmd += ['--dsymutil', config.dsymutil]
 
-if config.filecheck:
+if is_configured('filecheck'):
   dotest_cmd += ['--filecheck', config.filecheck]
 
-if config.yaml2obj:
+if is_configured('yaml2obj'):
   dotest_cmd += ['--yaml2obj', config.yaml2obj]
 
-if config.lldb_libs_dir:
+if is_configured('lldb_libs_dir'):
   dotest_cmd += ['--lldb-libs-dir', config.lldb_libs_dir]
 
 if 'lldb-repro-capture' in config.available_features or \
     'lldb-repro-replay' in config.available_features:
   dotest_cmd += ['--skip-category=lldb-vscode', '--skip-category=std-module']
 
-if config.enabled_plugins:
+if is_configured('enabled_plugins'):
   for plugin in config.enabled_plugins:
     dotest_cmd += ['--enable-plugin', plugin]
 
+if is_configured('dotest_lit_args_str'):
   # We don't want to force users passing arguments to lit to use `;` as a
   # separator. We use Python's simple lexical analyzer to turn the args into a
   # list. Pass there arguments last so they can override anything that was
   # already configured.
-if config.dotest_lit_args_str:
   dotest_cmd.extend(shlex.split(config.dotest_lit_args_str))
 
-
 # Load LLDB test format.
 sys.path.append(os.path.join(config.lldb_src_root, "test", "API"))
 import lldbtest
 
 # testFormat: The test format to use to interpret tests.
 config.test_format = lldbtest.LLDBTest(dotest_cmd)

lldb/test/API/lldbtest.py

def getTestsInDirectory(self, testSuite, path_in_suite, litConfig,
                 if ext in localConfig.suffixes:
                     yield lit.Test.Test(testSuite, path_in_suite +
                                         (filename, ), localConfig)
 
     def execute(self, test, litConfig):
         if litConfig.noExecute:
             return lit.Test.PASS, ''
 
-        if not test.config.lldb_enable_python:
+        if not getattr(test.config, 'lldb_enable_python', False):
             return (lit.Test.UNSUPPORTED, 'Python module disabled')
 
         if test.config.unsupported:
             return (lit.Test.UNSUPPORTED, 'Test is unsupported')
 
         testPath, testFile = os.path.split(test.getSourcePath())
 
         # The Python used to run lit can be different from the Python LLDB was