Add remote testing support to the lit config.

[jroelofs]

Summary

This doesn't claim to support ShTest tests yet, but I've added in a hacked-together wrapper script remoteRun.py as one possible solution. I'm still not certain how to orchestrate executable copy-in for those cases.

Here I've also punted on what to do about test data files. The affected tests are:

libc++ :: std/input.output/file.streams/fstreams/filebuf.virtuals/pbackfail.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/filebuf.virtuals/underflow.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.assign/member_swap.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.assign/move_assign.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.assign/nonmember_swap.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.cons/move.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.cons/pointer.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.cons/string.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.members/close.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.members/open_pointer.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.members/open_string.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.members/rdbuf.pass.cpp
libc++ :: std/localization/locales/locale.convenience/conversions/conversions.buffer/pbackfail.pass.cpp
libc++ :: std/localization/locales/locale.convenience/conversions/conversions.buffer/underflow.pass.cpp

I tested this on my mac with an SSHExecutor that connects to localhost, and copies executables into /tmp to get a feel for the overhead. That run took ~414s, whereas a normal one takes about half that at ~204s.

This isn't quite ready for commit, but it's a lot closer... There's still a few things I'd like to pick your brains on, @danalbert and @EricWF.

[jroelofs]

remoteRun.py

8 ↗

