Skip to content

Cleanup some cgo handling #4466

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Cleanup some cgo handling #4466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 4 additions & 9 deletions go/private/actions/archive.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
is_external_pkg = is_external_pkg,
)
else:
cgo_deps = depset()
cgo_deps = []
emit_compilepkg(
go,
sources = source.srcs,
Expand Down Expand Up @@ -204,26 +204,21 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
runfiles = source.runfiles,
_validation_output = out_nogo_validation,
_nogo_diagnostics = out_diagnostics,
_cgo_deps = cgo_deps,
_cgo_deps = depset(cgo_deps),
)
x_defs = dict(source.x_defs)
for a in direct:
x_defs.update(a.x_defs)

# Ensure that the _cgo_export.h of the current target comes first when cgo_exports is iterated
# by prepending it and specifying the order explicitly. This is required as the CcInfo attached
# to the archive only exposes a single header rather than combining all headers.
cgo_exports_direct = [out_cgo_export_h] if out_cgo_export_h else []
cgo_exports = depset(direct = cgo_exports_direct, transitive = [a.cgo_exports for a in direct], order = "preorder")
return GoArchive(
source = source,
data = data,
direct = direct,
libs = depset(direct = [out_lib], transitive = [a.libs for a in direct]),
transitive = depset([data], transitive = [a.transitive for a in direct]),
x_defs = x_defs,
cgo_deps = depset(transitive = [cgo_deps] + [a.cgo_deps for a in direct]),
cgo_exports = cgo_exports,
cgo_deps = depset(cgo_deps, transitive = [a.cgo_deps for a in direct]),
cgo_export = out_cgo_export_h,
Copy link
Member

@fmeum fmeum Oct 3, 2025

Choose a reason for hiding this comment

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

I guess this is technically a breaking change since GoArchive is public API? Could we keep this backwards compatible by making this a one-element depset?

Copy link
Contributor Author

@dzbarsky dzbarsky Oct 3, 2025

Choose a reason for hiding this comment

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

I'm not a huge fan of 1-element depsets and code comments that say "this is small so flattening is ok, trust me", they tend to get stale :) I suppose we could add an additional field for it just for backwards-compat, although it feels like anyone consuming it should be able to adjust prety trivially

Really the out_cgo_export_h could have been a plain return from the function, it's just a bit annoying to wire up because the functions are invoked via go.binary calling go.archive.

Copy link
Member

@fmeum fmeum Oct 4, 2025

Choose a reason for hiding this comment

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

@jayconrod Do you think this breakage is minor enough that we can pull it off?

I would want to fix the regressions introduced by the latest minor release before we merge a change that's intentionally breaking though.

Copy link
Collaborator

@jayconrod jayconrod Oct 6, 2025

Choose a reason for hiding this comment

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

I'd lean toward preserving backward compatibility.

I'm not sure I really understand this change though. cgo_exports has a single direct element, but it also contains files from transitive dependencies. You need all of those to include the top-level file. Why drop those? I'm sure it's possible to reassemble the same set, but it seems harder.

runfiles = runfiles,
_headers = headers,
)
27 changes: 12 additions & 15 deletions go/private/rules/binary.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,6 @@ load(
"non_go_transition",
)

_EMPTY_DEPSET = depset([])

def _include_path(hdr):
if not hdr.root.path:
fail("Expected hdr to be a generated file, got source file: " + hdr.path)
Expand All @@ -66,9 +64,10 @@ def _include_path(hdr):

