Copy data files to the remote runner.

[danalbert]

Summary

The data files for any given test will be in the same directory as the
source with a file name that matches *.dat. To make these available to
tests running remotely (such as over adb or ssh), copy them into the
test's remote working directory.

Note that we will perform more copies than we actually need. The data
files in the directory may only be used by one of the tests, but will
be copied for all tests in the same directory.

This patch also moves the remote test binary into the working
directory (previously it was only invoked from the working directory
rather than existing in it).

[EricWF]

LGTM as long as the inline comment is addressed.

Is there rational for just not requiring that the source tree be checked out on the target already? I haven't looked to hard at this executor stuff but that seems like it might solve some of these problems.

test/libcxx/test/executor.py

116 ↗

(On Diff #21381)

Is is possible that two tests could be run in parallel and try and use the same path?

[jroelofs]

test/libcxx/test/executor.py

113 ↗

(On Diff #21381)

This feels like it's at the wrong level of abstraction. Shouldn't the "which files do I copy in" part be determined by the caller?

116 ↗

(On Diff #21381)

As long as the implementation of remote_temp_dir provides uniquing between the processes, then this will be fine. For the SSHExecutor, that is implemented by 'mktemp -d', which DTRT. I don't remember off the top of my head what the LocalExecutor does. It's definitely worth adding another "parallelism gotcha" note to the Executor interface covering this situation.

[danalbert]

Make determining file dependencies the caller's responsibility.

test/libcxx/test/executor.py

113 ↗

(On Diff #21381)

Good point. New version moves this responsibility up.

[jroelofs]

LGTM with a couple of small tweaks.

test/libcxx/test/executor.py

16 ↗

(On Diff #21628)

Need to update the docs to reference the new parameter.

116 ↗

(On Diff #21628)

Should probably

s/libcxx_test/libcxx_test\.exe/

... even though Linux and Darwin don't care about the extension. It makes the intent slightly clearer.

Perhaps this should go in a separate commit? This line LGTM if you want to commit that now.

127 ↗

(On Diff #21628)

It would be better if this were rolled in with the above call to copy_in, that way the copy could later be implemented with compression/decompression steps at either end.

test/libcxx/test/format.py

118 ↗

(On Diff #21628)

I would much rather these be explicitly called out in the test's metadata, but this is fine for now. Perhaps add a TODO?

[danalbert]

Address review comments

[jroelofs]

LGTM

test/libcxx/test/executor.py

128 ↗

(On Diff #21643)

sorry, what I meant was:

to_copy = ([exe_path], [target_exe])
if file_deps is not None:
    dev_paths = [os.path.join(target_cwd, os.path.basename(f))
                 for f in file_deps]
    to_copy[0] += file_deps
    to_copy[1] += dev_paths
self.copy_in(to_copy[0], to_copy[1])

[danalbert]

Bundle more files

libcxx/trunk/test/libcxx/test/executor.py

 import os
 
 from libcxx.test import tracing
 
 from lit.util import executeCommand  # pylint: disable=import-error
 
 
 class Executor(object):
-    def run(self, exe_path, cmd, local_cwd, env=None):
+    def run(self, exe_path, cmd, local_cwd, file_deps=None, env=None):
         """Execute a command.
             Be very careful not to change shared state in this function.
             Executor objects are shared between python processes in `lit -jN`.
         Args:
-            exe_path: str:   Local path to the executable to be run
-            cmd: [str]:      subprocess.call style command
-            local_cwd: str:  Local path to the working directory
-            env: {str: str}: Environment variables to execute under
+            exe_path: str:    Local path to the executable to be run
+            cmd: [str]:       subprocess.call style command
+            local_cwd: str:   Local path to the working directory
+            file_deps: [str]: Files required by the test
+            env: {str: str}:  Environment variables to execute under
         Returns:
             out, err, exitCode
         """
         raise NotImplementedError
 
 
 class LocalExecutor(Executor):
     def __init__(self):
         super(LocalExecutor, self).__init__()
 
-    def run(self, exe_path, cmd=None, work_dir='.', env=None):
+    def run(self, exe_path, cmd=None, work_dir='.', file_deps=None, env=None):
         cmd = cmd or [exe_path]
         env_cmd = []
         if env:
             env_cmd += ['env']
             env_cmd += ['%s=%s' % (k, v) for k, v in env.items()]
         if work_dir == '.':
             work_dir = os.getcwd()
         return executeCommand(env_cmd + cmd, cwd=work_dir)
 
 
 class PrefixExecutor(Executor):
     """Prefix an executor with some other command wrapper.
 
     Most useful for setting ulimits on commands, or running an emulator like
     qemu and valgrind.
     """
     def __init__(self, commandPrefix, chain):
         super(PrefixExecutor, self).__init__()
 
         self.commandPrefix = commandPrefix
         self.chain = chain
 
-    def run(self, exe_path, cmd=None, work_dir='.', env=None):
+    def run(self, exe_path, cmd=None, work_dir='.', file_deps=None, env=None):
         cmd = cmd or [exe_path]
         return self.chain.run(exe_path, self.commandPrefix + cmd, work_dir,
-                              env=env)
+                              file_deps, env=env)
 
 
 class PostfixExecutor(Executor):
     """Postfix an executor with some args."""
     def __init__(self, commandPostfix, chain):
         super(PostfixExecutor, self).__init__()
 
         self.commandPostfix = commandPostfix
         self.chain = chain
 
-    def run(self, exe_path, cmd=None, work_dir='.', env=None):
+    def run(self, exe_path, cmd=None, work_dir='.', file_deps=None, env=None):
         cmd = cmd or [exe_path]
-        return self.chain.run(cmd + self.commandPostfix, work_dir, env=env)
+        return self.chain.run(cmd + self.commandPostfix, work_dir, file_deps,
+                              env=env)
 
 
 
 class TimeoutExecutor(PrefixExecutor):
     """Execute another action under a timeout.
 
     Deprecated. http://reviews.llvm.org/D6584 adds timeouts to LIT.
     """
class RemoteExecutor(Executor):
 
     def delete_remote(self, remote):
         try:
             self._execute_command_remote(['rm', '-rf', remote])
         except OSError:
             # TODO: Log failure to delete?
             pass
 
-    def run(self, exe_path, cmd=None, work_dir='.', env=None):
+    def run(self, exe_path, cmd=None, work_dir='.', file_deps=None, env=None):
         target_exe_path = None
         target_cwd = None
         try:
-            target_exe_path = self.remote_temp_file()
             target_cwd = self.remote_temp_dir()
+            target_exe_path = os.path.join(target_cwd, 'libcxx_test.exe')
             if cmd:
                 # Replace exe_path with target_exe_path.
                 cmd = [c if c != exe_path else target_exe_path for c in cmd]
             else:
                 cmd = [target_exe_path]
-            self.copy_in([exe_path], [target_exe_path])
+
+            srcs = [exe_path]
+            dsts = [target_exe_path]
+            if file_deps is not None:
+                dev_paths = [os.path.join(target_cwd, os.path.basename(f))
+                             for f in file_deps]
+                srcs.extend(file_deps)
+                dsts.extend(dev_paths)
+            self.copy_in(srcs, dsts)
             return self._execute_command_remote(cmd, target_cwd, env)
-        except:
-            raise
         finally:
-            if target_exe_path:
-                self.delete_remote(target_exe_path)
             if target_cwd:
                 self.delete_remote(target_cwd)
 
     def _execute_command_remote(self, cmd, remote_work_dir='.', env=None):
         raise NotImplementedError()
 
 
 class SSHExecutor(RemoteExecutor):

libcxx/trunk/test/libcxx/test/format.py

def _evaluate_pass_test(self, test, tmpBase, execDir, lit_config):
                 report = libcxx.util.makeReport(cmd, out, err, rc)
                 report += "Compilation failed unexpectedly!"
                 return lit.Test.FAIL, report
             # Run the test
             local_cwd = os.path.dirname(source_path)
             env = None
             if self.exec_env:
                 env = self.exec_env
+            # TODO: Only list actually needed files in file_deps.
+            # Right now we just mark all of the .dat files in the same
+            # directory as dependencies, but it's likely less than that. We
+            # should add a `// FILE-DEP: foo.dat` to each test to track this.
+            data_files = [os.path.join(local_cwd, f)
+                          for f in os.listdir(local_cwd) if f.endswith('.dat')]
             out, err, rc = self.executor.run(exec_path, [exec_path],
-                                             local_cwd, env)
+                                             local_cwd, data_files, 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