[Lldb-commits] [PATCH] D50751: Allow use of self.filecheck in LLDB tests (c.f self.expect)

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/packages/Python/lldbsuite/test/configuration.py:194
+        # of the test build dir is the LLVM build dir.
+        llvm_build_dir = os.path.dirname(os.path.abspath(test_build_dir))
+    assert os.path.lexists(llvm_build_dir)
----------------
This is true if we build LLDB as part of LLVM, but this doesn't seem correct if LLDB is built against an existing LLVM (which is in general supported in LLDB's CMake if IIUC). I think the best way is to pass the binary directory for this to dotest and configure it in the lit.cfg.


================
Comment at: lldb/packages/Python/lldbsuite/test/configuration.py:204
+    filecheck_path = os.path.join(get_llvm_build_dir(), "bin", "FileCheck")
+    assert os.path.lexists(filecheck_path)
+    return filecheck_path
----------------
Windows itself can probably handle the fact that the `.exe` suffix is missing for FileCheck, but this assert will fail in this situation.


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:51
 import sys
+import commands
+import tempfile
----------------
I think that list is supposed to be sorted alphabetically


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2166
+        # Write the output to a temporary file.
+        input_file = tempfile.NamedTemporaryFile()
+        input_file.write(output)
----------------
Making a temporary file and opening it later by name is not really supported (see the NamedTemporaryFile docs). Could we maybe pass the output to FileCheck via stdin? That would also save us from the detour over the fs.


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2181
+        filecheck_bin = configuration.get_filecheck_path()
+        filecheck_cmd = "{0} {1} -input-file {2} {3}".format(filecheck_bin,
+                check_file_abs, input_file.name, filecheck_options)
----------------
We should quote the paths in here. Or at least, the temporary file path should be quoted as it will contain spaces on some setups.


https://reviews.llvm.org/D50751