[PATCH] Add remote testing support to the lit config.

Dan Albert danalbert at google.com
Thu Feb 5 14:27:14 PST 2015 

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.


================
Comment at: libcxx/test/format.py:98
@@ -91,1 +97,3 @@
+    def _evaluate_pass_test(self, test, tmpBase, execDir, lit_config,
+                            target_executor):
         source_path = test.getSourcePath()
----------------
No reason for this to be an argument, it's a member.

================
Comment at: libcxx/test/format.py:121
@@ -112,4 +120,3 @@
                 cmd = lit_config.valgrindArgs + cmd
-            cmd += [exec_path]
-            out, err, rc = lit.util.executeCommand(
-                cmd, cwd=os.path.dirname(source_path))
+            if target_executor:
+                target_exec_path = target_executor.remote_from_local_file(exec_path)
----------------
jroelofs wrote:
> 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.
> 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).

================
Comment at: test/libcxx/test/config.py:14
@@ -13,3 +13,3 @@
 from libcxx.compiler import CXXCompiler
-
+from libcxx.test.remote import *
 
----------------
Should avoid wildcard imports.

================
Comment at: test/libcxx/test/config.py:123
@@ +122,3 @@
+        if exec_str:
+            self.target_executor = eval(exec_str)
+            self.lit_config.note("inferred target_executor as: %r" % exec_str)
----------------
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.

================
Comment at: test/libcxx/test/config.py:270
@@ -259,1 +269,3 @@
 
+        # Insert the platform name into the available features as a lower case.
+        # Strip the '2' from linux2.
----------------
jroelofs wrote:
> I don't know where this came from. Bad merge maybe?
Yes. ericwf added this the other day because it was something that went missing from the libcxxabi config (one test has a // REQUIRES: linux2)

================
Comment at: test/libcxx/test/format.py:122
@@ +121,3 @@
+            if target_executor:
+                target_exec_path = target_executor.remote_from_local_file(exec_path)
+                local_cwd = os.path.dirname(source_path)
----------------
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.

================
Comment at: test/libcxx/test/format.py:149
@@ +148,3 @@
+            # If there's a target_executor, let it do its own exe cleanup
+            if not target_executor:
+                self._clean(exec_path)
----------------
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).

================
Comment at: test/libcxx/test/remote.py:6
@@ +5,3 @@
+# Shamelessly stolen from lit/util.py, so we don't
+# have to deal with a lit import:
+import signal
----------------
What makes import hard here?

================
Comment at: test/libcxx/test/remote.py:193
@@ +192,3 @@
+
+class ValgrindExecutor(PrefixExecutor):
+    pass
----------------
This would be a nice way to factor out that cruft. Good thinking.

================
Comment at: test/libcxx/test/remote.py:212
@@ +211,3 @@
+        self.username = username
+        self.host     = host
+        self.scpCommand = 'scp'
----------------
Don't align assignments (you haven't matched all of them anyway).

================
Comment at: test/libcxx/test/remote.py:223
@@ +222,3 @@
+        # For temp cleanup
+        self.remote_temp_files = []
+        self.remote_temp_dirs = []
----------------
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.)

================
Comment at: test/libcxx/test/remote.py:255
@@ +254,3 @@
+    def remote_from_local_file(self, local_file):
+        if not self.filemap.has_key(local_file):
+            remote_file = self.remote_temp_file(".some_file")
----------------
    if local_file not in self.filemap:

================
Comment at: test/libcxx/test/remote.py:306
@@ +305,3 @@
+        remote_cmd = ' '.join(command)
+        if remote_work_dir is not '.' and remote_work_dir is not '':
+          remote_cmd = 'cd ' + remote_work_dir + ' && ' + remote_cmd
----------------
Use `!=` for comparing strings rather than `is not`. `is not` should be used to test that two things are exactly the same object.

================
Comment at: test/libcxx/test/remote.py:318
@@ +317,3 @@
+
+class DebugExecutor(Executor):
+    def __init__(self, label, chain, before=True, after=True):
----------------
jroelofs wrote:
> This class screams that it wants reflection... I just don't know how to do that.
You probably want something along the lines of http://stackoverflow.com/a/3467879/632035

Should make the whole class unnecessary.

http://reviews.llvm.org/D7380

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/

More information about the cfe-commits mailing list