Skip to content

Commit

Permalink
Remove more hasattr/getattr calls in go_context (#4054)
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?**
The current logic to grab all the providers is confusing; it runs the
fallback logics even in the case when `go_context_data` is found. Easier
to follow when we pass the data in explicitly, and a bit faster as well.

I also considered making a `go_context_v2` api that doesn't have all the
fallbacks and using that to power `go_context`, but didn't do it for
now.

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

Fixes #

**Other notes for review**
  • Loading branch information
dzbarsky authored Aug 20, 2024
1 parent 756d39c commit f3029e2
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 20 deletions.
42 changes: 25 additions & 17 deletions go/private/context.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,8 @@ def go_context(
importpath = None,
importmap = None,
embed = None,
importpath_aliases = None):
importpath_aliases = None,
go_context_data = None):
"""Returns an API used to build Go code.
See /go/toolchains.rst#go-context
Expand All @@ -390,26 +391,33 @@ def go_context(
attr = ctx.attr
toolchain = ctx.toolchains[GO_TOOLCHAIN]
cgo_context_info = None
go_context_info = None
go_config_info = None
stdlib = None
coverdata = None
nogo = None
if hasattr(attr, "_go_context_data"):
go_context_data = _flatten_possibly_transitioned_attr(attr._go_context_data)

if go_context_data == None:
if hasattr(attr, "_go_context_data"):
go_context_data = _flatten_possibly_transitioned_attr(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]
go_context_info = go_context_data[GoContextInfo]
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 = _flatten_possibly_transitioned_attr(attr._stdlib)[GoStdLib]
else:
go_context_data = _flatten_possibly_transitioned_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 = _flatten_possibly_transitioned_attr(attr._stdlib)[GoStdLib]
go_context_info = go_context_data[GoContextInfo]

mode = get_mode(ctx, toolchain, cgo_context_info, go_config_info)

Expand Down Expand Up @@ -515,8 +523,8 @@ def go_context(
importpath_aliases = importpath_aliases,
pathtype = pathtype,
cgo_tools = cgo_tools,
nogo = nogo,
coverdata = coverdata,
nogo = go_context_info.nogo if go_context_info else None,
coverdata = go_context_info.coverdata if go_context_info else None,
coverage_enabled = ctx.configuration.coverage_enabled,
coverage_instrumented = ctx.coverage_instrumented(),
env = env,
Expand Down
8 changes: 7 additions & 1 deletion go/private/rules/binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,13 @@ _go_cc_aspect = aspect(

def _go_binary_impl(ctx):
"""go_binary_impl emits actions for compiling and linking a go executable."""
go = go_context(ctx, include_deprecated_properties = False)
go = go_context(
ctx,
include_deprecated_properties = False,
importpath = ctx.attr.importpath,
embed = ctx.attr.embed,
go_context_data = ctx.attr._go_context_data,
)

is_main = go.mode.link not in (LINKMODE_SHARED, LINKMODE_PLUGIN)
library = go.new_library(go, importable = False, is_main = is_main)
Expand Down
19 changes: 18 additions & 1 deletion go/private/rules/library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,11 @@ def _go_library_impl(ctx):
include_deprecated_properties = False,
importpath = ctx.attr.importpath,
importmap = ctx.attr.importmap,
importpath_aliases = ctx.attr.importpath_aliases,
embed = ctx.attr.embed,
go_context_data = ctx.attr._go_context_data,
)

library = go.new_library(go)
source = go.library_to_source(go, ctx.attr, library, ctx.coverage_instrumented())
archive = go.archive(go, source)
Expand Down Expand Up @@ -204,8 +207,22 @@ go_library = rule(

# See docs/go/core/rules.md#go_library for full documentation.

def _go_tool_library_impl(ctx):
"""Implements the go_tool_library() rule."""
go = go_context(ctx, include_deprecated_properties = False)

library = go.new_library(go)
source = go.library_to_source(go, ctx.attr, library, ctx.coverage_instrumented())
archive = go.archive(go, source)

return [
library,
source,
archive,
]

go_tool_library = rule(
_go_library_impl,
_go_tool_library_impl,
attrs = {
"data": attr.label_list(allow_files = True),
"srcs": attr.label_list(allow_files = True),
Expand Down
8 changes: 7 additions & 1 deletion go/private/rules/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,13 @@ def _go_test_impl(ctx):
It emits an action to run the test generator, and then compiles the
test into a binary."""

go = go_context(ctx, include_deprecated_properties = False)
go = go_context(
ctx,
include_deprecated_properties = False,
importpath = ctx.attr.importpath,
embed = ctx.attr.embed,
go_context_data = ctx.attr._go_context_data,
)

# Compile the library to test with internal white box tests
internal_library = go.new_library(go, testfilter = "exclude")
Expand Down

0 comments on commit f3029e2

Please sign in to comment.