[Lldb-commits] [PATCH] D42281: WIP: compile the LLDB tests out-of-tree

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: packages/Python/lldbsuite/test/dotest.py:625
     os.environ["LLDB_TEST"] = scriptPath
+    os.environ["LLDB_BUILD"] = configuration.test_build_dir
 
----------------
Here this has the possibility of setting `os.environ["LLDB_BUILD"] = None`.  Is this desired?


================
Comment at: packages/Python/lldbsuite/test/dotest.py:1193
+    # Note that it's not dotest's job to clean this directory.
+    orig_working_dir = os.getcwd()
+    if configuration.test_build_dir:
----------------
Nothing appears to use `orig_working_dir`, and we never set a new working directory anyway, so it doesn't seem necessary to save the original working directory.  Can this line be removed?


================
Comment at: packages/Python/lldbsuite/test/dotest.py:1198
+    else:
+        configuration.test_build_dir = os.getcwd()
+
----------------
This relates to the environment variable comment earlier.  The code that sets the environment variable gets run before this.  So that could set the environment variable to `None`, and then this code could set `configuration.test_build_dir` to something valid.  It seems to me like we should ensure that the environment variable always gets set to *something*, then the multiprocessing children can just assume it has a valid value.


================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:2246
+        if ("LLDB_BUILD" in os.environ):
+            full_dir = os.path.join(os.environ["LLDB_BUILD"], self.mydir)
+            try: os.makedirs(full_dir)
----------------
Related to previous comments about the environment variable, can we just assume the variable is set here?


================
Comment at: packages/Python/lldbsuite/test/plugins/builder_base.py:53
 
-
-def getMake():
-    """Returns the name for GNU make"""
+def getMake(rel_testdir):
+    """Returns the invocation for GNU make"""
----------------
Can you call this maybe `test_subdir` or something, and update the docstring to indicate what it actually is?


================
Comment at: packages/Python/lldbsuite/test/plugins/builder_base.py:62
+    lldb_test = os.environ["LLDB_TEST"]
+    lldb_build = os.environ["LLDB_BUILD"]
+    build_dir = os.path.join(lldb_build, rel_testdir)
----------------
Related to my previous comment about environment variables, but you are assuming it exists here, so it does seem like we can assume it exists in the other codepath too.


================
Comment at: packages/Python/lldbsuite/test/plugins/builder_base.py:216
     return True
+    #  #import traceback
+    #  # traceback.print_stack()
----------------
Can this be deleted?


https://reviews.llvm.org/D42281