Skip to content

Commit

Permalink
Remove output group for debug files from cc_shared_library
Browse files Browse the repository at this point in the history
It is not necessary to have this mechanism in the cc_shared_library rule
itself. The same data can be obtained by using aspects as shown in
https://github.com/oquenchil/bazel-contrib/tree/main/cc/tools/cc_shared_library
by the static_linker_inputs.bzl and claimed_exports.bzl files.

Removing this code from the cc_shared_library rule simplifies an already
complicated rule and potentially makes the rule more efficient since the
creation of the action for the debug files would use up memory and cpu even
when the debug files weren't actually used. This could be mitigated by a C++
configuration flag but then the debug files couldn't be added to a filegroup if
the flag was disabled.

I couldn't find anyone relying on that output group from within their build.
The Tensorflow codebase doesn't have references to it anywhere.

If you want this functionality please refer to the aspects mentioned above.
Please also keep in mind that
https://github.com/oquenchil/bazel-contrib/tree/main/cc/tools/cc_shared_library
is a personal repository and won't be actively maintained by the Bazel team.
Anything there that is generally useful can be potentially moved to
https://github.com/bazel-contrib and the actual users can help with
maintenance. The Bazel team for now can only commit to the actual core
functionality provided by the cc_* rules (including cc_shared_library) but not
to any auxiliary tools/utilities that are seldom used.

RELNOTES:none
PiperOrigin-RevId: 533990119
Change-Id: I5006113cef845d808f93f183188a4f5022a9c287
  • Loading branch information
oquenchil authored and copybara-github committed May 22, 2023
1 parent 8a07dbe commit 354fd50
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 65 deletions.
23 changes: 4 additions & 19 deletions src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -365,18 +365,7 @@ def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_c

throw_linked_but_not_exported_errors(linked_statically_but_not_exported)

rule_impl_debug_files = None
if cpp_config.experimental_cc_shared_library_debug():
debug_linker_inputs_file = ["Owner: " + str(ctx.label)]
for linker_input in static_linker_inputs:
debug_linker_inputs_file.append(str(linker_input.owner))
link_once_static_libs_debug_file = ctx.actions.declare_file(ctx.label.name + "_link_once_static_libs.txt")
ctx.actions.write(link_once_static_libs_debug_file, "\n".join(debug_linker_inputs_file), False)
transitive_debug_files_list = []
for dep in ctx.attr.dynamic_deps:
transitive_debug_files_list.append(dep[OutputGroupInfo].rule_impl_debug_files)
rule_impl_debug_files = depset([link_once_static_libs_debug_file], transitive = transitive_debug_files_list)
return (cc_common.create_linking_context(linker_inputs = depset(exports_map.values() + static_linker_inputs, order = "topological")), rule_impl_debug_files)
return cc_common.create_linking_context(linker_inputs = depset(exports_map.values() + static_linker_inputs, order = "topological"))

def _create_transitive_linking_actions(
ctx,
Expand Down Expand Up @@ -456,9 +445,8 @@ def _create_transitive_linking_actions(
cc_info = cc_common.merge_cc_infos(cc_infos = [cc_info_without_extra_link_time_libraries, extra_link_time_libraries_cc_info])
cc_linking_context = cc_info.linking_context

rule_impl_debug_files = None
if len(ctx.attr.dynamic_deps) > 0:
cc_linking_context, rule_impl_debug_files = _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_config)
cc_linking_context = _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_config)
link_deps_statically = True
if linking_mode == linker_mode.LINKING_DYNAMIC:
link_deps_statically = False
Expand Down Expand Up @@ -489,7 +477,7 @@ def _create_transitive_linking_actions(
win_def_file = win_def_file,
)
cc_launcher_info = cc_internal.create_cc_launcher_info(cc_info = cc_info_without_extra_link_time_libraries, compilation_outputs = cc_compilation_outputs_with_only_objects)
return (cc_linking_outputs, cc_launcher_info, rule_impl_debug_files, cc_linking_context)
return (cc_linking_outputs, cc_launcher_info, cc_linking_context)

def _use_pic(ctx, cc_toolchain, cpp_config, feature_configuration):
if _is_link_shared(ctx):
Expand Down Expand Up @@ -702,7 +690,7 @@ def cc_binary_impl(ctx, additional_linkopts):
if extra_link_time_libraries != None:
linker_inputs_extra, runtime_libraries_extra = extra_link_time_libraries.build_libraries(ctx = ctx, static_mode = linking_mode != linker_mode.LINKING_DYNAMIC, for_dynamic_library = _is_link_shared(ctx))