(On Diff #19239)

I considered passing this in as the first argument, and using eval. Better ideas?

[jroelofs]

libcxx/test/config.py

152

I forgot a

else:
    self.target_executor = None

here

libcxx/test/format.py

116–118

I kept these cases separate to avoid clashing with your configuration, @danalbert. It might be better to drop the else side of this, and have target_executor be a LocalExecutor instead of None when doing local testing. I'll save that decision for later to minimize invasiveness.

libcxx/test/remote.py

6 ↗

(On Diff #19239)

I couldn't figure out how to get the include of lit.util to work here when using the wrapper script. It would be better to include that than to duplicate the code here.

lit.site.cfg.in

21

This part is obviously not supposed to go upstream, but I put it in the patch to show how to use it.

I think there are at least two common patterns that might be worth exposing through cmake:

config.target_executor = "SSHExecutor('username', 'hostname')"

config.target_executor = "PrefixExecutor('/path/to/run/script', LocalExecutor())"

Thoughts? Should we just pass the whole blob in on the cmake configure command line, or do something more sophisticated?

[jroelofs]

Fix the LocalExecutor

[jroelofs]

Set target_executor to None when there isn't one.

[jroelofs]

Another thing I haven't attempted to deal with in this patch is that there are a lot of assumptions that $host==$target for test parameter discovery. Need to clean those up at a later point.

test/libcxx/test/config.py

270 ↗

(On Diff #19418)

I don't know where this came from. Bad merge maybe?

test/libcxx/test/remote.py

202 ↗

(On Diff #19418)

This one is already deprecated. Going to remove it as soon as the lit timeouts patch goes in.

318 ↗

(On Diff #19418)

This class screams that it wants reflection... I just don't know how to do that.

test/lit.site.cfg.in

21 ↗

(On Diff #19418)

Again, I don't want to upstream this part... it's just an example.

test/remoteRun.py

1 ↗

(On Diff #19418)

This file isn't used yet, but we could use it (or something like it) to to ShTest tests.

[danalbert]

I'm happy this is mostly unintrusive. I was expecting a bigger mess :)

If you decide not to go with my suggestions on L122, I'll probably have more to say about the executors themselves, otherwise I'll wait for the next revision since that will completely change them.

libcxx/test/format.py

97

No reason for this to be an argument, it's a member.

116–118

and have target_executor be a LocalExecutor instead of None when doing local testing

Was going to suggest exactly that. Should keep things cleaner and make sure neither side bit rots (because there will only be one side).

test/libcxx/test/config.py

14 ↗

(On Diff #19418)

Should avoid wildcard imports.

123 ↗

(On Diff #19418)

Ewwwwwww eval? Unless there's a really good reason to use an expression that can be evaluated over just naming a type, I think this should be:

mod_path, _, executor = exec_str.rpartition('.')
mod = importlib.import_module(mod_path)
self.target_executor = getattr(mod, executor)()

Where exec_str is the fully qualified type name (i.e. libcxx.test.remote.LocalExecutor). Then the wildcard import and the ugly eval can go away.

270 ↗

(On Diff #19418)

Yes. ericwf added this the other day because it was something that went missing from the libcxxabi config (one test has a // REQUIRES: linux2)

test/libcxx/test/format.py

122 ↗

(On Diff #19418)

I think this part of the logic could probably just be pushed down in to the executor. The parameters to exc.run() would become exec_path, source_path, env={}, valgrind_mess. exc.copy_in() becomes unnecessary, be the executor can decide for itself what should be copied. Same goes for exc.remote_from_local_dir() and exc.remote_from_local_file(), and exc.delete_remote(). This entire added block (and some the the existing code above) would become:

out, err, rc = executor.run(exec_path, source_path, self.exec_env, valgrind_mess)

That should also help with replacing target_executor = None with executor = LocalExecutor(), as each method of LocalExecutor would become a no-op.

This will probably cause some duplication between the various remote executors, so it might make sense to have a common base for all of them that factors that out.

149 ↗

(On Diff #19418)

This should stay as it was. The target executor should clean up its own mess, but not the mess from the higher level (this is true with and without my suggestion at L122).

test/libcxx/test/remote.py

6 ↗

(On Diff #19418)

What makes import hard here?

193 ↗

(On Diff #19418)

This would be a nice way to factor out that cruft. Good thinking.

212 ↗

(On Diff #19418)

Don't align assignments (you haven't matched all of them anyway).

223 ↗

(On Diff #19418)

Your naming is inconsistent. self.scpCommand but self.remote_temp_files. I prefer PEP8 stlye (with underscores), but LIT already chose the anti-Python style, so let's try to stick with that. (I'm guilty of this crime too.)

255 ↗

(On Diff #19418)

if local_file not in self.filemap:

306 ↗

(On Diff #19418)

Use != for comparing strings rather than is not. is not should be used to test that two things are exactly the same object.

318 ↗

(On Diff #19418)

You probably want something along the lines of http://stackoverflow.com/a/3467879/632035

Should make the whole class unnecessary.

[jroelofs]

test/libcxx/test/format.py

149 ↗

(On Diff #19418)

Good point.

test/libcxx/test/remote.py

6 ↗

(On Diff #19418)

when imported from remoteRun.py, the imports here don't work because we do funny business somewhere in the lit.cfg that adds the llvm imports to some path that python uses to search from.

I'll add the imports, drop this duped code, and drop the remoteRun.py. Can add that back and fix the import problem later when we try to get ShTest going.

212 ↗

(On Diff #19418)

ok.

I really wish phabricator used a monospaced font for diffs.... haven't been able to find a setting that changes that.

223 ↗

(On Diff #19418)

as in camelCase for vars, and underscore_separated_functions() ? Ok.

[danalbert]

test/libcxx/test/remote.py

223 ↗

(On Diff #19418)

Actually, looking at lit's Test.py, it looks like the format is underscores for variables, camel-case for functions.

[jroelofs]

Address most of @danalbert's comments. There are still a few remaining that I need to take care of though.

[jroelofs]

test/libcxx/test/config.py

126 ↗

(On Diff #19537)

Drop this line.

123 ↗

(On Diff #19418)

This means that the executors cannot be configured or chained via their constructor arguments, which is something I think is necessary to make, say, SSHExecutors work.... unless I start parsing the args, which leads to just re-inventing eval.

test/libcxx/test/format.py

122 ↗

(On Diff #19537)

Still need to push most of this into target_executor.run

test/libcxx/test/remote.py

115 ↗

(On Diff #19537)

!=

283 ↗

(On Diff #19537)

I think this is much better, but it's also extremely complicated now. :/

[danalbert]

test/libcxx/test/config.py

123 ↗

(On Diff #19418)

Ah, right. That makes sense. Carry on.

[jroelofs]

Address the rest of @danalbert's comments

[jroelofs]

Add TargetInfo class for determining things like locale support and platform name.

[jroelofs]

Uploaded a diff from the wrong dir. Sorry for the spam.

[jroelofs]

Remove something I was using for testing an idea. It didn't pan out.

[jroelofs]

Make capitalization consistent.

[EricWF]

Drive by comments.

test/libcxx/test/config.py

120 ↗

(On Diff #19632)

Isn't this always set to the default by CMake?

122 ↗

(On Diff #19632)

Remove this. Use the lit_config reporting methods.

test/libcxx/test/format.py

78 ↗

(On Diff #19632)

Isn't this always true? Can we detect wether we have a local executor for the time being?

test/libcxx/test/remote.py

11–21 ↗

(On Diff #19632)

I would throw a NotImplemented or similar error from these "abstract" methods

52 ↗

(On Diff #19632)

I would probably throw from these abstract methods too if they shouldn't be called.

[jroelofs]

Address @EricWF's driveby comments.

[danalbert]

Looking better. At first glance, I think there's some more cleanup that can be done in remote.py, but I'll finish the review first thing tomorrow.

BTW, I like the TargetInfo bit. It looks like that change is actually cleanly separates from the rest of this. That part LGTM, so go ahead and submit that portion as its own commit.

test/libcxx/test/config.py

1 ↗

(On Diff #19722)

Unused.

116 ↗

(On Diff #19722)

Now that even the base case is a LocalExecutor, I think the target_ prefix of this is misleading.

120 ↗

(On Diff #19722)

Default should be "LocalExecutor". Then the if/else can disappear.

128 ↗

(On Diff #19722)

useValgrind should just go away. If the user wants valgrind, they can add the ValgrindExecutor themselves.

592 ↗

(On Diff #19722)

Use parens rather than the \

And keep the not with the thing it's negating.

test/libcxx/test/format.py

89 ↗

(On Diff #19722)

No reason to pass this as an argument. Just refer to it as self.target_executor in _evaluate_pass_test.

118 ↗

(On Diff #19722)

The above three lines are just env = self.exec_env.

118 ↗

(On Diff #19722)

Ignore the above. I traced it back and found that the default self.exec_env is {}, not None. Phabricator isn't letting me delete the comment though...

[danalbert]

Here's my take on remote.py: https://gist.github.com/DanAlbert/a45ac580c3fc61917856

This patch no longer applies cleanly to ToT, and I don't know what revision it was generate against, so I wasn't able to apply this to my working tree to test it. Once it's rebased I'll verify.

[jroelofs]

Rebase & use @danalbert's remote.py + tracing.py almost verbatim.

[jroelofs]

Address (what I think are) the rest of @danalbert's comments on this patch.

[jroelofs]

I split the TargetInfo part into its own patch: http://reviews.llvm.org/D7601

This patch will need a slight rebase after that goes in.

libcxx/test/config.py

474

I think this needs to stay as lowercase, actually.

test/libcxx/test/config.py

120 ↗

(On Diff #19632)

Oh yeah.

122 ↗

(On Diff #19632)

oops

128 ↗

(On Diff #19722)

This suggestion is a buildbot-breaking change. Can clean this up after any zorg bots that care about valgrind are moved over to using the new cmake flag.

[jroelofs]

Fix the race condition in SSHExecutor, and leave a note saying to be careful.

[jroelofs]

Didn't get all the files in the diff. Sorry for the spam.

[jroelofs]

libcxx/test/config.py

608–609

I think this is evidence that I need to rebase the patch. The target_info stuff already went in.

libcxx/test/tracing.py

6 ↗

(On Diff #19915)

delete this line.

[danalbert]

LGTM with the one nit.

Thanks a lot for taking care of this. Should greatly simplify my test setup.

libcxx/test/config.py

125

This isn't inferred, this is explicit. The else path is inferred.

[jroelofs]

And with much rejoicing, there was r230592.

CMakeLists.txt

if (LIT_EXECUTABLE)
   pythonize_bool(LIBCXX_ENABLE_EXCEPTIONS)
   pythonize_bool(LIBCXX_ENABLE_RTTI)
   pythonize_bool(LIBCXX_ENABLE_SHARED)
   pythonize_bool(LIBCXX_BUILD_32_BITS)
   pythonize_bool(LIBCXX_ENABLE_THREADS)
   pythonize_bool(LIBCXX_ENABLE_MONOTONIC_CLOCK)
   set(LIBCXX_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
       "TargetInfo to use when setting up test environment.")
+  set(LIBCXX_EXECUTOR "None" CACHE STRING
+      "Executor to use when running tests.")
 
   set(AUTO_GEN_COMMENT "## Autogenerated by libcxx configuration.\n# Do not edit!")
 
   configure_file(
     ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.in
     ${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg
     @ONLY)
 

libcxx/test/config.py

 import importlib
 import locale
 import os
 import platform
 import pkgutil
 import re
 import shlex
 import sys
 
 import lit.Test  # pylint: disable=import-error,no-name-in-module
 import lit.util  # pylint: disable=import-error,no-name-in-module
 
 from libcxx.test.format import LibcxxTestFormat
 from libcxx.compiler import CXXCompiler
-
+from libcxx.test.executor import *
+from libcxx.test.tracing import *
 
 def loadSiteConfig(lit_config, config, param_name, env_name):
     # We haven't loaded the site specific configuration (the user is
     # probably trying to run on a test file directly, and either the site
     # configuration hasn't been created by the build system, or we are in an
     # out-of-tree build situation).
     site_cfg = lit_config.params.get(param_name,
                                      os.environ.get(env_name))
def get_lit_bool(self, name, default=None):
         if conf.lower() in ('1', 'true'):
             return True
         if conf.lower() in ('', '0', 'false'):
             return False
         self.lit_config.fatal(
             "parameter '{}' should be true or false".format(name))
 
     def configure(self):
+        self.configure_executor()
         self.configure_target_info()
         self.configure_cxx()
         self.configure_triple()
         self.configure_src_root()
         self.configure_obj_root()
         self.configure_cxx_library_root()
         self.configure_use_system_cxx_lib()
         self.configure_use_clang_verify()
def print_config_info(self):
         self.lit_config.note('Using compile flags: %s'
                              % self.cxx.compile_flags)
         self.lit_config.note('Using link flags: %s' % self.cxx.link_flags)
         # Print as list to prevent "set([...])" from being printed.
         self.lit_config.note('Using available_features: %s' %
                              list(self.config.available_features))
         self.lit_config.note('Using environment: %r' % self.env)
 
+    def get_test_format(self):
+        return LibcxxTestFormat(
+            self.cxx,
+            self.use_clang_verify,
+            self.execute_external,
+            self.executor,
+            exec_env=self.env)
+
+    def configure_executor(self):
+        exec_str = self.get_lit_conf('executor', "None")
+        te = eval(exec_str)
+        if te:
+            self.lit_config.note("inferred executor as: %r" % exec_str)
+            if self.lit_config.useValgrind:
+                # We have no way of knowing where in the chain the
+                # ValgrindExecutor is supposed to go. It is likely
+                # that the user wants it at the end, but we have no
+                # way of getting at that easily.
+                selt.lit_config.fatal("Cannot infer how to create a Valgrind "
+                                      " executor.")
+        else:
+            te = LocalExecutor()
+            if self.lit_config.useValgrind:
+                te = ValgrindExecutor(self.lit_config.valgrindArgs, te)
+        self.executor = te
+
     def configure_target_info(self):
         default = "libcxx.test.target_info.LocalTI"
         info_str = self.get_lit_conf('target_info', default)
         mod_path, _, info = info_str.rpartition('.')
         mod = importlib.import_module(mod_path)
         self.target_info = getattr(mod, info)()
         if info_str != default:
             self.lit_config.note("inferred target_info as: %r" % info_str)
 
-    def get_test_format(self):
-        return LibcxxTestFormat(
-            self.cxx,
-            self.use_clang_verify,
-            self.execute_external,
-            exec_env=self.env)
-
     def configure_cxx(self):
         # Gather various compiler parameters.
         cxx = self.get_lit_conf('cxx_under_test')
 
         # If no specific cxx_under_test was given, attempt to infer it as
         # clang++.
         if cxx is None:
             clangxx = lit.util.which('clang++',
                                      self.config.environment['PATH'])
             if clangxx:
                 cxx = clangxx
                 self.lit_config.note(
                     "inferred cxx_under_test as: %r" % cxx)
def configure_default_compile_flags(self):
         # default given in lit.site.cfg is not present. If default is not
         # present then force c++11.
         std = self.get_lit_conf('std', 'c++11')
         self.cxx.compile_flags += ['-std={0}'.format(std)]
         self.config.available_features.add(std)
         # Configure include paths
         self.cxx.compile_flags += ['-nostdinc++']
         self.configure_compile_flags_header_includes()
-        if sys.platform.startswith('linux'):
+        if self.target_info.platform() == 'linux':
             self.cxx.compile_flags += ['-D__STDC_FORMAT_MACROS',
                                        '-D__STDC_LIMIT_MACROS',
                                        '-D__STDC_CONSTANT_MACROS']
         # Configure feature flags.
         self.configure_compile_flags_exceptions()
         self.configure_compile_flags_rtti()
         enable_32bit = self.get_lit_bool('enable_32bit', False)
         if enable_32bit:
def configure_link_flags_abi_library(self):
         else:
             self.lit_config.fatal(
                 'C++ ABI setting %s unsupported for tests' % cxx_abi)
 
     def configure_extra_library_flags(self):
         enable_threads = self.get_lit_bool('enable_threads', True)
         llvm_unwinder = self.get_lit_bool('llvm_unwinder', False)
         target_platform = self.target_info.platform()
         if target_platform == 'darwin':
             self.cxx.link_flags += ['-lSystem']
         elif target_platform == 'linux':
             if not llvm_unwinder:
                 self.cxx.link_flags += ['-lgcc_eh']
             self.cxx.link_flags += ['-lc', '-lm']
             if enable_threads:
                 self.cxx.link_flags += ['-lpthread']
             self.cxx.link_flags += ['-lrt']
def configure_triple(self):
             # Example: x86_64-apple-darwin14.0.0 -> x86_64-apple-darwin14
             # The 5th group handles triples greater than 3 parts
             # (ex x86_64-pc-linux-gnu).
             target_triple = re.sub(r'([^-]+)-([^-]+)-([^.]+)([^-]*)(.*)',
                                    r'\1-\2-\3\5', target_triple)
             # linux-gnu is needed in the triple to properly identify linuxes
             # that use GLIBC. Handle redhat and opensuse triples as special
             # cases and append the missing `-gnu` portion.
-            if target_triple.endswith('redhat-linux') or \
-               target_triple.endswith('suse-linux'):
+            if (target_triple.endswith('redhat-linux') or
+                target_triple.endswith('suse-linux')):
                 target_triple += '-gnu'
             self.config.target_triple = target_triple
             self.lit_config.note(
                 "inferred target_triple as: %r" % self.config.target_triple)
 
     def configure_env(self):
-        if sys.platform == 'darwin' and not self.use_system_cxx_lib:
+        if (self.target_info.platform() == 'darwin' and
+            not self.use_system_cxx_lib):
             libcxx_library = self.get_lit_conf('libcxx_library')
             if libcxx_library:
                 cxx_library_root = os.path.dirname(libcxx_library)
             else:
                 cxx_library_root = self.cxx_library_root
             if cxx_library_root:
                 self.env['DYLD_LIBRARY_PATH'] = cxx_library_root

libcxx/test/format.py

class LibcxxTestFormat(object):
 
     Tests fall into two categories:
       FOO.pass.cpp - Executable test which should compile, run, and exit with
                      code 0.
       FOO.fail.cpp - Negative test case which is expected to fail compilation.
       FOO.sh.cpp   - A test that uses LIT's ShTest format.
     """
 
-    def __init__(self, cxx, use_verify_for_fail, execute_external, exec_env):
+    def __init__(self, cxx, use_verify_for_fail, execute_external,
+                 executor, exec_env):
         self.cxx = cxx
         self.use_verify_for_fail = use_verify_for_fail
         self.execute_external = execute_external
+        self.executor = executor
         self.exec_env = dict(exec_env)
 
     # TODO: Move this into lit's FileBasedTest
     def getTestsInDirectory(self, testSuite, path_in_suite,
                             litConfig, localConfig):
         source_path = testSuite.getSourcePath(path_in_suite)
         for filename in os.listdir(source_path):
             # Ignore dot files and excluded tests.
def _execute(self, test, lit_config):
         # res is not an instance of lit.test.Result. Expand res into its parts.
         script, tmpBase, execDir = res
         # Check that we don't have run lines on tests that don't support them.
         if not is_sh_test and len(script) != 0:
             lit_config.fatal('Unsupported RUN line found in test %s' % name)
 
         # Dispatch the test based on its suffix.
         if is_sh_test:
+            if self.executor:
+                # We can't run ShTest tests with a executor yet.
+                # For now, bail on trying to run them
+                return lit.Test.UNSUPPORTED, 'ShTest format not yet supported'
             return lit.TestRunner._runShTest(test, lit_config,
                                              self.execute_external, script,
                                              tmpBase, execDir)
         elif is_fail_test:
             return self._evaluate_fail_test(test)
         elif is_pass_test:
             return self._evaluate_pass_test(test, tmpBase, execDir, lit_config)
         else:
             # No other test type is supported
             assert False
 
     def _clean(self, exec_path):  # pylint: disable=no-self-use
         libcxx.util.cleanFile(exec_path)
 
     def _evaluate_pass_test(self, test, tmpBase, execDir, lit_config):
         source_path = test.getSourcePath()
         exec_path = tmpBase + '.exe'
         object_path = tmpBase + '.o'
         # Create the output directory if it does not already exist.
         lit.util.mkdir_p(os.path.dirname(tmpBase))
         try:
             # Compile the test
             cmd, out, err, rc = self.cxx.compileLinkTwoSteps(
                 source_path, out=exec_path, object_file=object_path,
                 cwd=execDir)
             compile_cmd = cmd
             if rc != 0:
                 report = libcxx.util.makeReport(cmd, out, err, rc)
                 report += "Compilation failed unexpectedly!"
                 return lit.Test.FAIL, report
             # Run the test
-            cmd = []
+            local_cwd = os.path.dirname(source_path)
+            env = None
             if self.exec_env:
-                cmd += ['env']
-                cmd += ['%s=%s' % (k, v) for k, v in self.exec_env.items()]
-            if lit_config.useValgrind:
-                cmd = lit_config.valgrindArgs + cmd
-            cmd += [exec_path]
-            out, err, rc = lit.util.executeCommand(
-                cmd, cwd=os.path.dirname(source_path))
+                env = self.exec_env
+            out, err, rc = self.executor.run(exec_path, [exec_path],
+                                             local_cwd, env)
             if rc != 0:
                 report = libcxx.util.makeReport(cmd, out, err, rc)
                 report = "Compiled With: %s\n%s" % (compile_cmd, report)
                 report += "Compiled test failed unexpectedly!"
                 return lit.Test.FAIL, report
             return lit.Test.PASS, ''
         finally:
             # Note that cleanup of exec_file happens in `_clean()`. If you

lit.site.cfg.in

 config.cxx_abi                  = "@LIBCXX_CXX_ABI_LIBNAME@"
 config.use_sanitizer            = "@LLVM_USE_SANITIZER@"
 config.abi_library_path         = "@LIBCXX_CXX_ABI_LIBRARY_PATH@"
 config.configuration_variant    = "@LIBCXX_LIT_VARIANT@"
 config.target_triple            = "@LIBCXX_TARGET_TRIPLE@"
 config.sysroot                  = "@LIBCXX_SYSROOT@"
 config.gcc_toolchain            = "@LIBCXX_GCC_TOOLCHAIN@"
 config.target_info              = "@LIBCXX_TARGET_INFO@"
+config.executor                 = "@LIBCXX_EXECUTOR@"
 
 # Let the main config do the real work.
 lit_config.load_config(config, "@LIBCXX_SOURCE_DIR@/test/lit.cfg")