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

Use edge transitions not self transitions #3130

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions go/private/rules/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ bzl_library(
"//go/private:providers",
"//go/private:rpath",
"//go/private/rules:transition",
"@bazel_skylib//lib:dicts",
],
)

Expand Down Expand Up @@ -132,6 +133,7 @@ bzl_library(
"//go/private:providers",
"//go/private/rules:binary",
"//go/private/rules:transition",
"@bazel_skylib//lib:dicts",
"@bazel_skylib//lib:structs",
],
)
Expand All @@ -148,6 +150,7 @@ bzl_library(
"//go/private:mode",
"//go/private:platforms",
"//go/private:providers",
"//go/private/skylib/lib:dicts",
],
)

Expand Down
52 changes: 39 additions & 13 deletions go/private/rules/binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,11 @@ load(
)
load(
"//go/private/rules:transition.bzl",
"go_transition_rule",
"ForwardingPastTransitionProvider",
"forward_through_transition_impl",
"go_transition",
"non_go_transition",
"transition_attrs",
)
load(
"//go/private:mode.bzl",
Expand All @@ -44,6 +47,10 @@ load(
"//go/private:rpath.bzl",
"rpath",
)
load(
"@bazel_skylib//lib:dicts.bzl",
"dicts",
)

_EMPTY_DEPSET = depset([])

Expand Down Expand Up @@ -124,19 +131,14 @@ def _go_binary_impl(ctx):
executable = executable,
)

providers = [
providers_to_forward = [
library,
source,
archive,
OutputGroupInfo(
cgo_exports = archive.cgo_exports,
compilation_outputs = [archive.data.file],
),
DefaultInfo(
files = depset([executable]),
runfiles = runfiles,
executable = executable,
),
]

# If the binary's linkmode is c-archive or c-shared, expose CcInfo
Expand Down Expand Up @@ -165,11 +167,19 @@ def _go_binary_impl(ctx):
ccinfo = cc_common.merge_cc_infos(
cc_infos = [ccinfo] + [d[CcInfo] for d in source.cdeps],
)
providers.append(ccinfo)
providers_to_forward.append(ccinfo)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe leave this as providers as these providers are still forwarded only in addition to being advertised by the rule itself?


return providers
forwarding_provider = ForwardingPastTransitionProvider(executable = executable, runfiles = runfiles, providers_to_forward = providers_to_forward)
return providers_to_forward + [
forwarding_provider,
DefaultInfo(
files = depset([executable]),
runfiles = runfiles,
executable = executable,
),
]

_go_binary_kwargs = {
go_binary_kwargs = {
"implementation": _go_binary_impl,
"attrs": {
"srcs": attr.label_list(
Expand Down Expand Up @@ -394,9 +404,25 @@ _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_binary = rule(executable = True, **go_binary_kwargs)
go_non_executable_transition_binary = rule(executable = False, **go_binary_kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
go_non_executable_transition_binary = rule(executable = False, **go_binary_kwargs)
go_non_executable_binary = rule(executable = False, **go_binary_kwargs)


go_transition_binary = rule(
implementation = forward_through_transition_impl,
attrs = dicts.add(transition_attrs, {
"is_windows": attr.bool(),
"transition_dep": attr.label(cfg = go_transition),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is only an internal attribute, we could help the IntelliJ plugin here by giving it a name recognized by its aspect, see fmeum/rules_meta@8d57be3.

}),
executable = True,
)
go_non_executable_transition_transition_binary = rule(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
go_non_executable_transition_transition_binary = rule(
go_non_executable_transition_binary = rule(

implementation = forward_through_transition_impl,
attrs = dicts.add(transition_attrs, {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementation and attrs could be extracted into e.g. _go_transition_binary_kwargs.

"is_windows": attr.bool(),
"transition_dep": attr.label(cfg = go_transition),
}),
executable = False,
)

def _go_tool_binary_impl(ctx):
sdk = ctx.attr.sdk[GoSDK]
Expand Down
52 changes: 42 additions & 10 deletions go/private/rules/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,20 @@ load(
)
load(
"//go/private/rules:transition.bzl",
"go_transition_rule",
"ForwardingPastTransitionProvider",
"forward_through_transition_impl",
"go_transition",
"non_go_transition",
"transition_attrs",
)
load(
"//go/private:mode.bzl",
"LINKMODE_NORMAL",
)
load(
"@bazel_skylib//lib:dicts.bzl",
"dicts",
)
load(
"@bazel_skylib//lib:structs.bzl",
"structs",
Expand All @@ -54,6 +61,22 @@ load(
def _testmain_library_to_source(go, attr, source, merge):
source["deps"] = source["deps"] + [attr.library]

go_transition_test = rule(
implementation = forward_through_transition_impl,
attrs = dicts.add(transition_attrs, {
"transition_dep": attr.label(cfg = go_transition),
"is_windows": attr.bool(),
# Workaround for bazelbuild/bazel#6293. See comment in lcov_merger.sh.
"_lcov_merger": attr.label(
executable = True,
default = "//go/tools/builders:lcov_merger",
cfg = "target",
),
}),
executable = True,
test = True,
)

def _go_test_impl(ctx):
"""go_test_impl implements go testing.

Expand Down Expand Up @@ -168,13 +191,8 @@ def _go_test_impl(ctx):
# source file is present, Bazel will set the COVERAGE_OUTPUT_FILE
# environment variable during tests and will save that file to the build
# events + test outputs.
return [
providers_to_forward = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, could stay as providers.

test_archive,
DefaultInfo(
files = depset([executable]),
runfiles = runfiles,
executable = executable,
),
OutputGroupInfo(
compilation_outputs = [internal_archive.data.file],
),
Expand All @@ -187,7 +205,22 @@ def _go_test_impl(ctx):
testing.TestEnvironment(env),
]

_go_test_kwargs = {
forwarding_provider = ForwardingPastTransitionProvider(
executable = executable,
runfiles = runfiles,
providers_to_forward = providers_to_forward,
)

return providers_to_forward + [
forwarding_provider,
DefaultInfo(
files = depset([executable]),
runfiles = runfiles,
executable = executable,
),
]

go_test_kwargs = {
"implementation": _go_test_impl,
"attrs": {
"data": attr.label_list(
Expand Down Expand Up @@ -448,8 +481,7 @@ _go_test_kwargs = {
""",
}

go_test = rule(**_go_test_kwargs)
go_transition_test = go_transition_rule(**_go_test_kwargs)
go_test = 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
Loading