Skip to content

Commit

Permalink
Remove GoSource/GoLibrary from test/binary rules (#4044)
Browse files Browse the repository at this point in the history
**What type of PR is this?**
Starlark cleanup

**What does this PR do? Why is it needed?**
We don't want go_binary or go_test targets to be included in `deps` or
`embed`. We have a check for it today, but we can push this into the
starlark type system by ensuring they don't have these providers.

**Which issues(s) does this PR fix?**

Fixes #

**Other notes for review**

---------

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
  • Loading branch information
dzbarsky and fmeum authored Aug 16, 2024
1 parent a01ba7c commit 2d22666
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 28 deletions.
21 changes: 0 additions & 21 deletions go/private/context.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -284,10 +284,7 @@ def _library_to_source(go, attr, library, coverage_instrumented):
"pgoprofile": getattr(attr, "pgoprofile", None),
}

for dep in source["deps"]:
_check_binary_dep(go, dep, "deps")
for e in getattr(attr, "embed", []):
_check_binary_dep(go, e, "embed")
_merge_embed(source, e)

source["deps"] = _dedup_archives(source["deps"])
Expand Down Expand Up @@ -324,24 +321,6 @@ def _collect_runfiles(go, data, deps):
[get_source(t).runfiles for t in deps],
)

def _check_binary_dep(go, dep, edge):
"""Checks that this rule doesn't depend on a go_binary or go_test.
go_binary and go_test may return providers with useful information for other
rules (like go_path), but go_binary and go_test may not depend on other
go_binary and go_test targets. Their dependencies may be built in
different modes, resulting in conflicts and opaque errors.
"""
if (type(dep) == "Target" and
DefaultInfo in dep and
getattr(dep[DefaultInfo], "files_to_run", None) and
dep[DefaultInfo].files_to_run.executable):
fail("rule {rule} depends on executable {dep} via {edge}. This is not safe for cross-compilation. Depend on go_library instead.".format(
rule = str(go.label),
dep = str(dep.label),
edge = edge,
))

def _check_importpaths(ctx):
paths = []
p = getattr(ctx.attr, "importpath", "")
Expand Down
2 changes: 0 additions & 2 deletions go/private/rules/binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,6 @@ def _go_binary_impl(ctx):
validation_output = archive.data._validation_output

providers = [
library,
source,
archive,
OutputGroupInfo(
cgo_exports = archive.cgo_exports,
Expand Down
6 changes: 1 addition & 5 deletions go/private/rules/cross.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
load(
"//go/private:providers.bzl",
"GoArchive",
"GoLibrary",
"GoSource",
)
load(
"//go/private/rules:transition.bzl",
Expand Down Expand Up @@ -84,8 +82,6 @@ def _go_cross_impl(ctx):
providers = [
ctx.attr.target[provider]
for provider in [
GoLibrary,
GoSource,
GoArchive,
OutputGroupInfo,
CcInfo,
Expand All @@ -100,7 +96,7 @@ _go_cross_kwargs = {
"target": attr.label(
doc = """Go binary target to transition to the given platform and/or sdk_version.
""",
providers = [GoLibrary, GoSource, GoArchive],
providers = [GoArchive],
mandatory = True,
),
"platform": attr.label(
Expand Down
3 changes: 3 additions & 0 deletions tests/core/starlark/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
load(":common_tests.bzl", "common_test_suite")
load(":context_tests.bzl", "context_test_suite")
load(":provider_tests.bzl", "provider_test_suite")
load(":sdk_tests.bzl", "sdk_test_suite")

common_test_suite()

context_test_suite()

provider_test_suite()

sdk_test_suite()
69 changes: 69 additions & 0 deletions tests/core/starlark/provider_tests.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts")
load("//go:def.bzl", "go_binary", "go_library", "go_test")

# go_binary and go_test targets must not be used as deps/embed attributes;
# their dependencies may be built in different modes, resulting in conflicts and opaque errors.
def _providers_test_impl(ctx):
env = analysistest.begin(ctx)
asserts.expect_failure(env, "does not have mandatory providers")
return analysistest.end(env)

providers_test = analysistest.make(
_providers_test_impl,
expect_failure = True,
)

def provider_test_suite():
go_binary(
name = "go_binary",
tags = ["manual"],
)

go_library(
name = "lib_binary_deps",
deps = [":go_binary"],
tags = ["manual"],
)

providers_test(
name = "go_binary_deps_test",
target_under_test = ":lib_binary_deps",
)

go_library(
name = "lib_binary_embed",
embed = [":go_binary"],
tags = ["manual"],
)

providers_test(
name = "go_binary_embed_test",
target_under_test = ":lib_binary_embed",
)

go_test(
name = "go_test",
tags = ["manual"],
)

go_library(
name = "lib_test_deps",
deps = [":go_test"],
tags = ["manual"],
)

providers_test(
name = "go_test_deps_test",
target_under_test = ":lib_test_deps",
)

go_library(
name = "lib_embed_test",
embed = [":go_test"],
tags = ["manual"],
)

providers_test(
name = "go_test_embed_test",
target_under_test = ":lib_embed_test",
)

0 comments on commit 2d22666

Please sign in to comment.