cc_linking_outputs_binary, cc_launcher_info, rule_impl_debug_files, deps_cc_linking_context = _create_transitive_linking_actions(
cc_linking_outputs_binary, cc_launcher_info, deps_cc_linking_context = _create_transitive_linking_actions(
ctx,
cc_toolchain,
feature_configuration,
Expand Down Expand Up @@ -815,9 +803,6 @@ def cc_binary_impl(ctx, additional_linkopts):
if generated_def_file != None:
output_groups["def_file"] = depset([generated_def_file])

if rule_impl_debug_files != None:
output_groups["rule_impl_debug_files"] = rule_impl_debug_files

if cc_linking_outputs_binary_library != None:
# For consistency and readability.
library_to_link = cc_linking_outputs_binary_library
Expand Down
13 changes: 0 additions & 13 deletions src/main/starlark/builtins_bzl/common/cc/cc_shared_library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -555,10 +555,8 @@ def _cc_shared_library_impl(ctx):
runfiles = ctx.runfiles(
files = runfiles_files,
)
transitive_debug_files = []
for dep in ctx.attr.dynamic_deps:
runfiles = runfiles.merge(dep[DefaultInfo].data_runfiles)
transitive_debug_files.append(dep[OutputGroupInfo].rule_impl_debug_files)

precompiled_only_dynamic_libraries_runfiles = []
for precompiled_dynamic_library in precompiled_only_dynamic_libraries:
Expand All @@ -572,16 +570,6 @@ def _cc_shared_library_impl(ctx):
for export in deps:
exports[str(export.label)] = True

debug_files = []
exports_debug_file = ctx.actions.declare_file(ctx.label.name + "_exports.txt")
ctx.actions.write(content = "\n".join(["Owner:" + str(ctx.label)] + exports.keys()), output = exports_debug_file)

link_once_static_libs_debug_file = ctx.actions.declare_file(ctx.label.name + "_link_once_static_libs.txt")
ctx.actions.write(content = "\n".join(["Owner:" + str(ctx.label)] + curr_link_once_static_libs_set), output = link_once_static_libs_debug_file)

debug_files.append(exports_debug_file)
debug_files.append(link_once_static_libs_debug_file)

if not semantics.get_experimental_link_static_libraries_once(ctx):
curr_link_once_static_libs_set = {}

Expand All @@ -607,7 +595,6 @@ def _cc_shared_library_impl(ctx):
OutputGroupInfo(
main_shared_library_output = depset(library),
interface_library = depset(interface_library),
rule_impl_debug_files = depset(direct = debug_files, transitive = transitive_debug_files),
),
CcSharedLibraryInfo(
dynamic_deps = merged_cc_shared_library_info,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ load(
":starlark_tests.bzl",
"additional_inputs_test",
"build_failure_test",
"debug_files_test",
"interface_library_output_group_test",
"linking_order_test",
"paths_test",
Expand Down Expand Up @@ -328,7 +327,6 @@ sh_test(
":bar_so",
":binary",
":cc_test",
":debug_files",
":foo_so",
] + select({
":is_bazel": [
Expand All @@ -339,12 +337,6 @@ sh_test(
}),
)

filegroup(
name = "debug_files",
srcs = ["binary"],
output_group = "rule_impl_debug_files",
)

linking_order_test(
name = "linking_action_test",
target = ":foo_so",
Expand Down Expand Up @@ -447,12 +439,6 @@ build_failure_test(
target = "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets:two_dynamic_deps_same_export_in_binary",
)

analysis_test(
name = "debug_files_test",
impl = debug_files_test,
target = ":binary",
)

interface_library_output_group_test(
name = "interface_library_output_group_test",
target = ":foo_so",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,25 +157,6 @@ def _paths_test_macro(name):

paths_test = _paths_test_macro

def _debug_files_test_impl(env, target):
expected_files = [
"bar_so_exports.txt",
"bar_so_link_once_static_libs.txt",
"diff_pkg_so_exports.txt",
"diff_pkg_so_link_once_static_libs.txt",
"foo_so_exports.txt",
"foo_so_link_once_static_libs.txt",
"binary_link_once_static_libs.txt",
]

actual_files = []
for debug_file in target[OutputGroupInfo].rule_impl_debug_files.to_list():
actual_files.append(debug_file.basename)

env.expect.that_collection(expected_files).contains_exactly(actual_files)

debug_files_test = _debug_files_test_impl

def _runfiles_test_impl(env, target):
if not env.ctx.target_platform_has_constraint(env.ctx.attr._is_linux[platform_common.ConstraintValueInfo]):
return
Expand Down

0 comments on commit 354fd50

Please sign in to comment.