Skip to content

Commit

Permalink
Update cc_dist_library() to include transitive sources
Browse files Browse the repository at this point in the history 
The cc_dist_library() rule originally included only the sources from
direct dependencies. This resulted in a less than ideal developer
experience, because if you ever added a new cc_library() then you would
have to carefully update the necessary cc_dist_library() targets to
ensure that the change was correctly reflected in the CMake build.

This commit addresses that issue by making cc_dist_library() include
transitive sources. We have to be careful to avoid introducing ODR
violations (e.g. from libprotoc duplicating sources from libprotobuf),
so we introduce a new dist_deps attribute on cc_dist_library(). Anything
in dist_deps is assumed to be covered by a separate cc_dist_library()
and is not included. We also make sure to exclude anything that's not
part of our repo (i.e. Abseil and zlib).
  • Loading branch information
@acozzette
acozzette committed Nov 4, 2022
1 parent 1019b94 commit b146519
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 74 deletions.
56 changes: 17 additions & 39 deletions pkg/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,15 +344,8 @@ cc_dist_library(
}),
tags = ["manual"],
deps = [
"//src/google/protobuf:arena",
"//src/google/protobuf:arena_align",
"//src/google/protobuf:arena_allocation_policy",
"//src/google/protobuf:arena_cleanup",
"//src/google/protobuf:arena_config",
"//src/google/protobuf:protobuf_lite",
"//src/google/protobuf/io",
"//src/google/protobuf/io:io_win32",
"//src/google/protobuf/stubs:lite",
],
)

