From 2d22666b0a6eb7f21d97a4681e777c60bcfb778b Mon Sep 17 00:00:00 2001 From: David Zbarsky Date: Fri, 16 Aug 2024 03:23:04 -0400 Subject: [PATCH] Remove GoSource/GoLibrary from test/binary rules (#4044) **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 --- go/private/context.bzl | 21 -------- go/private/rules/binary.bzl | 2 - go/private/rules/cross.bzl | 6 +-- tests/core/starlark/BUILD.bazel | 3 ++ tests/core/starlark/provider_tests.bzl | 69 ++++++++++++++++++++++++++ 5 files changed, 73 insertions(+), 28 deletions(-) create mode 100644 tests/core/starlark/provider_tests.bzl diff --git a/go/private/context.bzl b/go/private/context.bzl index 469e3ee234..bddcc01472 100644 --- a/go/private/context.bzl +++ b/go/private/context.bzl @@ -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"]) @@ -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", "") diff --git a/go/private/rules/binary.bzl b/go/private/rules/binary.bzl index a13351d97c..193fd3741a 100644 --- a/go/private/rules/binary.bzl +++ b/go/private/rules/binary.bzl @@ -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, diff --git a/go/private/rules/cross.bzl b/go/private/rules/cross.bzl index 4868d86c40..ff7ef63c18 100644 --- a/go/private/rules/cross.bzl +++ b/go/private/rules/cross.bzl @@ -15,8 +15,6 @@ load( "//go/private:providers.bzl", "GoArchive", - "GoLibrary", - "GoSource", ) load( "//go/private/rules:transition.bzl", @@ -84,8 +82,6 @@ def _go_cross_impl(ctx): providers = [ ctx.attr.target[provider] for provider in [ - GoLibrary, - GoSource, GoArchive, OutputGroupInfo, CcInfo, @@ -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( diff --git a/tests/core/starlark/BUILD.bazel b/tests/core/starlark/BUILD.bazel index 5f47b1557f..3c10ffb7ad 100644 --- a/tests/core/starlark/BUILD.bazel +++ b/tests/core/starlark/BUILD.bazel @@ -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() diff --git a/tests/core/starlark/provider_tests.bzl b/tests/core/starlark/provider_tests.bzl new file mode 100644 index 0000000000..5d8b355b76 --- /dev/null +++ b/tests/core/starlark/provider_tests.bzl @@ -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", + )