bazel-contrib / rules_go

Go rules for Bazel
Apache License 2.0
1.38k stars 661 forks source link

go_test: could not use x_defs to stamp rlocationpath of another binary in data #3847

Open sluongng opened 9 months ago

sluongng commented 9 months ago

What version of rules_go are you using?

0.44.2

What version of gazelle are you using?

0.35.0

What version of Bazel are you using?

7.0.2

Does this issue reproduce with the latest releases of all the above?

yes

What operating system and processor architecture are you using?

MacOS ARM64

Any other potentially useful information about your toolchain?

Irrelevant

What did you do?

Here is a minimal reproduce https://github.com/sluongng/rules-go-test-xdefs-issue/blob/9360ec82c4f307d0e986f36580b2b90cb6760f1c/BUILD#L10-L12

What did you expect to see?

The x_defs should detect the runfile locations of the target and add it to the Go variable

What did you see instead?

ERROR: /Users/sluongng/work/misc/test-go/BUILD:6:8: in go_test rule //:a_test: label '//liba:liba' in $(location) expression is not a declared prerequisite of this rule
ERROR: /Users/sluongng/work/misc/test-go/BUILD:6:8: Analysis of target '//:a_test' failed
sluongng commented 9 months ago

I think this is a go_test issue and not a Bazel issue because similar x_defs on a go_library target would work.

fmeum commented 9 months ago

It's not clear to me why the test in https://github.com/bazelbuild/rules_go/commit/f5da3bebc4c655d026a5e93587f35a16145f3379 doesn't catch this. Could you try to make a minimal change to it that triggers the issue?

sluongng commented 9 months ago

Let's see https://github.com/bazelbuild/rules_go/pull/3848

sluongng commented 9 months ago

I could reproduce with this diff

diff --git a/tests/core/go_test/x_defs/BUILD.bazel b/tests/core/go_test/x_defs/BUILD.bazel
index 7edc4885..9111d581 100644
--- a/tests/core/go_test/x_defs/BUILD.bazel
+++ b/tests/core/go_test/x_defs/BUILD.bazel
@@ -59,12 +59,21 @@ go_library(
     },
 )

+sh_binary(
+    name = "my_bin",
+    srcs = ["my_bin.sh"],
+)
+
 go_test(
     name = "x_defs_test",
     srcs = ["x_defs_test.go"],
-    data = ["x_defs_test.go"],
+    data = [
+        "x_defs_test.go",
+        ":my_bin",
+    ],
     x_defs = {
         "BinGo": "$(rlocationpath x_defs_test.go)",
+        "temp": "$(rlocationpath :my_bin)",
     },
     deps = [
         ":x_defs_lib",

Perhaps it's only trigger if the input was a label? 🤔

sluongng commented 9 months ago

with

diff --git a/go/private/context.bzl b/go/private/context.bzl
index 38e1d673..1d1bb4ee 100644
--- a/go/private/context.bzl
+++ b/go/private/context.bzl
@@ -291,6 +291,7 @@ def _library_to_source(go, attr, library, coverage_instrumented):
         if "." not in k:
             k = "{}.{}".format(library.importmap, k)
         x_defs[k] = v
+    print("DEBUG: {}".format(x_defs))
     source["x_defs"] = x_defs
     if not source["cgo"]:
         for k in ("cdeps", "cppopts", "copts", "cxxopts", "clinkopts"):

I got

DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {"github.com/bazelbuild/rules_go/tests/core/go_test/x_defs/x_defs_lib.LibGo": "io_bazel_rules_go/tests/core/go_test/x_defs/x_defs_lib.go"}
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {"tests/core/go_test/x_defs/x_defs_test.BinGo": "io_bazel_rules_go/tests/core/go_test/x_defs/x_defs_test.go", "tests/core/go_test/x_defs/x_defs_test.temp": "$(rlocationpath :my_bin)"}
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {"tests/core/go_test/x_defs/x_defs_test_test.BinGo": "io_bazel_rules_go/tests/core/go_test/x_defs/x_defs_test.go", "tests/core/go_test/x_defs/x_defs_test_test.temp": "$(rlocationpath :my_bin)"}

So I think ctx.expand_location was not working as expected?

sluongng commented 9 months ago

I updated the PR with my latest setup

> bazel shutdown
> bazel test tests/core/go_test/x_defs:x_defs_test
Starting local Bazel server and connecting to it...
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {}
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {}
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {}
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {}
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {}
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {"github.com/bazelbuild/rules_go/tests/core/go_test/x_defs/x_defs_lib.LibGo": "io_bazel_rules_go/tests/core/go_test/x_defs/x_defs_lib.go", "github.com/bazelbuild/rules_go/tests/core/go_test/x_defs/x_defs_lib.temp": "io_bazel_rules_go/tests/core/go_test/x_defs/blah.txt"}
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {"tests/core/go_test/x_defs/x_defs_test.BinGo": "io_bazel_rules_go/tests/core/go_test/x_defs/x_defs_test.go", "tests/core/go_test/x_defs/x_defs_test.temp": "io_bazel_rules_go/tests/core/go_test/x_defs/blah.txt"}
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {"tests/core/go_test/x_defs/x_defs_test_test.BinGo": "io_bazel_rules_go/tests/core/go_test/x_defs/x_defs_test.go", "tests/core/go_test/x_defs/x_defs_test_test.temp": "$(rlocationpath blah.txt)"}
DEBUG: /Users/sluongng/work/bazelbuild/rules_go/go/private/context.bzl:294:10: DEBUG: {}
ERROR: /Users/sluongng/work/bazelbuild/rules_go/tests/core/go_test/x_defs/BUILD.bazel:72:8: in go_test rule //tests/core/go_test/x_defs:x_defs_test: label '//tests/core/go_test/x_defs:blah.txt' in $(location) expression is not a declared prerequisite of this rule
ERROR: /Users/sluongng/work/bazelbuild/rules_go/tests/core/go_test/x_defs/BUILD.bazel:72:8: Analysis of target '//tests/core/go_test/x_defs:x_defs_test' failed
ERROR: Analysis of target '//tests/core/go_test/x_defs:x_defs_test' failed; build aborted
...

So it looks like the expand_location worked in go_library and not in go_test on a data label target. I wonder if the data label was not included in the final providers of go_test somehow 🤔

sluongng commented 9 months ago

Ok I figured out. We do not pass the full context to external compile action but only a subset of context. That subset context was missing data attribute.

Changed the PR to a fix.