Skip to content

Commit

Permalink
Cleanup importpath/importmap handling in go_context (#4052)
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?**
This looks like a 50-100ms speedup from avoiding the array allocation in
infer_importpaths and reduced `getattr` calls. Is `getattr` slower than
direct property access? (If not, we can avoid passing in the values from
go_library)

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

Fixes #

**Other notes for review**
  • Loading branch information
dzbarsky authored Aug 17, 2024
1 parent c8a7ebc commit 6749a38
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 26 deletions.
56 changes: 32 additions & 24 deletions go/private/context.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -319,38 +319,22 @@ def _collect_runfiles(go, data, deps):
[get_source(t).runfiles for t in deps],
)

def _check_importpaths(ctx):
paths = []
p = getattr(ctx.attr, "importpath", "")
if p:
paths.append(p)
p = getattr(ctx.attr, "importmap", "")
if p:
paths.append(p)
paths.extend(getattr(ctx.attr, "importpath_aliases", ()))

for p in paths:
if ":" in p:
fail("import path '%s' contains invalid character :" % p)

def _infer_importpath(ctx, attr):
def _infer_importpath(ctx, embeds, importpath, importmap):
VENDOR_PREFIX = "/vendor/"

# Check if paths were explicitly set, either in this rule or in an
# embedded rule.
attr_importpath = getattr(attr, "importpath", "")
attr_importmap = getattr(attr, "importmap", "")
embed_importpath = ""
embed_importmap = ""
for embed in getattr(attr, "embed", []):
for embed in embeds:
lib = embed[GoLibrary]
if lib.pathtype == EXPLICIT_PATH:
embed_importpath = lib.importpath
embed_importmap = lib.importmap
break

importpath = attr_importpath or embed_importpath
importmap = attr_importmap or embed_importmap or importpath
importpath = importpath or embed_importpath
importmap = importmap or embed_importmap or importpath
if importpath:
return importpath, importmap, EXPLICIT_PATH

Expand Down Expand Up @@ -392,7 +376,14 @@ def get_nogo(go):
else:
return None

def go_context(ctx, attr = None, include_deprecated_properties = True):
def go_context(
ctx,
attr = None,
include_deprecated_properties = True,
importpath = None,
importmap = None,
embed = None,
importpath_aliases = None):
"""Returns an API used to build Go code.
See /go/toolchains.rst#go-context
Expand Down Expand Up @@ -477,9 +468,26 @@ def go_context(ctx, attr = None, include_deprecated_properties = True):
cc_toolchain_files = depset()
cgo_tools = None

_check_importpaths(ctx)
importpath, importmap, pathtype = _infer_importpath(ctx, attr)
importpath_aliases = tuple(getattr(attr, "importpath_aliases", ()))
if importpath == None:
importpath = getattr(attr, "importpath", "")
if ":" in importpath:
fail("import path '%s' contains invalid character :" % importpath)

if importmap == None:
importmap = getattr(attr, "importmap", "")
if ":" in importmap:
fail("import path '%s' contains invalid character :" % importmap)

if importpath_aliases == None:
importpath_aliases = getattr(attr, "importpath_aliases", ())
for p in importpath_aliases:
if ":" in p:
fail("import path '%s' contains invalid character :" % p)

if embed == None:
embed = getattr(attr, "embed", [])

importpath, importmap, pathtype = _infer_importpath(ctx, embed, importpath, importmap)

if include_deprecated_properties:
deprecated_properties = {
Expand Down
8 changes: 7 additions & 1 deletion go/private/rules/library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,13 @@ load(

def _go_library_impl(ctx):
"""Implements the go_library() rule."""
go = go_context(ctx, include_deprecated_properties = False)
go = go_context(
ctx,
include_deprecated_properties = False,
importpath = ctx.attr.importpath,
importmap = ctx.attr.importmap,
embed = ctx.attr.embed,
)
library = go.new_library(go)
source = go.library_to_source(go, ctx.attr, library, ctx.coverage_instrumented())
archive = go.archive(go, source)
Expand Down
2 changes: 1 addition & 1 deletion go/private/rules/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ def _go_test_impl(ctx):
resolve = None,
)
test_deps = external_archive.direct + [external_archive] + ctx.attr._testmain_additional_deps
if ctx.configuration.coverage_enabled:
if go.coverage_enabled:
test_deps.append(go.coverdata)
test_source = go.library_to_source(go, struct(
srcs = [struct(files = [main_go])],
Expand Down

0 comments on commit 6749a38

Please sign in to comment.