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

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Thanks!



================
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) {
----------------
phosek wrote:
> This is likely going to be useful elsewhere, do you plan to move into a `.gni` file later?
Right now I have one of these per {lld,llvm,clang}/test/BUILD.gn locally. The main thing in the template is the list of defines and that's somewhat different everywhere. (I'm also using a separate python script in my local branch, so I'm not 100% sure how upstream clang/test/BUILD.gn will look like. Let's revisit this when the clang test build file arrives.


================
Comment at: llvm/utils/gn/secondary/lld/test/BUILD.gn:13
+    sources = [
+      invoker.input,
+    ]
----------------
phosek wrote:
> Is this formatted with `gn format`? I'm surprised it doesn't put this onto a single line.
Yes, I run `git ls-files '*.gn' '*.gni' | xargs -n 1 gn format` every now and then (and just did again, no changes). I think gn puts one-element lists on one line after += but not after =.


================
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"),
----------------
phosek wrote:
> Do we care about builds being relocatable? If yes then we should invoke `rebase_path` with `root_build_dir` as a second argument.
That's a great comment! lit currently wants absolute directories. I explored having relocatable build dirs in https://github.com/nico/llvm-project-20170507/tree/gn_swarmingdemo (the top commit has all relevant bits) and it's possible but needs some lit changes. So for now this needs to be as-is, sadly.


================
Comment at: llvm/utils/gn/secondary/lld/test/BUILD.gn:84
+    "//llvm/tools/llc",
+    "//llvm/tools/llvm-ar:symlinks",
+    "//llvm/tools/llvm-as",
----------------
phosek wrote:
> 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.
I figured for targets having symlinks, it's simpler to always depend on the symlink target, since that's easy to remember. Don't feel strongly about it, though.


================
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}"
----------------
phosek wrote:
> You can use `not_needed` for that purpose.
Oh, that's nicer. Done.


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

https://reviews.llvm.org/D55836