def new_cc_import(
go,
hdrs = _EMPTY_DEPSET,
defines = _EMPTY_DEPSET,
local_defines = _EMPTY_DEPSET,
headers = None,
includes = None,
defines = None,
local_defines = None,
dynamic_library = None,
static_library = None,
alwayslink = False,
Expand All @@ -77,8 +76,8 @@ def new_cc_import(
compilation_context = cc_common.create_compilation_context(
defines = defines,
local_defines = local_defines,
headers = hdrs,
includes = depset([_include_path(hdr) for hdr in hdrs.to_list()]),
headers = headers,
includes = includes,
),
linking_context = cc_common.create_linking_context(
linker_inputs = depset([
Expand Down Expand Up @@ -137,9 +136,7 @@ def _go_binary_impl(ctx):
importable = False,
is_main = is_main,
)
name = ctx.attr.basename
if not name:
name = ctx.label.name
name = ctx.attr.basename or ctx.label.name
executable = None
if ctx.attr.out:
# Use declare_file instead of attr.output(). When users set output files
Expand All @@ -161,7 +158,7 @@ def _go_binary_impl(ctx):
providers = [
archive,
OutputGroupInfo(
cgo_exports = archive.cgo_exports,
cgo_exports = [archive.cgo_export] if archive.cgo_export else [],
compilation_outputs = [archive.data.file],
nogo_fix = [nogo_diagnostics] if nogo_diagnostics else [],
_validation = [validation_output] if validation_output else [],
Expand Down Expand Up @@ -202,14 +199,14 @@ def _go_binary_impl(ctx):
"windows": ["-mthreads"],
}.get(go.mode.goos, ["-pthread"]),
}
cgo_exports = archive.cgo_exports.to_list()
if cgo_exports:
if archive.cgo_export:
header = ctx.actions.declare_file("{}.h".format(name))
ctx.actions.symlink(
output = header,
target_file = cgo_exports[0],
target_file = archive.cgo_export,
)
cc_import_kwargs["hdrs"] = depset([header])
cc_import_kwargs["headers"] = depset([header])
cc_import_kwargs["includes"] = depset([_include_path(header)])
if go.mode.linkmode == LINKMODE_C_SHARED:
cc_import_kwargs["dynamic_library"] = executable
elif go.mode.linkmode == LINKMODE_C_ARCHIVE:
Expand Down
5 changes: 2 additions & 3 deletions go/private/rules/cgo.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ def cgo_configure(go, srcs, cdeps, cppopts, copts, cxxopts, clinkopts):

inputs_direct = []
inputs_transitive = []
deps_direct = []
deps = []
lib_opts = []
runfiles = go._ctx.runfiles(collect_data = True)
seen_alwayslink_libs = {}
Expand All @@ -104,7 +104,7 @@ def cgo_configure(go, srcs, cdeps, cppopts, copts, cxxopts, clinkopts):
inputs_transitive.append(cc_transitive_headers)
cc_libs, cc_link_flags = _cc_libs_and_flags(d)
inputs_direct.extend(cc_libs)
deps_direct.extend(cc_libs)
deps.extend(cc_libs)
cc_defines = d[CcInfo].compilation_context.defines.to_list()
cppopts.extend(["-D" + define for define in cc_defines])
cc_includes = d[CcInfo].compilation_context.includes.to_list()
Expand Down Expand Up @@ -170,7 +170,6 @@ def cgo_configure(go, srcs, cdeps, cppopts, copts, cxxopts, clinkopts):
fail("unknown library has neither cc nor objc providers: %s" % d.label)

inputs = depset(direct = inputs_direct, transitive = inputs_transitive)
deps = depset(direct = deps_direct)

# HACK: some C/C++ toolchains will ignore libraries (including dynamic libs
# specified with -l flags) unless they appear after .o or .a files with
Expand Down
2 changes: 1 addition & 1 deletion go/private/rules/library.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def _go_library_impl(ctx):
extensions = ["go"],
),
OutputGroupInfo(
cgo_exports = archive.cgo_exports,
cgo_exports = [archive.cgo_export] if archive.cgo_export else [],
compilation_outputs = [archive.data.file],
nogo_fix = [nogo_diagnostics] if nogo_diagnostics else [],
_validation = [validation_output] if validation_output else [],
Expand Down
1 change: 0 additions & 1 deletion go/private/rules/test.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,7 +737,6 @@ def _recompile_external_deps(go, external_go_info, internal_archive, library_lab
transitive = depset(direct = [arc_data], transitive = [a.transitive for a in deps]),
x_defs = go_info.x_defs,
cgo_deps = depset(transitive = [arc_data._cgo_deps] + [a.cgo_deps for a in deps]),
cgo_exports = depset(transitive = [a.cgo_exports for a in deps]),
runfiles = go_info.runfiles,
mode = go.mode,
_headers = internal_archive._headers,
Expand Down
4 changes: 2 additions & 2 deletions go/providers.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,9 @@ which is available through the :param:`data` field.
| The direct cgo dependencies of this library. |
| This has the same constraints as things that can appear in the deps of a cc_library_. |
+--------------------------------+-----------------------------------------------------------------+
| :param:`cgo_exports` | :type:`depset of GoInfo` |
| :param:`cgo_export` | :type:`File` |
+--------------------------------+-----------------------------------------------------------------+
| The transitive set of c headers needed to reference exports of this archive. |
| The c headers needed to reference exports of this archive. |
+--------------------------------+-----------------------------------------------------------------+
| :param:`runfiles` | runfiles_ |
+--------------------------------+-----------------------------------------------------------------+
Expand Down