Gather errors in cc_shared_library before failing.

This CL takes care of errors of the form:
Error in fail: //foo:bar is already linked statically in //baz:qux_shared but not exported

They are now all gathered into a list and the error is thrown only once we know them all.

RELNOTES:none
PiperOrigin-RevId: 466356464
Change-Id: I87c925d0667774247c2a573f319912fd30658623

src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl

@@ -15,7 +15,7 @@
 """cc_binary Starlark implementation replacing native"""
 
 load(":common/cc/semantics.bzl", "semantics")
-load(":common/cc/experimental_cc_shared_library.bzl", "CcSharedLibraryInfo", "GraphNodeInfo", "build_exports_map_from_only_dynamic_deps", "build_link_once_static_libs_map", "merge_cc_shared_library_infos")
+load(":common/cc/experimental_cc_shared_library.bzl", "CcSharedLibraryInfo", "GraphNodeInfo", "build_exports_map_from_only_dynamic_deps", "build_link_once_static_libs_map", "merge_cc_shared_library_infos", "throw_linked_but_not_exported_errors")
 load(":common/cc/cc_helper.bzl", "cc_helper")
 
 CcInfo = _builtins.toplevel.CcInfo
@@ -403,6 +403,7 @@ def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_c
     (link_statically_labels, link_dynamically_labels) = _separate_static_and_dynamic_link_libraries(graph_structure_aspect_nodes, can_be_linked_dynamically)
 
     linker_inputs_seen = {}
+    linked_statically_but_not_exported = {}
     for linker_input in linker_inputs:
         stringified_linker_input = cc_helper.stringify_linker_input(linker_input)
         if stringified_linker_input in linker_inputs_seen:
@@ -411,10 +412,12 @@ def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context, cpp_c
         owner = str(linker_input.owner)
         if owner not in link_dynamically_labels and (owner in link_statically_labels or _get_canonical_form(ctx.label) == owner):
             if owner in link_once_static_libs_map:
-                fail(owner + " is already linked statically in " + link_once_static_libs_map[owner] + " but not exported.")
+                linked_statically_but_not_exported.setdefault(link_once_static_libs_map[owner], []).append(owner)
             else:
                 static_linker_inputs.append(linker_input)
 
+    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)]

src/main/starlark/builtins_bzl/common/cc/experimental_cc_shared_library.bzl

