Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sluongng opened this issue Feb 5, 2024 · 7 comments

Comments

@sluongng
Copy link
Contributor

sluongng commented Feb 5, 2024

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
Copy link
Contributor Author

sluongng commented Feb 5, 2024

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
Copy link
Member

fmeum commented Feb 6, 2024

It's not clear to me why the test in f5da3be doesn't catch this. Could you try to make a minimal change to it that triggers the issue?

@sluongng
Copy link
Contributor Author

sluongng commented Feb 6, 2024

Let's see #3848

@sluongng
Copy link
Contributor Author

sluongng commented Feb 6, 2024

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
Copy link
Contributor Author

sluongng commented Feb 6, 2024

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
Copy link
Contributor Author

sluongng commented Feb 6, 2024

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
Copy link
Contributor Author

sluongng commented Feb 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants