Skip to content

Commit

Permalink
Transition on edges not self
Browse files Browse the repository at this point in the history
As per bazelbuild/bazel#15157 currently we
can't configure any transition-relevant attributes using select. This is
because we use self-transitions on targets, and when this happens,
configurable attributes aren't passed to the transition, so we make
incorrect transition decisions.

We only actually consult configuration data in `_go_context_data`, so we
only actually need to transition on the edges which (transitively) reach
a `_go_context_data`, which is `_go_context_data` itself and `deps`.
  • Loading branch information
illicitonion committed Jun 14, 2022
1 parent 8d6b21c commit f848748
Show file tree
Hide file tree
Showing 9 changed files with 221 additions and 84 deletions.
27 changes: 20 additions & 7 deletions go/private/context.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -365,20 +365,21 @@ def go_context(ctx, attr = None):
coverdata = None
nogo = None
if hasattr(attr, "_go_context_data"):
if CgoContextInfo in attr._go_context_data:
cgo_context_info = attr._go_context_data[CgoContextInfo]
go_config_info = attr._go_context_data[GoConfigInfo]
stdlib = attr._go_context_data[GoStdLib]
coverdata = attr._go_context_data[GoContextInfo].coverdata
nogo = attr._go_context_data[GoContextInfo].nogo
go_context_data = _flatten(attr._go_context_data)
if CgoContextInfo in go_context_data:
cgo_context_info = go_context_data[CgoContextInfo]
go_config_info = go_context_data[GoConfigInfo]
stdlib = go_context_data[GoStdLib]
coverdata = go_context_data[GoContextInfo].coverdata
nogo = go_context_data[GoContextInfo].nogo
if getattr(attr, "_cgo_context_data", None) and CgoContextInfo in attr._cgo_context_data:
cgo_context_info = attr._cgo_context_data[CgoContextInfo]
if getattr(attr, "cgo_context_data", None) and CgoContextInfo in attr.cgo_context_data:
cgo_context_info = attr.cgo_context_data[CgoContextInfo]
if hasattr(attr, "_go_config"):
go_config_info = attr._go_config[GoConfigInfo]
if hasattr(attr, "_stdlib"):
stdlib = attr._stdlib[GoStdLib]
stdlib = _flatten(attr._stdlib)[GoStdLib]

mode = get_mode(ctx, toolchain, cgo_context_info, go_config_info)
tags = mode.tags
Expand Down Expand Up @@ -837,3 +838,15 @@ go_config = rule(

def _expand_opts(go, attribute_name, opts):
return [go._ctx.expand_make_variables(attribute_name, opt, {}) for opt in opts]

# Used to get attribute values which may have been transitioned.
# Transitioned attributes end up as lists.
# We never use split-transitions, so we always expect exactly one element in those lists.
# But if the attribute wasn't transitioned, it won't be a list.
def _flatten(maybe_list):
if type(maybe_list) == "list":
if len(maybe_list) == 1:
return maybe_list[0]
else:
fail("Expected exactly one element in list but got {}".format(maybe_list))
return maybe_list
4 changes: 2 additions & 2 deletions go/private/mode.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ def get_mode(ctx, go_toolchain, cgo_context_info, go_config_info):
debug = go_config_info.debug if go_config_info else False
linkmode = go_config_info.linkmode if go_config_info else LINKMODE_NORMAL
cover_format = go_config_info and go_config_info.cover_format
goos = go_toolchain.default_goos
goarch = go_toolchain.default_goarch
goos = go_toolchain.default_goos if getattr(ctx.attr, "goos", "auto") == "auto" else ctx.attr.goos
goarch = go_toolchain.default_goarch if getattr(ctx.attr, "goarch", "auto") == "auto" else ctx.attr.goarch

# TODO(jayconrod): check for more invalid and contradictory settings.
if pure and race:
Expand Down
9 changes: 5 additions & 4 deletions go/private/rules/binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ load(
)
load(
"//go/private/rules:transition.bzl",
"go_transition_rule",
"go_transition",
"non_go_transition",
)
load(
Expand Down Expand Up @@ -213,6 +213,7 @@ _go_binary_kwargs = {
doc = """List of Go libraries this package imports directly.
These may be `go_library` rules or compatible rules with the [GoLibrary] provider.
""",
cfg = go_transition,
),
"embed": attr.label_list(
providers = [GoLibrary],
Expand All @@ -225,6 +226,7 @@ _go_binary_kwargs = {
embedding binary may not also have `cgo = True`. See [Embedding] for
more information.
""",
cfg = go_transition,
),
"embedsrcs": attr.label_list(
allow_files = True,
Expand Down Expand Up @@ -390,7 +392,7 @@ _go_binary_kwargs = {
</ul>
""",
),
"_go_context_data": attr.label(default = "//:go_context_data"),
"_go_context_data": attr.label(default = "//:go_context_data", cfg = go_transition),
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
Expand All @@ -410,8 +412,7 @@ _go_binary_kwargs = {
}

go_binary = rule(executable = True, **_go_binary_kwargs)
go_transition_binary = go_transition_rule(executable = True, **_go_binary_kwargs)
go_non_executable_transition_binary = go_transition_rule(executable = False, **_go_binary_kwargs)
go_non_executable_binary = rule(executable = False, **_go_binary_kwargs)

def _go_tool_binary_impl(ctx):
sdk = ctx.attr.sdk[GoSDK]
Expand Down
8 changes: 5 additions & 3 deletions go/private/rules/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ load(
)
load(
"//go/private/rules:transition.bzl",
"go_transition_rule",
"go_transition",
"non_go_transition",
)
load(
Expand Down Expand Up @@ -220,6 +220,7 @@ _go_test_kwargs = {
doc = """List of Go libraries this test imports directly.
These may be go_library rules or compatible rules with the [GoLibrary] provider.
""",
cfg = go_transition,
),
"embed": attr.label_list(
providers = [GoLibrary],
Expand All @@ -231,6 +232,7 @@ _go_test_kwargs = {
and the embedding library may not also have `cgo = True`. See [Embedding]
for more information.
""",
cfg = go_transition,
),
"embedsrcs": attr.label_list(
allow_files = True,
Expand Down Expand Up @@ -402,10 +404,11 @@ _go_test_kwargs = {
See [Cross compilation] for more information.
""",
),
"_go_context_data": attr.label(default = "//:go_context_data"),
"_go_context_data": attr.label(default = "//:go_context_data", cfg = go_transition),
"_testmain_additional_deps": attr.label_list(
providers = [GoLibrary],
default = ["//go/tools/bzltestutil"],
cfg = go_transition,
),
# Workaround for bazelbuild/bazel#6293. See comment in lcov_merger.sh.
"_lcov_merger": attr.label(
Expand Down Expand Up @@ -453,7 +456,6 @@ _go_test_kwargs = {
}

go_test = rule(**_go_test_kwargs)
go_transition_test = go_transition_rule(**_go_test_kwargs)

def _recompile_external_deps(go, external_source, internal_archive, library_labels):
"""Recompiles some archives in order to split internal and external tests.
Expand Down
56 changes: 0 additions & 56 deletions go/private/rules/transition.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -80,62 +80,6 @@ _SETTING_KEY_TO_ORIGINAL_SETTING_KEY = {
for setting in TRANSITIONED_GO_SETTING_KEYS
}

def go_transition_wrapper(kind, transition_kind, name, **kwargs):
"""Wrapper for rules that may use transitions.
This is used in place of instantiating go_binary or go_transition_binary
directly. If one of the transition attributes is set explicitly, it
instantiates the rule with a transition. Otherwise, it instantiates the
regular rule. This prevents targets from being rebuilt for an alternative
configuration identical to the default configuration.
"""
transition_keys = ("goos", "goarch", "pure", "static", "msan", "race", "gotags", "linkmode")
need_transition = any([key in kwargs for key in transition_keys])
if need_transition:
transition_kind(name = name, **kwargs)
else:
kind(name = name, **kwargs)

def go_transition_rule(**kwargs):
"""Like "rule", but adds a transition and mode attributes."""
kwargs = dict(kwargs)
kwargs["attrs"].update({
"goos": attr.string(
default = "auto",
values = ["auto"] + {goos: None for goos, _ in GOOS_GOARCH}.keys(),
),
"goarch": attr.string(
default = "auto",
values = ["auto"] + {goarch: None for _, goarch in GOOS_GOARCH}.keys(),
),
"pure": attr.string(
default = "auto",
values = ["auto", "on", "off"],
),
"static": attr.string(
default = "auto",
values = ["auto", "on", "off"],
),
"msan": attr.string(
default = "auto",
values = ["auto", "on", "off"],
),
"race": attr.string(
default = "auto",
values = ["auto", "on", "off"],
),
"gotags": attr.string_list(default = []),
"linkmode": attr.string(
default = "auto",
values = ["auto"] + LINKMODES,
),
"_whitelist_function_transition": attr.label(
default = "@bazel_tools//tools/whitelists/function_transition_whitelist",
),
})
kwargs["cfg"] = go_transition
return rule(**kwargs)

def _go_transition_impl(settings, attr):
# NOTE: Keep the list of rules_go settings set by this transition in sync
# with POTENTIALLY_TRANSITIONED_SETTINGS.
Expand Down
30 changes: 18 additions & 12 deletions go/private/rules/wrappers.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,11 @@ load(
load(
"//go/private/rules:binary.bzl",
"go_binary",
"go_non_executable_transition_binary",
"go_transition_binary",
"go_non_executable_binary",
)
load(
"//go/private/rules:test.bzl",
"go_test",
"go_transition_test",
)
load(
"//go/private/rules:transition.bzl",
"go_transition_wrapper",
)

def _cgo(name, kwargs):
Expand All @@ -51,12 +45,24 @@ def go_library_macro(name, **kwargs):
def go_binary_macro(name, **kwargs):
"""See docs/go/core/rules.md#go_binary for full documentation."""
_cgo(name, kwargs)

if kwargs.get("goos") != None or kwargs.get("goarch") != None:
for key, value in kwargs.items():
if type(value) == "select":
# In the long term, we should replace goos/goarch with Bazel-native platform
# support, but while we have the mechanisms, we try to avoid people trying to use
# _both_ goos/goarch _and_ native platform support.
#
# It's unclear to users whether the select should happen before or after the
# goos/goarch is reconfigured, and we can't interpose code to force either
# behaviour, so we forbid this.
fail("Cannot use select for go_binary with goos/goarch set, but {} was a select".format(key))

if kwargs.get("linkmode", default = LINKMODE_NORMAL) in LINKMODES_EXECUTABLE:
go_transition_wrapper(go_binary, go_transition_binary, name = name, **kwargs)
go_binary(name = name, **kwargs)
else:
# A non-normal link mode implies the use of transitions, so we don't have to define a
# non-executable version of the untransitioned go_binary.
go_transition_wrapper(None, go_non_executable_transition_binary, name = name, **kwargs)
go_non_executable_binary(name = name, **kwargs)

if kwargs.get("linkmode") in (LINKMODE_C_ARCHIVE, LINKMODE_C_SHARED):
# Create an alias to tell users of the `.cc` rule that it is deprecated.
native.alias(
Expand All @@ -70,4 +76,4 @@ def go_binary_macro(name, **kwargs):
def go_test_macro(name, **kwargs):
"""See docs/go/core/rules.md#go_test for full documentation."""
_cgo(name, kwargs)
go_transition_wrapper(go_test, go_transition_test, name = name, **kwargs)
go_test(name = name, **kwargs)
10 changes: 10 additions & 0 deletions tests/core/go_binary/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,16 @@ load(":many_deps.bzl", "many_deps")

test_suite(name = "go_binary")

go_bazel_test(
name = "configurable_attribute_bad_test",
srcs = ["configurable_attribute_bad_test.go"],
)

go_bazel_test(
name = "configurable_attribute_good_test",
srcs = ["configurable_attribute_good_test.go"],
)

go_binary(
name = "hello",
srcs = ["hello.go"],
Expand Down
69 changes: 69 additions & 0 deletions tests/core/go_binary/configurable_attribute_bad_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// Copyright 2022 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package configurable_attribute_bad_test

import (
"strings"
"testing"

"github.com/bazelbuild/rules_go/go/tools/bazel_testing"
)

func TestMain(m *testing.M) {
bazel_testing.TestMain(m, bazel_testing.Args{
Main: `
-- BUILD.bazel --
load("@io_bazel_rules_go//go:def.bzl", "go_binary")
go_binary(
name = "main",
srcs = [
"main.go",
],
goos = "darwin",
goarch = "amd64",
gotags = select({
"@io_bazel_rules_go//go/platform:linux": ["penguins"],
"//conditions:default": ["nopenguins"],
}),
)
-- main.go --
package main
import "fmt"
func main() {
fmt.Println("Howdy")
}
`,
})
}

func TestConfigurableGotagsAttribute(t *testing.T) {
_, err := bazel_testing.BazelOutput("build", "//:main")
if err == nil {
t.Fatal("Want error")
}
eErr, ok := err.(*bazel_testing.StderrExitError)
if !ok {
t.Fatalf("Want StderrExitError but got %v", err)
}
stderr := eErr.Error()
want := "Cannot use select for go_binary with goos/goarch set, but gotags was a select"
if !strings.Contains(stderr, want) {
t.Fatalf("Want error message containing %q but got %v", want, stderr)
}
}
Loading

0 comments on commit f848748

Please sign in to comment.