From 68aad18cfc01400bf6c3f447b6cd7d21dcc8f01f Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 30 Jan 2023 08:46:17 -0800 Subject: [PATCH] Rename roots in cc_shared_library to deps The attribute 'roots' was initially called 'exports', however this generated confusion as it did not behave like 'exports' in other rules nor does the cc_shared_library rule guarantee that the symbols from targets in that attribute would be exported. From 'exports' it was renamed 'roots' to express that these are the targets at the top of the tree whose transitive closure would be linked into this shared library (dynamically or statically). However, 'roots' also departs from the standard attribute name used across all rules, 'deps'. 'roots' does not even convey that the attribute accepts other targets. Now that the attribute 'static_deps' has been removed, 'deps' again is the best attribute name we could have. It adds consistency with other rules and makes sure that the attribute assumed by tooling by default to contain the main dependencies is the same. With the --experimental_cc_shared_library flag, the attribute 'deps' will still work. Without the experimental flag, 'deps' must be used. Eventually, we will remove the experimental flag, therefore all targets should rename the attribute. RELNOTES[inc]: The attribute cc_shared_library.roots is renamed to 'deps' PiperOrigin-RevId: 505705317 Change-Id: I69a25f6a835c87626aeeef6c9c3def5c148a31f7 --- .../cc/experimental_cc_shared_library.bzl | 47 +++++++++++++++---- .../test_cc_shared_library/BUILD.builtin_test | 24 +++++----- .../failing_targets/BUILD.builtin_test | 10 ++-- 3 files changed, 54 insertions(+), 27 deletions(-) diff --git a/src/main/starlark/builtins_bzl/common/cc/experimental_cc_shared_library.bzl b/src/main/starlark/builtins_bzl/common/cc/experimental_cc_shared_library.bzl index 457d483e859688..5972527b79876e 100644 --- a/src/main/starlark/builtins_bzl/common/cc/experimental_cc_shared_library.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/experimental_cc_shared_library.bzl @@ -251,6 +251,7 @@ def _filter_inputs( ctx, feature_configuration, cc_toolchain, + deps, transitive_exports, preloaded_deps_direct_labels, link_once_static_libs_map): @@ -260,7 +261,7 @@ def _filter_inputs( graph_structure_aspect_nodes = [] dependency_linker_inputs = [] direct_exports = {} - for export in ctx.attr.roots: + for export in deps: direct_exports[str(export.label)] = True dependency_linker_inputs.extend(export[CcInfo].linking_context.linker_inputs.to_list()) graph_structure_aspect_nodes.append(export[GraphNodeInfo]) @@ -403,16 +404,40 @@ def _get_permissions(ctx): return ctx.attr.permissions return None +def _get_deps(ctx): + if len(ctx.attr.deps) and len(ctx.attr.roots): + fail( + "You are using the attribute 'roots' and 'deps'. 'deps' is the " + + "new name for the attribute 'roots'. The attribute 'roots' will be" + + "removed in the future", + attr = "roots", + ) + + deps = ctx.attr.deps + if not len(deps): + deps = ctx.attr.roots + + return deps + def _cc_shared_library_impl(ctx): semantics.check_experimental_cc_shared_library(ctx) - if len(ctx.attr.static_deps) and not cc_common.check_experimental_cc_shared_library(): - fail( - "This attribute is a no-op and its usage" + - " is forbidden after cc_shared_library is no longer experimental. " + - "Remove it from every cc_shared_library target", - attr = "static_deps", - ) + if not cc_common.check_experimental_cc_shared_library(): + if len(ctx.attr.static_deps): + fail( + "This attribute is a no-op and its usage" + + " is forbidden after cc_shared_library is no longer experimental. " + + "Remove it from every cc_shared_library target", + attr = "static_deps", + ) + if len(ctx.attr.roots): + fail( + "This attribute has been renamed to 'deps'. Simply rename the" + + " attribute on the target.", + attr = "roots", + ) + + deps = _get_deps(ctx) cc_toolchain = cc_helper.find_cpp_toolchain(ctx) feature_configuration = cc_common.configure_features( @@ -424,7 +449,7 @@ def _cc_shared_library_impl(ctx): merged_cc_shared_library_info = _merge_cc_shared_library_infos(ctx) exports_map = _build_exports_map_from_only_dynamic_deps(merged_cc_shared_library_info) - for export in ctx.attr.roots: + for export in deps: # Do not check for overlap between targets matched by the current # rule's exports_filter and what is in exports_map. A library in roots # will have to be linked in statically into the current rule with 100% @@ -457,6 +482,7 @@ def _cc_shared_library_impl(ctx): ctx, feature_configuration, cc_toolchain, + deps, exports_map, preloaded_deps_direct_labels, link_once_static_libs_map, @@ -534,7 +560,7 @@ def _cc_shared_library_impl(ctx): runfiles = runfiles.merge(ctx.runfiles(files = precompiled_only_dynamic_libraries_runfiles)) - for export in ctx.attr.roots: + for export in deps: export_label = str(export.label) if GraphNodeInfo in export and export[GraphNodeInfo].no_exporting: if export_label in curr_link_once_static_libs_set: @@ -654,6 +680,7 @@ cc_shared_library = rule( "preloaded_deps": attr.label_list(providers = [CcInfo]), "win_def_file": attr.label(allow_single_file = [".def"]), "roots": attr.label_list(providers = [CcInfo], aspects = [graph_structure_aspect]), + "deps": attr.label_list(providers = [CcInfo], aspects = [graph_structure_aspect]), "static_deps": attr.string_list(), "user_link_flags": attr.string_list(), "_def_parser": semantics.get_def_parser(), diff --git a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test index 40e8da6f5d7a1f..e9bc51e9fae11c 100644 --- a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test +++ b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test @@ -52,28 +52,28 @@ cc_binary( cc_shared_library( name = "python_module", features = ["windows_export_all_symbols"], - roots = [":a_suffix"], + deps = [":a_suffix"], shared_lib_name = "python_module.pyd", ) cc_shared_library( name = "a_so", features = ["windows_export_all_symbols"], - roots = [":a_suffix"], + deps = [":a_suffix"], ) cc_shared_library( name = "diamond_so", dynamic_deps = [":a_so"], features = ["windows_export_all_symbols"], - roots = [":qux"], + deps = [":qux"], ) cc_shared_library( name = "diamond2_so", dynamic_deps = [":a_so"], features = ["windows_export_all_symbols"], - roots = [":qux2"], + deps = [":qux2"], ) cc_binary( @@ -101,7 +101,7 @@ cc_shared_library( dynamic_deps = ["bar_so"], features = ["windows_export_all_symbols"], preloaded_deps = ["preloaded_dep"], - roots = [ + deps = [ "baz", "foo", "a_suffix", @@ -195,7 +195,7 @@ cc_shared_library( permissions = [ "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:permissions", ], - roots = [ + deps = [ "bar", "bar2", ] + select({ @@ -346,7 +346,7 @@ filegroup( cc_shared_library( name = "direct_so_file", features = ["windows_export_all_symbols"], - roots = [ + deps = [ ":direct_so_file_cc_lib", ], ) @@ -367,7 +367,7 @@ filegroup( cc_shared_library( name = "renamed_so_file", features = ["windows_export_all_symbols"], - roots = [ + deps = [ ":direct_so_file_cc_lib2", ], shared_lib_name = "renamed_so_file.so", @@ -417,23 +417,23 @@ cc_library( cc_shared_library( name = "lib_with_no_exporting_roots_1", - roots = [":static_lib_no_exporting"], + deps = [":static_lib_no_exporting"], ) cc_shared_library( name = "lib_with_no_exporting_roots_2", - roots = [":static_lib_no_exporting"], + deps = [":static_lib_no_exporting"], dynamic_deps = [":lib_with_no_exporting_roots_3"], ) cc_shared_library( name = "lib_with_no_exporting_roots_3", - roots = [":static_lib_no_exporting"], + deps = [":static_lib_no_exporting"], ) cc_shared_library( name = "lib_with_no_exporting_roots", - roots = [ + deps = [ ":static_lib_no_exporting", ":static_lib_exporting", ], diff --git a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test index 4c6860d184a896..6ab7018d6e2f0b 100644 --- a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test +++ b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test @@ -19,7 +19,7 @@ cc_binary( cc_shared_library( name = "should_fail_shared_lib", dynamic_deps = ["//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:bar_so"], - roots = [ + deps = [ ":intermediate", ], tags = TAGS, @@ -34,7 +34,7 @@ cc_library( cc_shared_library( name = "permissions_fail_so", - roots = [ + deps = [ "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:bar", ], tags = TAGS, @@ -69,7 +69,7 @@ cc_shared_library( ":b_so", ":b2_so", ], - roots = [ + deps = [ ":a", ], tags = TAGS, @@ -87,7 +87,7 @@ cc_binary( cc_shared_library( name = "b_so", - roots = [ + deps = [ ":b", ], tags = TAGS, @@ -95,7 +95,7 @@ cc_shared_library( cc_shared_library( name = "b2_so", - roots = [ + deps = [ ":b", ], tags = TAGS,