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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
