[PATCH] D55836: [gn build] Add check-lld target and make it work

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/utils/gn/secondary/lld/test/BUILD.gn:8
+# The bits common to writing lit.site.cfg.py.in and Unit/lit.site.cfg.py.in.
+template("write_lit_cfg") {
+  action(target_name) {
----------------
This is likely going to be useful elsewhere, do you plan to move into a `.gni` file later?


================
Comment at: llvm/utils/gn/secondary/lld/test/BUILD.gn:13
+    sources = [
+      invoker.input,
+    ]
----------------
Is this formatted with `gn format`? I'm surprised it doesn't put this onto a single line.


================
Comment at: llvm/utils/gn/secondary/lld/test/BUILD.gn:26
+      "LLD_BINARY_DIR=" +
+          rebase_path(get_label_info("//lld", "target_out_dir")),
+      "LLD_SOURCE_DIR=" + rebase_path("//lld"),
----------------
Do we care about builds being relocatable? If yes then we should invoke `rebase_path` with `root_build_dir` as a second argument.


================
Comment at: llvm/utils/gn/secondary/lld/test/BUILD.gn:84
+    "//llvm/tools/llc",
+    "//llvm/tools/llvm-ar:symlinks",
+    "//llvm/tools/llvm-as",
----------------
Does lld really depend on these symlinks (the same below)? I don't see any uses of e.g. llvm-ranlib, llvm-strip in lld's test suite.


================
Comment at: llvm/utils/gn/secondary/llvm/tools/llvm-config/BUILD.gn:49
+    # lib/Support/Windows/Path.inc
+    # Use ${l} and ${lib} once so that gn doesn't complain they're unused.
+    system_libs = "psapi.lib shell32.lib ole32.lib uuid.lib ${l}advapi32${lib}"
----------------
You can use `not_needed` for that purpose.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55836/new/

https://reviews.llvm.org/D55836