[PATCH] D56324: [gn build] Add build file for DebugInfoPDBTests

[Nico Weber via Phabricator via llvm-commits]

thakis created this revision.
thakis added a reviewer: phosek.

I'm pretty unhappy this patch. DebugInfoPDBTests uses an API that requires some magic txt file to be next to the unit test executable that stores the absolute path to the LLVM source root. The choices here are:

1. Don't use the unittest() template for DebugInfoPDBTests and set `output_dir` for unit tests in two places (the gni file for every test but this one, and the BUILD.gn file for this specific test).

2. Add a 3rd unittest_foo() template variation for this one test.

I went with the latter. Hopefully I can find a way to remove llvm::getInputFileDirectory() again at some point, making most of this unnecessary.

(The CMake build has the same issue.)


https://reviews.llvm.org/D56324

Files:
  llvm/utils/gn/secondary/llvm/unittests/BUILD.gn
  llvm/utils/gn/secondary/llvm/unittests/DebugInfo/PDB/BUILD.gn
  llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni


Index: llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni
===================================================================
--- llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni
+++ llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni
@@ -64,3 +64,35 @@
 set_defaults("unittest_with_custom_main") {
   configs = shared_binary_target_configs
 }
+
+# Like unittest(), but for tests that call llvm::getInputFileDirectory().
+# Doing that (and hence using this template) is discouraged.
+# Corresponds to add_unittest_with_input_files() in CMake.
+template("unittest_with_input_files") {
+  executable(target_name) {
+    forward_variables_from(invoker, "*")
+    assert(!defined(invoker.output_dir), "cannot set unittest output_dir")
+    assert(!defined(invoker.testonly), "cannot set unittest testonly")
+    output_dir = target_out_dir
+    deps += [ "//llvm/utils/unittest/UnitTestMain" ]
+    testonly = true
+
+    # DebugInfoPDBTests uses llvm::getInputFileDirectory(), which expects
+    # a file called llvm.srcdir.txt next to the test executable that contains
+    # the path of the source directory (which contains this file).
+    # lit doesn't change the cwd while running googletests, so the cwd isn't
+    # well-defined. This means this has to be an absolute path.
+    # FIXME: This doesn't work with swarming. This should really be a data
+    # dependency, and the cwd while tests requiring input files run should
+    # be required to be some fixed directory.
+    # FIXME: Also, the GN way is to write this file at build time. But since
+    # there's only one use of this, and since this is a pattern that hopefully
+    # will disappear again, and since it doesn't have any measurable performance
+    # hit, write the file at GN time.
+    write_file("$output_dir/llvm.srcdir.txt", rebase_path("."))
+  }
+}
+
+set_defaults("unittest_with_input_files") {
+  configs = shared_binary_target_configs
+}
Index: llvm/utils/gn/secondary/llvm/unittests/DebugInfo/PDB/BUILD.gn
===================================================================
--- /dev/null
+++ llvm/utils/gn/secondary/llvm/unittests/DebugInfo/PDB/BUILD.gn
@@ -0,0 +1,16 @@
+import("//llvm/utils/unittest/unittest.gni")
+
+unittest_with_input_files("DebugInfoPDBTests") {
+  deps = [
+    "//llvm/lib/DebugInfo/CodeView",
+    "//llvm/lib/DebugInfo/MSF",
+    "//llvm/lib/DebugInfo/PDB",
+    "//llvm/lib/Testing/Support",
+  ]
+  sources = [
+    "HashTableTest.cpp",
+    "NativeSymbolReuseTest.cpp",
+    "PDBApiTest.cpp",
+    "StringTableBuilderTest.cpp",
+  ]
+}
Index: llvm/utils/gn/secondary/llvm/unittests/BUILD.gn
===================================================================
--- llvm/utils/gn/secondary/llvm/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/unittests/BUILD.gn
@@ -12,9 +12,7 @@
     "DebugInfo/CodeView:DebugInfoCodeViewTests",
     "DebugInfo/DWARF:DebugInfoDWARFTests",
     "DebugInfo/MSF:DebugInfoMSFTests",
-
-    # FIXME: Add.
-    #"DebugInfo/PDB:DebugInfoPDBTests",
+    "DebugInfo/PDB:DebugInfoPDBTests",
     "Demangle:DemangleTests",
 
     # FIXME: Add.
@@ -31,8 +29,6 @@
     "ObjectYAML:ObjectYAMLTests",
     "OptRemarks:OptRemarksTests",
     "Option:OptionTests",
-    "OptRemarks:OptRemarksTests",
-    "Option:OptionTests",
 
     # FIXME: Add.
     #"Passes:PluginsTests",
@@ -51,7 +47,7 @@
     "tools/llvm-exegesis:LLVMExegesisTests",
   ]
 
-  # Target-dependend unit tests.
+  # Target-dependent unit tests.
   # FIXME: This matches how they are set up in the cmake build,
   # but if we disable an arch after building with it on, this
   # setup leaves behind stale executables.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D56324.180280.patch
Type: text/x-patch
Size: 3640 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190104/8364d521/attachment.bin>