fix copy_dynamic_libraries_to_binary for subdirectories

Copybara Import from #623

BEGIN_PUBLIC
fix copy_dynamic_libraries_to_binary for subdirectories (#623)

This is a new instance of a similar problem described in bazelbuild/bazel#12448. In order for windows binaries to run, the (non-system) dlls need to be copied to the same directory. However, the code that determined that was only looking at the package label and the workspace, but not the actual subdirectory. Previously, this was fixed by adding the `lib.is_source` check, but this only solves it for source files.

An example of code that can trigger this is https://github.com/bazel-contrib/rules_foreign_cc/blob/d8e34f6f9b66a13cd845011200d444b060dc1af1/examples/cmake_hello_world_lib/shared/BUILD.bazel

Closes #623
END_PUBLIC

COPYBARA_INTEGRATE_REVIEW=#623 from novas0x2a:fix-windows-dlls 83bc49c
PiperOrigin-RevId: 885077676
Change-Id: Ic94fb3f7488b4ba28e9fb13a7ed68657f70c3a25

Author: novas0x2a

.bazelci/presubmit.yml

@@ -30,6 +30,7 @@ test_targets_bazel_6: &test_targets_bazel_6
   - "-//cc:optional_current_cc_toolchain" # Not supported in Bazel 6
   - "-//tests/rule_based_toolchain/tool_map:_duplicate_action_test_subject" # Intentionally broken rule.
   - "-//tests/system_library:system_library_test" # Should be skipped on Windows and MacOS
+  - "-//tests/cc/common:test_runtime_dynamic_libraries_copy_behavior" # known bug, see https://github.com/bazelbuild/rules_cc/pull/623
 
 buildifier:
   version: latest

cc/private/rules_impl/cc_binary_impl.bzl

@@ -176,24 +176,13 @@ def _collect_runfiles(ctx, feature_configuration, cc_toolchain, libraries, cc_li
 
     return (builder.merge(ctx.runfiles(files = builder_artifacts, transitive_files = depset(builder_transitive_artifacts))), runtime_objects_for_coverage)
 
-def _get_target_sub_dir(target_name):
-    last_separator = target_name.rfind("/")
-    if last_separator == -1:
-        return ""
-    return target_name[0:last_separator]
-
-def _create_dynamic_libraries_copy_actions(ctx, dynamic_libraries_for_runtime):
+def _create_dynamic_libraries_copy_actions(ctx, binary, dynamic_libraries_for_runtime):
     result = []
     for lib in dynamic_libraries_for_runtime:
-        # If the binary and the DLL don't belong to the same package or the DLL is a source file,
-        # we should copy the DLL to the binary's directory.
-        if ctx.label.package != lib.owner.package or ctx.label.workspace_name != lib.owner.workspace_name or lib.is_source:
-            target_name = ctx.label.name
-            target_sub_dir = _get_target_sub_dir(target_name)
-            copy_file_path = lib.basename
-            if target_sub_dir != "":
-                copy_file_path = target_sub_dir + "/" + copy_file_path
-            copy = ctx.actions.declare_file(copy_file_path)
+        # If the binary and the DLL are not in the same directory, copy the DLL
+        # to the binary's directory.
+        if lib.dirname != binary.dirname:
+            copy = ctx.actions.declare_file(lib.basename, sibling = binary)
             ctx.actions.symlink(output = copy, target_file = lib, progress_message = "Copying Execution Dynamic Library")
             result.append(copy)
         else:
@@ -740,7 +729,7 @@ def cc_binary_impl(ctx, additional_linkopts, force_linkstatic = False):
         libraries = []
         for linker_input in linker_inputs:
             libraries.extend(linker_input.libraries)
-        copied_runtime_dynamic_libraries = _create_dynamic_libraries_copy_actions(ctx, _get_dynamic_libraries_for_runtime(is_static_mode, libraries))
+        copied_runtime_dynamic_libraries = _create_dynamic_libraries_copy_actions(ctx, binary, _get_dynamic_libraries_for_runtime(is_static_mode, libraries))
 
     # TODO(b/198254254)(bazel-team): Do we need to put original shared libraries (along with
     # mangled symlinks) into the RunfilesSupport object? It does not seem

tests/cc/common/cc_binary_configured_target_tests.bzl

@@ -1,9 +1,11 @@
 """Tests for cc_binary."""
 
-load("@rules_testing//lib:analysis_test.bzl", "test_suite")
+load("@rules_testing//lib:analysis_test.bzl", "analysis_test", "test_suite")
 load("@rules_testing//lib:truth.bzl", "matching")
-load("@rules_testing//lib:util.bzl", "util")
+load("@rules_testing//lib:util.bzl", "TestingAspectInfo", "util")
 load("//cc:cc_binary.bzl", "cc_binary")
+load("//cc:cc_import.bzl", "cc_import")
+load("//cc:cc_library.bzl", "cc_library")
 load("//tests/cc/testutil:cc_analysis_test.bzl", "cc_analysis_test")
 load("//tests/cc/testutil:cc_binary_target_subject.bzl", "cc_binary_target_subject")
 load("//tests/cc/testutil:link_action_subject.bzl", "link_action_subject")
@@ -125,6 +127,112 @@ def _test_action_graph_impl(env, target):
 
     # TODO: Test stripped action
 
+def _make_dll(name):
+    # make a fake dll so the dll shows up in the output directory (where the
+    # binary will be) instead of as a source file (the way cc_import on its own would)
+    # Example taken from
+    # https://github.com/bazelbuild/bazel/blob/f720ed385e245e292b0afe19ebd84e4283c30565/examples/windows/dll/windows_dll_library.bzl
+    dll = name + ".dll"
+    mask = name + "_mask"
+
+    util.helper_target(
+        cc_binary,
+        name = dll,
+        srcs = ["hello.cc"],
+        linkshared = 1,
+    )
+
+    # Mask the cc_binary behind a cc_import so we can depend on it as a library
+    util.helper_target(
+        cc_import,
+        name = mask,
+        shared_library = dll,
+    )
+
+    # cc_imports are always source files, so make it a generated file again
+    cc_library(
+        name = name,
+        deps = [mask],
+    )
+
+def _test_runtime_dynamic_libraries_copy_behavior(name, **kwargs):
+    sub_dir_lib = name + "/dst/sub/sub_dir_lib"
+    same_dir_lib = name + "/dst/same_dir_lib"
+    binary_target = name + "/dst/hello"
+
+    # project a dll into the same directory as the binary, and a second one in
+    # a subdirectory
+    _make_dll(same_dir_lib)
+    _make_dll(sub_dir_lib)
+
+    util.helper_target(
+        cc_binary,
+        name = binary_target,
+        srcs = ["hello.cc"],
+        deps = [
+            same_dir_lib,
+            sub_dir_lib,
+        ],
+        linkstatic = False,
+    )
+
+    analysis_test(
+        name = name,
+        impl = _test_runtime_dynamic_libraries_copy_behavior_impl,
+        target = binary_target,
+        # TODO: This would be better-done with the mock toolchain, once that is
+        # wired up
+        attrs = {
+            "copy_feature_supported": attr.bool(),
+        },
+        attr_values = {
+            "copy_feature_supported": select({
+                # copy_dynamic_libraries_to_binary is only defined in the
+                # windows toolchain currently
+                "@platforms//os:windows": True,
+                "//conditions:default": False,
+            }),
+            "size": "small",
+        },
+        **kwargs
+    )
+
+def _test_runtime_dynamic_libraries_copy_behavior_impl(env, target):
+    if not env.ctx.attr.copy_feature_supported:
+        return
+
+    test_name = env.ctx.label.name
+
+    expected_copied_library = "tests/cc/common/{name}/dst/sub_dir_lib.dll".format(
+        name = test_name,
+    )
+    expected_same_dir_library = "tests/cc/common/{name}/dst/same_dir_lib.dll".format(
+        name = test_name,
+    )
+
+    # Both libraries should be listed as runtime dependencies, but...
+    expected_libraries = [expected_copied_library, expected_same_dir_library]
+
+    env.expect \
+        .that_target(target) \
+        .output_group("runtime_dynamic_libraries") \
+        .contains_exactly(expected_libraries)
+
+    actions = {}
+    for action in target[TestingAspectInfo].actions:
+        if action.mnemonic != "Symlink":
+            continue
+
+        for output in action.outputs.to_list():
+            if output.short_path in expected_libraries:
+                actions[output.short_path] = action
+
+    # ... only one of the libraries should have a Symlink action (Copying
+    # Execution Dynamic Library) since the other one should already be in the
+    # same dir
+    expected_copies = [expected_copied_library]
+    env.expect.that_dict(actions).keys().contains_exactly(expected_copies)
+
 def cc_binary_configured_target_tests(name):
     test_suite(
         name = name,
@@ -133,5 +241,6 @@ def cc_binary_configured_target_tests(name):
             _test_headers_not_passed_to_linking_action,
             _test_no_duplicate_linkopts,
             _test_action_graph,
+            _test_runtime_dynamic_libraries_copy_behavior,
         ],
     )