@@ -268,6 +268,7 @@ def _filter_inputs(
     exports = {}
     linker_inputs_seen = {}
     dynamic_only_roots = {}
+    linked_statically_but_not_exported = {}
     for linker_input in dependency_linker_inputs:
         stringified_linker_input = cc_helper.stringify_linker_input(linker_input)
         if stringified_linker_input in linker_inputs_seen:
@@ -279,8 +280,7 @@ def _filter_inputs(
             linker_inputs.append(dynamic_linker_input)
         elif owner in link_statically_labels:
             if owner in link_once_static_libs_map:
-                fail(owner + " is already linked statically in " +
-                     link_once_static_libs_map[owner] + " but not exported")
+                linked_statically_but_not_exported.setdefault(link_once_static_libs_map[owner], []).append(owner)
 
             is_direct_export = owner in direct_exports
             dynamic_only_libraries = []
@@ -345,9 +345,27 @@ def _filter_inputs(
             message += dynamic_only_root + "\n"
         fail(message)
 
+    _throw_linked_but_not_exported_errors(linked_statically_but_not_exported)
     _throw_error_if_unaccounted_libs(unaccounted_for_libs)
     return (exports, linker_inputs, curr_link_once_static_libs_map.keys(), precompiled_only_dynamic_libraries)
 
+def _throw_linked_but_not_exported_errors(error_libs_dict):
+    if not error_libs_dict:
+        return
+
+    error_builder = ["The following libraries were linked statically by different cc_shared_libraries but not exported:\n"]
+    for cc_shared_library_target, error_libs in error_libs_dict.items():
+        error_builder.append("cc_shared_library %s:\n" % str(cc_shared_library_target))
+        for error_lib in error_libs:
+            error_builder.append("  \"%s\",\n" % str(error_lib))
+
+    error_builder.append("If you are sure that the previous libraries are exported by the cc_shared_libraries because:\n")
+    error_builder.append("  1. You have visibility declarations in the source code\n")
+    error_builder.append("  2. Or you are passing a visibility script to the linker to export symbols from them\n")
+    error_builder.append("then add those libraries to roots or exports_filter for each cc_shared_library.\n")
+
+    fail("".join(error_builder))
+
 def _throw_error_if_unaccounted_libs(unaccounted_for_libs):
     if not unaccounted_for_libs:
         return
@@ -654,3 +672,4 @@ for_testing_dont_use_check_if_target_under_path = _check_if_target_under_path
 merge_cc_shared_library_infos = _merge_cc_shared_library_infos
 build_link_once_static_libs_map = _build_link_once_static_libs_map
 build_exports_map_from_only_dynamic_deps = _build_exports_map_from_only_dynamic_deps
+throw_linked_but_not_exported_errors = _throw_linked_but_not_exported_errors

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test

@@ -1,4 +1,4 @@
-load(":starlark_tests.bzl", "additional_inputs_test", "build_failure_test", "debug_files_test", "linking_suffix_test", "paths_test", "runfiles_test", "interface_library_output_group_test")
+load(":starlark_tests.bzl", "additional_inputs_test", "build_failure_test", "debug_files_test", "interface_library_output_group_test", "linking_suffix_test", "paths_test", "runfiles_test")
 
 LINKABLE_MORE_THAN_ONCE = "LINKABLE_MORE_THAN_ONCE"
 
@@ -41,22 +41,22 @@ cc_binary(
 
 cc_shared_library(
     name = "a_so",
-    roots = [":a_suffix"],
     features = ["windows_export_all_symbols"],
+    roots = [":a_suffix"],
 )
 
 cc_shared_library(
     name = "diamond_so",
     dynamic_deps = [":a_so"],
-    roots = [":qux"],
     features = ["windows_export_all_symbols"],
+    roots = [":qux"],
 )
 
 cc_shared_library(
     name = "diamond2_so",
     dynamic_deps = [":a_so"],
-    roots = [":qux2"],
     features = ["windows_export_all_symbols"],
+    roots = [":qux2"],
 )
 
 cc_binary(
@@ -75,12 +75,14 @@ cc_binary(
 cc_shared_library(
     name = "foo_so",
     additional_linker_inputs = select({
-      "//src/conditions:linux": [
-        ":foo.lds",
-        ":additional_script.txt",
-      ],
-      "//conditions:default": []}),
+        "//src/conditions:linux": [
+            ":foo.lds",
+            ":additional_script.txt",
+        ],
+        "//conditions:default": [],
+    }),
     dynamic_deps = ["bar_so"],
+    features = ["windows_export_all_symbols"],
     preloaded_deps = ["preloaded_dep"],
     roots = [
         "baz",
@@ -93,13 +95,13 @@ cc_shared_library(
         "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:prebuilt",
     ],
     user_link_flags = select({
-      "//src/conditions:linux": [
-        "-Wl,-rpath,kittens",
-        "-Wl,--version-script=$(location :foo.lds)",
-        "-Wl,--script=$(location :additional_script.txt)",
-      ],
-      "//conditions:default": []}),
-    features = ["windows_export_all_symbols"],
+        "//src/conditions:linux": [
+            "-Wl,-rpath,kittens",
+            "-Wl,--version-script=$(location :foo.lds)",
+            "-Wl,--script=$(location :additional_script.txt)",
+        ],
+        "//conditions:default": [],
+    }),
 )
 
 cc_library(
@@ -110,15 +112,17 @@ cc_library(
 
 cc_library(
     name = "foo",
-    srcs = ["foo.cc",  "direct_so_file_cc_lib2.h"]
-       + select({
+    srcs = [
+        "foo.cc",
+        "direct_so_file_cc_lib2.h",
+    ] + select({
         "//src/conditions:linux": [":renamed_so_file_copy.so"],
         "//conditions:default": [],
-       }),
+    }),
     hdrs = ["foo.h"],
     defines = select({
-      "//src/conditions:linux": ["IS_LINUX"],
-      "//conditions:default": [],
+        "//src/conditions:linux": ["IS_LINUX"],
+        "//conditions:default": [],
     }),
     deps = [
         "preloaded_dep",
@@ -147,8 +151,8 @@ cc_library(
 cc_library(
     name = "qux",
     srcs = ["qux.cc"],
-    linkstamp = "l.cc",
     hdrs = ["qux.h"],
+    linkstamp = "l.cc",
 )
 
 cc_library(
@@ -168,12 +172,14 @@ config_setting(
 cc_shared_library(
     name = "bar_so",
     additional_linker_inputs = select({
-      "//src/conditions:linux": [":bar.lds",],
-      "//conditions:default": []}),
+        "//src/conditions:linux": [":bar.lds"],
+        "//conditions:default": [],
+    }),
     exports_filter = [
         "bar3",  # Exported transitive dependency
         "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:bar",
     ],
+    features = ["windows_export_all_symbols"],
     permissions = [
         "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:permissions",
     ],
@@ -190,12 +196,11 @@ cc_shared_library(
         "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:qux2",
     ],
     user_link_flags = select({
-      "//src/conditions:linux": [
-        "-Wl,--version-script=$(location :bar.lds)",
-      ],
-      "//conditions:default": [],
+        "//src/conditions:linux": [
+            "-Wl,--version-script=$(location :bar.lds)",
+        ],
+        "//conditions:default": [],
     }),
-    features = ["windows_export_all_symbols"],
 )
 
 cc_library(
@@ -250,12 +255,13 @@ sh_test(
         ":cc_test",
         ":debug_files",
         ":foo_so",
-    ] +  select({
+    ] + select({
         ":is_bazel": [
-           "@bazel_tools//tools/bash/runfiles",
+            "@bazel_tools//tools/bash/runfiles",
         ],
         "//conditions:default": [
-        ],})
+        ],
+    }),
 )
 
 filegroup(
@@ -266,29 +272,40 @@ filegroup(
 
 linking_suffix_test(
     name = "linking_action_test",
-    target_under_test = ":foo_so",
     # TODO(bazel-team): Support this test on Windows and Mac.
     is_linux = select({
         "//src/conditions:linux": True,
-        "//conditions:default": False}),
+        "//conditions:default": False,
+    }),
+    target_under_test = ":foo_so",
 )
 
 additional_inputs_test(
     name = "additional_inputs_test",
-    target_under_test = ":foo_so",
     # TODO(bazel-team): Support this test on Windows and Mac.
     is_linux = select({
         "//src/conditions:linux": True,
-        "//conditions:default": False}),
+        "//conditions:default": False,
+    }),
+    target_under_test = ":foo_so",
 )
 
 build_failure_test(
-    name = "link_once_repeated_test",
-    message = "already linked statically in " +
-              "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:foo_so but not exported.",
+    name = "link_once_repeated_test_binary",
+    messages = [
+        "cc_shared_library/test_cc_shared_library:barX\",",
+    ],
     target_under_test = "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets:should_fail_binary",
 )
 
+build_failure_test(
+    name = "link_once_repeated_test_shared_lib",
+    messages = [
+        "cc_shared_library/test_cc_shared_library:barX\",",
+    ],
+    target_under_test = "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets:should_fail_shared_lib",
+)
+
 paths_test(
     name = "path_matching_test",
 )
@@ -307,7 +324,10 @@ build_failure_test(
 
 cc_library(
     name = "prebuilt",
-    srcs = [":just_main_output", "direct_so_file_cc_lib.h"],
+    srcs = [
+        "direct_so_file_cc_lib.h",
+        ":just_main_output",
+    ],
 )
 
 filegroup(
@@ -318,10 +338,10 @@ filegroup(
 
 cc_shared_library(
     name = "direct_so_file",
+    features = ["windows_export_all_symbols"],
     roots = [
         ":direct_so_file_cc_lib",
     ],
-    features = ["windows_export_all_symbols"],
 )
 
 genrule(
@@ -339,21 +359,27 @@ filegroup(
 
 cc_shared_library(
     name = "renamed_so_file",
+    features = ["windows_export_all_symbols"],
     roots = [
         ":direct_so_file_cc_lib2",
     ],
     shared_lib_name = "renamed_so_file.so",
-    features = ["windows_export_all_symbols"],
 )
 
 cc_library(
     name = "direct_so_file_cc_lib",
-    srcs = ["direct_so_file_cc_lib.cc", "direct_so_file_cc_lib.h"],
+    srcs = [
+        "direct_so_file_cc_lib.cc",
+        "direct_so_file_cc_lib.h",
+    ],
 )
 
 cc_library(
     name = "direct_so_file_cc_lib2",
-    srcs = ["direct_so_file_cc_lib2.cc", "direct_so_file_cc_lib2.h"],
+    srcs = [
+        "direct_so_file_cc_lib2.cc",
+        "direct_so_file_cc_lib2.h",
+    ],
 )
 
 cc_shared_library_permissions(
@@ -384,18 +410,20 @@ debug_files_test(
 
 interface_library_output_group_test(
     name = "interface_library_output_group_test",
-    target_under_test = ":foo_so",
     is_windows = select({
         "//src/conditions:windows": True,
-        "//conditions:default": False}),
+        "//conditions:default": False,
+    }),
+    target_under_test = ":foo_so",
 )
 
 runfiles_test(
     name = "runfiles_test",
-    target_under_test = ":python_test",
     is_linux = select({
         "//src/conditions:linux": True,
-        "//conditions:default": False}),
+        "//conditions:default": False,
+    }),
+    target_under_test = ":python_test",
 )
 
 build_failure_test(

src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test

@@ -9,11 +9,29 @@ TAGS = [
 
 cc_binary(
     name = "should_fail_binary",
-    dynamic_deps = ["//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:foo_so"],
+    dynamic_deps = ["//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:bar_so"],
     tags = TAGS,
     deps = [
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:foo",
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:qux",
+        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:barX",
+    ],
+)
+
+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 = [
+        ":intermediate",
+    ],
+    static_deps = [
+        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:barX",
+    ],
+    tags = TAGS,
+)
+
+cc_library(
+    name = "intermediate",
+    deps = [
+        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:barX",
     ],
 )