[PATCH] D56219: [gn build] Add build files for LLVM unittests with a custom main() function

[Nico Weber via Phabricator via llvm-commits]

thakis marked an inline comment as done.
thakis added a comment.

Thanks!



================
Comment at: llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni:49
+    }
+    if (has_custom_main) {
+      deps += [ "//llvm/utils/unittest:gtest" ]
----------------
phosek wrote:
> thakis wrote:
> > I tried doing `if (defined(invoker.has_custom_main) && invoker.has_custom_main)` but then GN claimed:
> > 
> > ```
> > FAILED: build.ninja 
> > ../../../gn/out/gn --root=../.. -q --dotfile=../../llvm/utils/gn/.gn gen .
> > ERROR at //llvm/unittests/CodeGen/GlobalISel/BUILD.gn:19:21: Assignment had no effect.
> >   has_custom_main = true
> >                     ^---
> > You set the variable "has_custom_main" here and it was unused before it went
> > out of scope.
> > ```
> The way this is typically done is:
> ```
> has_custom_main = false # default value
> forward_variables_from(invoker, "*") # this is going to override the default value if set
> ```
> That way you don't need the extra condition on lines 46-48.
Ah, that explains the "assignment had no effect" error: It doesn't complain about the assignment to `has_custom_main` in the template use (even though that's where the diag points to), but that forward_variables_from forwards that into the local scope here where it isn't used. If I change the `    forward_variables_from(invoker, "*")` to `    forward_variables_from(invoker, "*", [ "has_custom_main" ])` then what I wanted to do works too.

Here's how each approach looks:

```
@@ -13,7 +17,7 @@
 template("unittest") {
   executable(target_name) {
     # Foward everything (configs, sources, deps, ...).
-    forward_variables_from(invoker, "*")
+    forward_variables_from(invoker, "*", [ "has_custom_main" ])
     assert(!defined(invoker.output_dir), "cannot set unittest output_dir")
     assert(!defined(invoker.testonly), "cannot set unittest testonly")
 
@@ -37,7 +41,12 @@ template("unittest") {
     # If you change output_dir here, look through
     # `git grep target_out_dir '*/unittests/*'` and update those too.
     output_dir = target_out_dir
-    deps += [ "//llvm/utils/unittest/UnitTestMain" ]
+
+    if (defined(invoker.has_custom_main) && invoker.has_custom_main) {
+      deps += [ "//llvm/utils/unittest:gtest" ]
+    } else {
+      deps += [ "//llvm/utils/unittest/UnitTestMain" ]
+    }
```

vs

```
@@ -12,7 +16,9 @@
 
 template("unittest") {
   executable(target_name) {
-    # Foward everything (configs, sources, deps, ...).
+    has_custom_main = false  # Default value.
+
+    # Foward everything (has_custom_main if set; configs, sources, deps, ...).
     forward_variables_from(invoker, "*")
     assert(!defined(invoker.output_dir), "cannot set unittest output_dir")
     assert(!defined(invoker.testonly), "cannot set unittest testonly")
@@ -37,7 +43,12 @@ template("unittest") {
     # If you change output_dir here, look through
     # `git grep target_out_dir '*/unittests/*'` and update those too.
     output_dir = target_out_dir
-    deps += [ "//llvm/utils/unittest/UnitTestMain" ]
+
+    if (has_custom_main) {
+      deps += [ "//llvm/utils/unittest:gtest" ]
+    } else {
+      deps += [ "//llvm/utils/unittest/UnitTestMain" ]
+    }
```


The first approach requires knowing about forward_variables_from's 3rd arg, but is slightly shorter. I went with the version that you suggested.


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

https://reviews.llvm.org/D56219