[libcxx] Add better support for custom test runners.

[danalbert]

Summary

I finally got around to merging the many, many changes to lit.cfg into
Android's libc++. This patch makes it simpler to actually use a custom
configuration and test format.

First, I've factored out _build, _run, and _clean methods from
_execute_test, since these are the likely parts that will need to be
overridden. This is likely a first step in the work jroelofs has been
doing with improving cross-compiling test execution.

Second, I've added a configuration_variant to the config. This
entry, if present, is a string that forms the prefix of the class that
is to be used to configure the test runner. For example, Android sets
config.configuration_variant = 'Android', and this causes an object
of type AndroidConfiguration to be constructed.

As an example of how this will be used, see:
https://android-review.googlesource.com/#/c/116022/

[EricWF]

test/lit.cfg

204 ↗

(On Diff #16530)

exec_path is always created so it should always be removed. If creating the file doesn't belong to a customization point then neither should cleaning it up.

Small nit: Will there ever be a case we want _clean(...) to throw? Should exception handling be the responsibility of _clean(...)?

529 ↗

(On Diff #16530)

Is there a prettier way to right this line? Just so I understand globals()['%sConfiguration' % cfg_variant] looks up and evaluates to a type with the name %sConfiguration?

[danalbert]

test/lit.cfg

204 ↗

(On Diff #16530)

The build step is the one that creates the file, and that _is_ customizable. Remote test executors need to customize the cleanup step because they need to remove the output from both the compilation location and the run location.

Yeah, I'm thinking _clean() worries about the exceptions, since the action taken might vary based on the internals (adb fails often, so probably just retry, whereas os.remove() will probably only fail for bad permissions, in which case just ignore).

529 ↗

(On Diff #16530)

I believe getattr(__module__, '%sConfiguration' % cfg_variant)(lit_config, config) would work too, but that isn't all that much better imo. At some point I'l like to teach LIT to add the directory of lit.cfg to PYTHONPATH so we can split some things out into different modules. Then this could be done in lit.site.cfg as from android import AndroidConfiguration as Configuration and lit.cfg would become

try:
    configuration = Configuration(lit_config, config)
except NameError:
    configuration = DefaultConfiguration(lit_config, config)

[EricWF]

test/lit.cfg

204 ↗

(On Diff #16530)

Isn't the file created by exec_file = tempfile.NamedTemporaryFile(suffix="exe", delete=False) on line 170? The _build step just overwrites it.

[danalbert]

test/lit.cfg

204 ↗

(On Diff #16530)

Still potentially have to deal with cleanup up remote paths. Also, in Android we generate a .o before linking (this is what our build system does, and I want to test our builds exactly).

[EricWF]

test/lit.cfg

204 ↗

(On Diff #16530)

Sure, I'm not saying scrap _clean as a customization point. I'm just wondering if we should always try and clean up exec_path since we always create it.

[danalbert]

test/lit.cfg

204 ↗

(On Diff #16530)

Eh. If you're overriding _clean(), it's your responsibility to call super if you want it to do the same things.

[EricWF]

LGTM modulo requested comments.

test/lit.cfg

204 ↗

(On Diff #16530)

I'll buy that. Can you add a comment so that is clear?

529 ↗

(On Diff #16530)

Please add a comment explaining this line.

[danalbert]

Address review comments.

[danalbert]

Closed by commit rL222698 (authored by @danalbert).

libcxx/trunk/test/lit.cfg

def execute_command(self, command, in_dir=None):
         kwargs = {
             'stdin' :subprocess.PIPE,
             'stdout':subprocess.PIPE,
             'stderr':subprocess.PIPE,
         }
         if in_dir:
             kwargs['cwd'] = in_dir
         p = subprocess.Popen(command, **kwargs)
-        out,err = p.communicate()
+        out, err = p.communicate()
         exitCode = p.wait()
 
         # Detect Ctrl-C in subprocess.
         if exitCode == -signal.SIGINT:
             raise KeyboardInterrupt
 
         return out, err, exitCode
 
def _execute(self, test, lit_config):
         if unsupported_features:
             return (lit.Test.UNSUPPORTED,
                     "Test is unsupported with the following features: %s" % (
                        ', '.join(unsupported_features),))
 
         # Evaluate the test.
         return self._evaluate_test(test, use_verify, lit_config)
 
+    def _build(self, exec_path, source_path, compile_only=False,
+               use_verify=False):
+        cmd = [self.cxx_under_test, '-o', exec_path,
+               source_path] + self.cpp_flags
+
+        if compile_only:
+            cmd += ['-c']
+        else:
+            cmd += self.ld_flags
+
+        if use_verify:
+            cmd += ['-Xclang', '-verify']
+
+        out, err, rc = self.execute_command(cmd)
+        return cmd, out, err, rc
+
+    def _clean(self, exec_path):
+        os.remove(exec_path)
+
+    def _run(self, exec_path, lit_config, in_dir=None):
+        cmd = []
+        if self.exec_env:
+            cmd.append('env')
+            cmd.extend('%s=%s' % (name, value)
+                       for name,value in self.exec_env.items())
+        cmd.append(exec_path)
+        if lit_config.useValgrind:
+            cmd = lit_config.valgrindArgs + cmd
+        out, err, exitCode = self.execute_command(cmd, in_dir)
+        return cmd, out, err, exitCode
+
     def _evaluate_test(self, test, use_verify, lit_config):
         name = test.path_in_suite[-1]
         source_path = test.getSourcePath()
         source_dir = os.path.dirname(source_path)
 
         # Check what kind of test this is.
         assert name.endswith('.pass.cpp') or name.endswith('.fail.cpp')
         expected_compile_fail = name.endswith('.fail.cpp')
 
         # If this is a compile (failure) test, build it and check for failure.
         if expected_compile_fail:
-            cmd = [self.cxx_under_test, '-c',
-                   '-o', '/dev/null', source_path] + self.cpp_flags
-            expected_rc = 1
-            if use_verify:
-                cmd += ['-Xclang', '-verify']
-                expected_rc = 0
-            out, err, rc = self.execute_command(cmd)
+            cmd, out, err, rc = self._build('/dev/null', source_path,
+                                            compile_only=True,
+                                            use_verify=use_verify)
+            expected_rc = 0 if use_verify else 1
             if rc == expected_rc:
                 return lit.Test.PASS, ""
             else:
                 report = """Command: %s\n""" % ' '.join(["'%s'" % a
                                                          for a in cmd])
                 report += """Exit Code: %d\n""" % rc
                 if out:
                     report += """Standard Output:\n--\n%s--""" % out
                 if err:
                     report += """Standard Error:\n--\n%s--""" % err
                 report += "\n\nExpected compilation to fail!"
                 return lit.Test.FAIL, report
         else:
             exec_file = tempfile.NamedTemporaryFile(suffix="exe", delete=False)
             exec_path = exec_file.name
             exec_file.close()
 
             try:
-                compile_cmd = [self.cxx_under_test, '-o', exec_path,
-                       source_path] + self.cpp_flags + self.ld_flags
-                cmd = compile_cmd
-                out, err, exitCode = self.execute_command(cmd)
-                if exitCode != 0:
+                cmd, out, err, rc = self._build(exec_path, source_path)
+                compile_cmd = cmd
+                if rc != 0:
                     report = """Command: %s\n""" % ' '.join(["'%s'" % a
                                                              for a in cmd])
-                    report += """Exit Code: %d\n""" % exitCode
+                    report += """Exit Code: %d\n""" % rc
                     if out:
                         report += """Standard Output:\n--\n%s--""" % out
                     if err:
                         report += """Standard Error:\n--\n%s--""" % err
                     report += "\n\nCompilation failed unexpectedly!"
                     return lit.Test.FAIL, report
 
-                cmd = []
-                if self.exec_env:
-                    cmd.append('env')
-                    cmd.extend('%s=%s' % (name, value)
-                               for name,value in self.exec_env.items())
-                cmd.append(exec_path)
-                if lit_config.useValgrind:
-                    cmd = lit_config.valgrindArgs + cmd
-                out, err, exitCode = self.execute_command(cmd, source_dir)
-                if exitCode != 0:
+                cmd, out, err, rc = self._run(exec_path, lit_config,
+                                              source_dir)
+                if rc != 0:
                     report = """Compiled With: %s\n""" % \
                         ' '.join(["'%s'" % a for a in compile_cmd])
                     report += """Command: %s\n""" % \
                         ' '.join(["'%s'" % a for a in cmd])
-                    report += """Exit Code: %d\n""" % exitCode
+                    report += """Exit Code: %d\n""" % rc
                     if out:
                         report += """Standard Output:\n--\n%s--""" % out
                     if err:
                         report += """Standard Error:\n--\n%s--""" % err
                     report += "\n\nCompiled test failed unexpectedly!"
                     return lit.Test.FAIL, report
             finally:
                 try:
-                    os.remove(exec_path)
+                    # Note that cleanup of exec_file happens in `_clean()`. If
+                    # you override this, cleanup is your reponsibility.
+                    self._clean(exec_path)
                 except:
                     pass
         return lit.Test.PASS, ""
 
 
 class Configuration(object):
     def __init__(self, lit_config, config):
         self.lit_config = lit_config

 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)
+cfg_variant = getattr(config, 'configuration_variant', '')
+if cfg_variant:
+    print 'Using configuration variant: %s' % cfg_variant
+
+# Construct an object of the type named `<VARIANT>Configuration`.
+configuration = globals()['%sConfiguration' % cfg_variant](lit_config, config)
 configuration.configure()
 config.test_format = configuration.get_test_format()