Expand All @@ -367,33 +360,11 @@ cc_dist_library(
}),
tags = ["manual"],
deps = [
"//src/google/protobuf:arena",
"//src/google/protobuf:arena_align",
"//src/google/protobuf:arena_allocation_policy",
"//src/google/protobuf:arena_cleanup",
"//src/google/protobuf:arena_config",
"//src/google/protobuf:port_def",
"//src/google/protobuf:protobuf_lite",
"//src/google/protobuf:protobuf_nowkt",
"//src/google/protobuf:wkt_cc_proto",
"//src/google/protobuf/compiler:importer",
"//src/google/protobuf/io",
"//src/google/protobuf/io:gzip_stream",
"//src/google/protobuf/io:io_win32",
"//src/google/protobuf/io:printer",
"//src/google/protobuf/io:tokenizer",
"//src/google/protobuf/io:zero_copy_sink",
"//src/google/protobuf/json",
"//src/google/protobuf/json:descriptor_traits",
"//src/google/protobuf/json:lexer",
"//src/google/protobuf/json:message_path",
"//src/google/protobuf/json:parser",
"//src/google/protobuf/json:unparser",
"//src/google/protobuf/json:untyped_message",
"//src/google/protobuf/json:writer",
"//src/google/protobuf/json:zero_copy_buffered_stream",
"//src/google/protobuf/stubs",
"//src/google/protobuf/stubs:lite",
"//src/google/protobuf/util:delimited_message_util",
"//src/google/protobuf/util:differencer",
"//src/google/protobuf/util:field_mask_util",
Expand All @@ -407,32 +378,27 @@ cc_dist_library(
name = "protoc",
tags = ["manual"],
deps = [
"//src/google/protobuf/compiler:code_generator",
"//src/google/protobuf/compiler:command_line_interface",
"//src/google/protobuf/compiler/cpp",
"//src/google/protobuf/compiler/cpp:names",
"//src/google/protobuf/compiler/cpp:names_internal",
"//src/google/protobuf/compiler/csharp",
"//src/google/protobuf/compiler/csharp:names",
"//src/google/protobuf/compiler/java",
"//src/google/protobuf/compiler/java:names",
"//src/google/protobuf/compiler/java:names_internal",
"//src/google/protobuf/compiler/objectivec",
"//src/google/protobuf/compiler/objectivec:line_consumer",
"//src/google/protobuf/compiler/objectivec:names",
"//src/google/protobuf/compiler/objectivec:names_internal",
"//src/google/protobuf/compiler/php",
"//src/google/protobuf/compiler/php:names",
"//src/google/protobuf/compiler/python",
"//src/google/protobuf/compiler/ruby",
],
dist_deps = [
":protobuf",
":protobuf_lite",
],
)

cc_dist_library(
name = "lite_test_util",
testonly = 1,
tags = ["manual"],
deps = ["//src/google/protobuf:lite_test_util"],
dist_deps = [":protobuf_lite"],
)

cc_dist_library(
Expand All @@ -446,6 +412,13 @@ cc_dist_library(
"//src/google/protobuf/compiler:annotation_test_util",
"//src/google/protobuf/compiler/cpp:unittest_lib",
],
dist_deps = [
":common_test",
":lite_test_util",
":protoc",
":protobuf",
":protobuf_lite",
],
)

cc_dist_library(
Expand All @@ -456,6 +429,11 @@ cc_dist_library(
"//src/google/protobuf/compiler:mock_code_generator",
"//src/google/protobuf/testing",
],
dist_deps = [
":protobuf",
":protobuf_lite",
":protoc",
],
)

################################################################################
Expand Down
100 changes: 67 additions & 33 deletions pkg/cc_dist_library.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,16 @@ CcFileList = provider(
)

def _flatten_target_files(targets):
return depset(transitive = [target.files for target in targets])
return depset(transitive = [
target.files
for target in targets
# Filter out targets from external workspaces
if target.label.workspace_name == "" or
target.label.workspace_name == "com_google_protobuf"
])

files = []
for target in targets:
files.extend(target.files.to_list())
return files
def _get_transitive_sources(targets, attr, deps):
return depset(targets, transitive = [getattr(dep[CcFileList], attr) for dep in deps if CcFileList in dep])

def _cc_file_list_aspect_impl(target, ctx):
# Extract sources from a `cc_library` (or similar):
Expand All @@ -165,14 +169,22 @@ def _cc_file_list_aspect_impl(target, ctx):
internal_hdrs.append(src)

return [CcFileList(
hdrs = _flatten_target_files(getattr(rule_attr, "hdrs", depset())),
textual_hdrs = _flatten_target_files(getattr(
rule_attr,
hdrs = _get_transitive_sources(
_flatten_target_files(rule_attr.hdrs).to_list(),
"hdrs",
rule_attr.deps,
),
textual_hdrs = _get_transitive_sources(
_flatten_target_files(rule_attr.textual_hdrs).to_list(),
"textual_hdrs",
depset(),
)),
internal_hdrs = depset(internal_hdrs),
srcs = depset(srcs),
rule_attr.deps,
),
internal_hdrs = _get_transitive_sources(
internal_hdrs,
"internal_hdrs",
rule_attr.deps,
),
srcs = _get_transitive_sources(srcs, "srcs", rule_attr.deps),
)]

cc_file_list_aspect = aspect(
Expand All @@ -198,18 +210,20 @@ Output is CcFileList. Example:
# srcs = depset([File("foo.cc")]),
# )
""",
required_providers = [CcInfo],
implementation = _cc_file_list_aspect_impl,
attr_aspects = ["deps"],
)

################################################################################
# Rule impl
################################################################################

def _collect_inputs(deps):
"""Collects files from a list of immediate deps.
"""Collects files from a list of deps.
This rule collects source files and linker inputs for C++ deps. Only
these immediate deps are considered, not transitive deps.
This rule collects source files and linker inputs transitively for C++
deps.
The return value is a struct with object files (linker inputs),
partitioned by PIC and non-pic, and the rules' source and header files:
Expand All @@ -221,9 +235,8 @@ def _collect_inputs(deps):
)
Args:
deps: Iterable of immediate deps. These will be treated as the "inputs,"
but not the transitive deps.
deps: Iterable of immediate deps, which will be treated as roots to
recurse transitively.
Returns:
A struct with linker inputs, source files, and header files.
"""
Expand Down Expand Up @@ -265,6 +278,27 @@ def _collect_inputs(deps):
),
)

# Given structs a and b returned from _collect_inputs(), returns a copy of a
# but with all files from b subtracted out.
def _subtract_files(a, b):
result_args = {}

top_level_fields = ["objects", "pic_objects"]
for field in top_level_fields:
to_remove = {e: None for e in getattr(b, field)}
result_args[field] = [e for e in getattr(a, field) if not e in to_remove]

cc_file_list_args = {}
file_list_fields = ["srcs", "hdrs", "internal_hdrs", "textual_hdrs"]
for field in file_list_fields:
to_remove = {e: None for e in getattr(b.cc_file_list, field).to_list()}
cc_file_list_args[field] = depset(
[e for e in getattr(a.cc_file_list, field).to_list() if not e in to_remove],
)
result_args["cc_file_list"] = CcFileList(**cc_file_list_args)

return struct(**result_args)

# Implementation for cc_dist_library rule.
def _cc_dist_library_impl(ctx):
cc_toolchain_info = find_cc_toolchain(ctx)
Expand All @@ -274,7 +308,7 @@ def _cc_dist_library_impl(ctx):
cc_toolchain = cc_toolchain_info,
)

