Skip to content

Commit

Permalink
Speedup compilepkg args handling (#4074)
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?**
A lot of the args between compilepkg and nogo are shared, so we can
reuse the `Args` object. Cleaned up a few other things that showed up in
profiles while I was here.

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

Fixes #

**Other notes for review**

---------

Co-authored-by: Fabian Meumertzheim <[email protected]>
  • Loading branch information
@dzbarsky @fmeum
dzbarsky and fmeum authored Oct 28, 2024
1 parent 97721d0 commit e4484f0
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 69 deletions.
110 changes: 45 additions & 65 deletions go/private/actions/compilepkg.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,13 @@ def emit_compilepkg(
fail("sources is a required parameter")
if out_lib == None:
fail("out_lib is a required parameter")
if bool(nogo) != bool(out_facts):

have_nogo = nogo != None
if have_nogo != (out_facts != None):
fail("nogo must be specified if and only if out_facts is specified")
if bool(nogo) != bool(out_nogo_log):
if have_nogo != (out_nogo_log != None):
fail("nogo must be specified if and only if out_nogo_log is specified")
if bool(nogo) != bool(out_nogo_validation):
if have_nogo != (out_nogo_validation != None):
fail("nogo must be specified if and only if out_nogo_validation is specified")

if cover and go.coverdata:
Expand All @@ -97,52 +99,53 @@ def emit_compilepkg(
inputs_transitive = [sdk.headers, sdk.tools, go.stdlib.libs]
outputs = [out_lib, out_export]

args = go.builder_args(go, "compilepkg", use_path_mapping = True)
args.add_all(sources, before_each = "-src")
args.add_all(embedsrcs, before_each = "-embedsrc", expand_directories = False)
args.add_all(
shared_args = go.builder_args(go, use_path_mapping = True)
shared_args.add_all(sources, before_each = "-src")

compile_args = go.tool_args(go)
compile_args.add_all(embedsrcs, before_each = "-embedsrc", expand_directories = False)
compile_args.add_all(
sources + [out_lib] + embedsrcs,
map_each = _embedroot_arg,
before_each = "-embedroot",
uniquify = True,
expand_directories = False,
)
args.add_all(
compile_args.add_all(
sources + [out_lib],
map_each = _embedlookupdir_arg,
before_each = "-embedlookupdir",
uniquify = True,
expand_directories = False,
)

cover_mode = None
if cover and go.coverdata:
if go.mode.race:
cover_mode = "atomic"
else:
cover_mode = "set"
args.add("-cover_mode", cover_mode)
args.add("-cover_format", go.mode.cover_format)
args.add_all(cover, before_each = "-cover")
shared_args.add("-cover_mode", cover_mode)
compile_args.add("-cover_format", go.mode.cover_format)
compile_args.add_all(cover, before_each = "-cover")

args.add_all(archives, before_each = "-arc", map_each = _archive)
shared_args.add_all(archives, before_each = "-arc", map_each = _archive)
if recompile_internal_deps:
args.add_all(recompile_internal_deps, before_each = "-recompile_internal_deps")
shared_args.add_all(recompile_internal_deps, before_each = "-recompile_internal_deps")
if importpath:
args.add("-importpath", importpath)
shared_args.add("-importpath", importpath)
else:
args.add("-importpath", go.label.name)
shared_args.add("-importpath", go.label.name)
if importmap:
args.add("-p", importmap)
args.add("-package_list", sdk.package_list)
shared_args.add("-p", importmap)
shared_args.add("-package_list", sdk.package_list)

args.add("-lo", out_lib)
args.add("-o", out_export)
compile_args.add("-lo", out_lib)
compile_args.add("-o", out_export)
if out_cgo_export_h:
args.add("-cgoexport", out_cgo_export_h)
compile_args.add("-cgoexport", out_cgo_export_h)
outputs.append(out_cgo_export_h)
if testfilter:
args.add("-testfilter", testfilter)
shared_args.add("-testfilter", testfilter)

link_mode_flag = link_mode_arg(go.mode)

Expand All @@ -156,10 +159,10 @@ def emit_compilepkg(
gc_flags.extend(go.toolchain.flags.compile)
if link_mode_flag:
gc_flags.append(link_mode_flag)
args.add("-gcflags", quote_opts(gc_flags))
compile_args.add("-gcflags", quote_opts(gc_flags))

if link_mode_flag:
args.add("-asmflags", link_mode_flag)
compile_args.add("-asmflags", link_mode_flag)

# cgo and the linker action don't support path mapping yet
# TODO: Remove the second condition after https://github.com/bazelbuild/bazel/pull/21921.
Expand All @@ -175,33 +178,33 @@ def emit_compilepkg(
if nogo:
cgo_go_srcs_for_nogo = go.declare_directory(go, path = out_lib.basename + ".cgo")
outputs.append(cgo_go_srcs_for_nogo)
args.add("-cgo_go_srcs", cgo_go_srcs_for_nogo.path)
compile_args.add("-cgo_go_srcs", cgo_go_srcs_for_nogo.path)
inputs_transitive.append(cgo_inputs)
inputs_transitive.append(go.cc_toolchain_files)
env["CC"] = go.cgo_tools.c_compiler_path
if cppopts:
args.add("-cppflags", quote_opts(cppopts))
compile_args.add("-cppflags", quote_opts(cppopts))
if copts:
args.add("-cflags", quote_opts(copts))
compile_args.add("-cflags", quote_opts(copts))
if cxxopts:
args.add("-cxxflags", quote_opts(cxxopts))
compile_args.add("-cxxflags", quote_opts(cxxopts))
if objcopts:
args.add("-objcflags", quote_opts(objcopts))
compile_args.add("-objcflags", quote_opts(objcopts))
if objcxxopts:
args.add("-objcxxflags", quote_opts(objcxxopts))
compile_args.add("-objcxxflags", quote_opts(objcxxopts))
if clinkopts:
args.add("-ldflags", quote_opts(clinkopts))
compile_args.add("-ldflags", quote_opts(clinkopts))

if go.mode.pgoprofile:
args.add("-pgoprofile", go.mode.pgoprofile)
compile_args.add("-pgoprofile", go.mode.pgoprofile)
inputs_direct.append(go.mode.pgoprofile)

go.actions.run(
inputs = depset(inputs_direct, transitive = inputs_transitive),
outputs = outputs,
mnemonic = "GoCompilePkgExternal" if is_external_pkg else "GoCompilePkg",
executable = go.toolchain._builder,
arguments = [args],
arguments = ["compilepkg", shared_args, compile_args],
env = env,
toolchain = GO_TOOLCHAIN_LABEL,
execution_requirements = execution_requirements,
Expand All @@ -210,14 +213,10 @@ def emit_compilepkg(
if nogo:
_run_nogo(
go,
shared_args = shared_args,
sources = sources,
cgo_go_srcs = cgo_go_srcs_for_nogo,
importpath = importpath,
importmap = importmap,
archives = archives,
recompile_internal_deps = recompile_internal_deps,
cover_mode = cover_mode,
testfilter = testfilter,
out_facts = out_facts,
out_log = out_nogo_log,
out_validation = out_nogo_validation,
Expand All @@ -226,15 +225,11 @@ def emit_compilepkg(

def _run_nogo(
go,
shared_args,
*,
sources,
cgo_go_srcs,
importpath,
importmap,
archives,
recompile_internal_deps,
cover_mode,
testfilter,
out_facts,
out_log,
out_validation,
Expand All @@ -248,30 +243,15 @@ def _run_nogo(
inputs_transitive = [sdk.tools, sdk.headers, go.stdlib.libs]
outputs = [out_facts, out_log]

args = go.builder_args(go, "nogo", use_path_mapping = True)
args.add_all(sources, before_each = "-src")
nogo_args = go.tool_args(go)
if cgo_go_srcs:
inputs_direct.append(cgo_go_srcs)
args.add_all([cgo_go_srcs], before_each = "-ignore_src")
if cover_mode:
args.add("-cover_mode", cover_mode)
if recompile_internal_deps:
args.add_all(recompile_internal_deps, before_each = "-recompile_internal_deps")
args.add_all(archives, before_each = "-arc", map_each = _archive)
if importpath:
args.add("-importpath", importpath)
else:
args.add("-importpath", go.label.name)
if importmap:
args.add("-p", importmap)
args.add("-package_list", sdk.package_list)
if testfilter:
args.add("-testfilter", testfilter)
nogo_args.add_all([cgo_go_srcs], before_each = "-ignore_src")

args.add_all(archives, before_each = "-facts", map_each = _facts)
args.add("-out_facts", out_facts)
args.add("-out_log", out_log)
args.add("-nogo", nogo)
nogo_args.add_all(archives, before_each = "-facts", map_each = _facts)
nogo_args.add("-out_facts", out_facts)
nogo_args.add("-out_log", out_log)
nogo_args.add("-nogo", nogo)

# This action runs nogo and produces the facts files for downstream nogo actions.
# It is important that this action doesn't fail if nogo produces findings, which allows users
Expand All @@ -285,7 +265,7 @@ def _run_nogo(
outputs = outputs,
mnemonic = "RunNogo",
executable = go.toolchain._builder,
arguments = [args],
arguments = ["nogo", shared_args, nogo_args],
env = go.env_for_path_mapping,
toolchain = GO_TOOLCHAIN_LABEL,
execution_requirements = SUPPORTS_PATH_MAPPING_REQUIREMENT,
Expand Down
7 changes: 3 additions & 4 deletions go/private/context.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ def _dirname(file):
def _builder_args(go, command = None, use_path_mapping = False):
args = go.actions.args()
args.use_param_file("-param=%s")
args.set_param_file_format("shell")
if command:
args.add(command)
sdk_root_file = go.sdk.root_file
Expand All @@ -187,14 +186,14 @@ def _builder_args(go, command = None, use_path_mapping = False):
# Use a file rather than goroot as the latter is just a string and thus
# not subject to path mapping.
args.add_all("-goroot", [goroot_file], map_each = _dirname, expand_directories = False)
args.add("-installsuffix", installsuffix(go.mode))
args.add_joined("-tags", go.mode.tags, join_with = ",")
mode = go.mode
args.add("-installsuffix", installsuffix(mode))
args.add_joined("-tags", mode.tags, join_with = ",")
return args

def _tool_args(go):
args = go.actions.args()
args.use_param_file("-param=%s")
args.set_param_file_format("shell")
return args

def _new_library(go, name = None, importpath = None, resolver = None, importable = True, testfilter = None, is_main = False, **kwargs):
Expand Down

0 comments on commit e4484f0

Please sign in to comment.