inputs = _collect_inputs(ctx.attr.deps)
inputs = _subtract_files(_collect_inputs(ctx.attr.deps), _collect_inputs(ctx.attr.dist_deps))

# For static libraries, build separately with and without pic.

Expand Down Expand Up @@ -346,20 +380,17 @@ Create libraries suitable for distribution.
This rule creates static and dynamic libraries from the libraries listed in
'deps'. The resulting libraries are suitable for distributing all of 'deps'
in a single logical library, for example, in an installable binary package.
Only the targets listed in 'deps' are included in the result (i.e., the
output does not include transitive dependencies), allowing precise control
over the library boundary.
The result includes all transitive dependencies, excluding those reachable
from 'dist_deps' or defined in a separate repository (e.g. Abseil).
The outputs of this rule are a dynamic library and a static library. (If
the build produces both PIC and non-PIC object files, then there is also a
second static library.) The example below illustrates additional details.
This rule is different from Bazel's experimental `shared_cc_library` in
several ways. First, this rule ignores transitive dependencies, which means
that dynamic library dependencies generally need to be specified via
'linkopts'. Second, this rule produces a static archive library in addition
to the dynamic shared library. Third, this rule is not directly usable as a
C++ dependency (although the outputs could be used, e.g., by `cc_import`).
This rule is different from Bazel's experimental `shared_cc_library` in two
ways. First, this rule produces a static archive library in addition to the
dynamic shared library. Second, this rule is not directly usable as a C++
dependency (although the outputs could be used, e.g., by `cc_import`).
Example:
Expand All @@ -372,15 +403,18 @@ Example:
cc_dist_library(
name = "dist",
linkopts = ["-la"], # libdist.so dynamically links against liba.so.
deps = [":b", ":c"], # Output contains b.o and c.o, but not a.o.
deps = [":b", ":c"], # Output contains a.o, b.o, and c.o.
)
""",
attrs = {
"deps": attr.label_list(
doc = ("The list of libraries to be included in the outputs. " +
"Only these targets' compilation outputs will be " +
"included (i.e., the transitive dependencies are not " +
"included in the output)."),
doc = ("The list of libraries to be included in the outputs, " +
"along with their transitive dependencies."),
aspects = [cc_file_list_aspect],
),
"dist_deps": attr.label_list(
doc = ("The list of cc_dist_library dependencies that " +
"should be excluded."),
aspects = [cc_file_list_aspect],
),
"linkopts": attr.string_list(
Expand Down
6 changes: 4 additions & 2 deletions src/file_lists.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ set(libprotobuf_lite_hdrs
${protobuf_SOURCE_DIR}/src/google/protobuf/metadata_lite.h
${protobuf_SOURCE_DIR}/src/google/protobuf/parse_context.h
${protobuf_SOURCE_DIR}/src/google/protobuf/port.h
${protobuf_SOURCE_DIR}/src/google/protobuf/port_def.inc
${protobuf_SOURCE_DIR}/src/google/protobuf/port_undef.inc
${protobuf_SOURCE_DIR}/src/google/protobuf/repeated_field.h
${protobuf_SOURCE_DIR}/src/google/protobuf/repeated_ptr_field.h
${protobuf_SOURCE_DIR}/src/google/protobuf/stubs/callback.h
Expand Down Expand Up @@ -545,6 +547,8 @@ set(lite_test_util_hdrs
${protobuf_SOURCE_DIR}/src/google/protobuf/map_lite_test_util.h
${protobuf_SOURCE_DIR}/src/google/protobuf/map_test_util_impl.h
${protobuf_SOURCE_DIR}/src/google/protobuf/proto3_lite_unittest.inc
${protobuf_SOURCE_DIR}/src/google/protobuf/test_util.inc
${protobuf_SOURCE_DIR}/src/google/protobuf/test_util2.h
${protobuf_SOURCE_DIR}/src/google/protobuf/test_util_lite.h
)

Expand Down Expand Up @@ -574,8 +578,6 @@ set(test_util_hdrs
${protobuf_SOURCE_DIR}/src/google/protobuf/reflection_tester.h
${protobuf_SOURCE_DIR}/src/google/protobuf/test_util.h
${protobuf_SOURCE_DIR}/src/google/protobuf/test_util.inc
${protobuf_SOURCE_DIR}/src/google/protobuf/test_util2.h
${protobuf_SOURCE_DIR}/src/google/protobuf/test_util_lite.h
${protobuf_SOURCE_DIR}/src/google/protobuf/wire_format_unittest.inc
)

Expand Down

0 comments on commit b146519

Please sign in to comment.